[Alex Gavin <a_gavin@xxxxxxxxxx>]

On 7/11/25 12:57, Pablo Martin-Gomez wrote:
> Hello, 
> 
> My comments are in-lined in your mail/patch. (Open my original mail in text mode, it will be easier
> to find them.)

Apologies, I missed that. Thanks for the feedback and quick response.

> 
> 
> Le ven. 11 juil. 2025, 21:52, Alex Gavin <a_gavin@xxxxxxxxxx <mailto:a_gavin@xxxxxxxxxx>> a écrit :
> 
>     On 7/11/25 11:34, Pablo MARTIN-GOMEZ wrote:
>     > Hi,
>     >
>     > There is two big functional and a 802.11 issues in your patch. Fix those and you should having a
>     > functioning patch.
> 
>     I understand that the contribution process requires submission through merge request on GitLab.
>     Possibly it is not clear, but I posted this WIP patch to seek review for an issue I'm running into
>     while implementing the nl80211 cipher suites dissector.
> 
>     Details are in the commit message below in addition to some 'TODOs' that I intend to resolve. Aside
>     from the functional issue I pointed in the commit message and inline comments, it's not clear what
>     issues you're referencing.
> 
>     One additional item I'm seeking feedback on, aside from patch functionality, is 'AKMS_*' definitions
>     location. They are presently in the 'packet-ieee80211.c' dissector. I'd like to move them to the
>     'packet-ieee80211.h' file which is already included by the nl80211 dissector.
>     > On 11/07/2025 00:56, Alex Gavin via Wireshark-dev wrote:
>     >> Presently this dissection only properly dissects one
>     >> cipher suite of the several that are in the message.
>     >>
>     >> Implementation inspired by dissection in 'ieee80211_tag_ext_supp_rates()'
>     >> in epan/dissectors/packet-ieee80211.c
>     >>
>     >> For context, each cipher suite is a u32 in a byte array. When
>     >> running 'iw phy0 info' in my system, the byte array is 44 bytes
>     >> total (the full NL80211_ATTR_CIPHER_SUITES is 48 bytes
>     >> including length and attribute, both 2 bytes each).
>     >>
>     >> For this attribute, expected parsing should grab u32 (4 byte)
>     >> chunks of the byte array, 4 bytes at a time. However, with this
>     >> patch, the first cipher suite successfully printed 'WPA (0x000fac01)',
>     >> but I get the following error:
>     >>
>     >> [Expert Info (Warning/Malformed): Trying to fetch an unsigned integer with length 44]
>     >>
>     >> Seemingly, this implementation is grabbing the full length of the
>     >> attribute, rather than the 4 bytes as configured in both the call to
>     >> 'proto_tree_add_item()' and associated header field definition.
>     >>
>     >> Steps to reproduce:
>     >> 1. sudo ip link add nlmon0 type nlmon
>     >> 2. sudo ip link set up dev nlmon0
>     >> 3. Run Wireshark w/ this patch
>     >> 4. iw phy0 info
>     >> 5. Filter for 'nl80211.attr_type == 57' (NL80211_ATTR_CIPHER_SUITES)
>     >> ---
>     >>   epan/dissectors/packet-netlink-nl80211.c | 78 ++++++++++++++++++++++++
>     >>   1 file changed, 78 insertions(+)
>     >>
>     >> diff --git a/epan/dissectors/packet-netlink-nl80211.c b/epan/dissectors/packet-netlink-nl80211.c
>     >> index 9e7fac5a31..f03dd7f008 100644
>     >> --- a/epan/dissectors/packet-netlink-nl80211.c
>     >> +++ b/epan/dissectors/packet-netlink-nl80211.c
>     >> @@ -27,6 +27,55 @@ typedef struct  {
>     >>   static dissector_handle_t ieee80211_handle;
>     >>   static dissector_table_t ieee80211_tag_dissector_table;
>     >>   +// TODO: These are defined in packet-ieee80211.c
>     >> +#define AKMS_NONE                 0x000FAC00
>     >> +#define AKMS_WPA                  0x000FAC01
>     >> +#define AKMS_PSK                  0x000FAC02
>     >> +#define AKMS_FT_IEEE802_1X        0x000FAC03
>     >> +#define AKMS_FT_PSK               0x000FAC04
>     >> +#define AKMS_WPA_SHA256           0x000FAC05
>     >> +#define AKMS_PSK_SHA256           0x000FAC06
>     >> +#define AKMS_TDLS                 0x000FAC07
>     >> +#define AKMS_SAE                  0x000FAC08
>     >> +#define AKMS_FT_SAE               0x000FAC09
>     >> +#define AKMS_AP_PEER_KEY          0x000FAC0A
>     >> +#define AKMS_WPA_SHA256_SUITEB    0x000FAC0B
>     >> +#define AKMS_WPA_SHA384_SUITEB    0x000FAC0C
>     >> +#define AKMS_FT_IEEE802_1X_SHA384 0x000FAC0D
>     >> +#define AKMS_FILS_SHA256          0x000FAC0E
>     >> +#define AKMS_FILS_SHA384          0x000FAC0F
>     >> +#define AKMS_FT_FILS_SHA256       0x000FAC10
>     >> +#define AKMS_FT_FILS_SHA384       0x000FAC11
>     >> +#define AKMS_OWE                  0x000FAC12
>     >> +#define AKMS_SAE_GROUP_DEPEND     0x000FAC18
>     >> +#define AKMS_FT_SAE_GROUP_DEPEND  0x000FAC19
>     >> +
>     >> +static const value_string ws_nl80211_cipher_suites_vals[] = {
>     >> +    { AKMS_NONE,                 "NONE" },
>     >> +    { AKMS_WPA,                  "WPA" },
>     >> +    { AKMS_PSK,                  "PSK" },
>     >> +    { AKMS_FT_IEEE802_1X,        "FT IEEE802.1X" },
>     >> +    { AKMS_FT_PSK,               "FT PSK" },
>     >> +    { AKMS_WPA_SHA256,           "WPA SHA256" },
>     >> +    { AKMS_PSK_SHA256,           "PSK SHA256" },
>     >> +    { AKMS_TDLS,                 "TDLS" },
>     >> +    { AKMS_SAE,                  "SAE" },
>     >> +    { AKMS_FT_SAE,               "FT SAE"},
>     >> +    { AKMS_AP_PEER_KEY,          "AP PEER KEY" },
>     >> +    { AKMS_WPA_SHA256_SUITEB,    "WPA SHA256 SUITEB" },
>     >> +    { AKMS_WPA_SHA384_SUITEB,    "WPA SHA256 SUITEB" },
>     >> +    { AKMS_FT_IEEE802_1X_SHA384, "FT IEEE8021.X SHA384" },
>     >> +    { AKMS_FILS_SHA256,          "FILS SHA256" },
>     >> +    { AKMS_FILS_SHA384,          "FILS SHA384" },
>     >> +    { AKMS_FT_FILS_SHA256,       "FT FILS SHA256" },
>     >> +    { AKMS_FT_FILS_SHA384,       "FT FILS SHA384" },
>     >> +    { AKMS_OWE,                  "OWE" },
>     >> +    { AKMS_SAE_GROUP_DEPEND,     "SAE GROUP DEPEND" },
>     >> +    { AKMS_FT_SAE_GROUP_DEPEND,  "FT SAE GROUP DEPEND" },
>     >> +    { 0, NULL }
>     >> +};
>     >> +static value_string_ext ws_nl80211_cipher_suites_vals_ext =
>     >> VALUE_STRING_EXT_INIT(ws_nl80211_cipher_suites_vals);
>     > You're mixing AKMS and cipher suites. Check Table 9-149 in 802.11-2020 or
>     ieee80211_rsn_cipher_vals
>     > in packet-ieee80211.c to get the cipher suites values.
>     >> +
>     >>   /* Extracted using tools/generate-nl80211-fields.py */
>     >>   /* Definitions from linux/nl80211.h {{{ */
>     >>   enum ws_nl80211_commands {
>     >> @@ -4170,10 +4219,12 @@ static int hf_nl80211_ifname;
>     >>   static int hf_nl80211_mac;
>     >>   static int hf_nl80211_alpha2;
>     >>   static int hf_nl80211_dbm;
>     >> +static int hf_nl80211_cipher_suites;
>     >>     static int ett_nl80211;
>     >>   static int ett_nl80211_frame;
>     >>   static int ett_nl80211_tag;
>     >> +static int ett_nl80211_cipher_suites;
>     >>     static int
>     >>   dissect_nl80211_generic(tvbuff_t *tvb, void *data _U_, struct packet_netlink_data *nl_data,
>     >> proto_tree *tree, int nla_type _U_, int offset, int len)
>     >> @@ -4419,6 +4470,24 @@ dissect_nl80211_sta_info(tvbuff_t *tvb, void *data, struct
>     >> packet_netlink_data *
>     >>       return offset;
>     >>   }
>     >>   +static int
>     >> +dissect_nl80211_cipher_suites(tvbuff_t *tvb, void *data _U_, struct packet_netlink_data *nl_data
>     >> _U_, proto_tree *tree, int nla_type _U_, int offset, int len)
>     >> +{
>     >> +    if (len < 4) {
>     >> +        // TODO: Error here, expect at least one u32
>     >> +        //expert_add_info_format(pinfo, field_data->item_tag_length, &ei_ieee80211_tag_length,
>     >> +        //                    "Tag length %u too short, must be greater than 0",
>     >> +        //                    tag_len);
>     >> +        return offset;
>     >> +    }
>     >> +
>     >> +    while (offset < len) {
>     >> +        proto_tree_add_item(tree, hf_nl80211_cipher_suites, tvb, offset, 4, ENC_LITTLE_ENDIAN);
>     >> +        offset += 1;
>     > You made a copy paste error here; the offset increment you want here is the size of a u32, so 4.
>     >> +    }
>     >> +
>     >> +    return offset;
>     >> +}
>     >>     static int
>     >>   dissect_nl80211_attrs(tvbuff_t *tvb, void *data, struct packet_netlink_data *nl_data,
>     proto_tree
>     >> *tree, int nla_type, int offset, int len)
>     >> @@ -4472,6 +4541,7 @@ dissect_nl80211_attrs(tvbuff_t *tvb, void *data, struct packet_netlink_data
>     >> *nl_
>     >>           { WS_NL80211_ATTR_REG_TYPE, &hf_nl80211_reg_type, NULL, NULL },
>     >>           { WS_NL80211_ATTR_AUTH_TYPE, &hf_nl80211_auth_type, NULL, NULL },
>     >>           { WS_NL80211_ATTR_KEY_TYPE, &hf_nl80211_key_type, NULL, NULL },
>     >> +        { WS_NL80211_ATTR_CIPHER_SUITES, &hf_nl80211_cipher_suites, &ett_nl80211_cipher_suites,
>     >> dissect_nl80211_cipher_suites },
>     > This table is only for simple values that do not need a dissector. Parsing
>     > WS_NL80211_ATTR_CIPHER_SUITES is very specific; you'll have to handle it specifically inside the
>     > switch case of dissect_nl80211_attrs.
>     >>           { WS_NL80211_ATTR_USE_MFP, &hf_nl80211_mfp, NULL, NULL },
>     >>           { WS_NL80211_ATTR_PS_STATE, &hf_nl80211_ps_state, NULL, NULL },
>     >>           { WS_NL80211_ATTR_WIPHY_TX_POWER_SETTING, &hf_nl80211_tx_power_setting, NULL, NULL },
>     >> @@ -4618,6 +4688,13 @@ proto_register_netlink_nl80211(void)
>     >>                 FT_INT32, BASE_DEC, NULL, 0x00,
>     >>                 NULL, HFILL }
>     >>           },
>     >> +
>     >> +        // TODO: Add to script?
>     >> +        { &hf_nl80211_cipher_suites,
>     >> +            { "Cipher Suite", "nl80211.cipher_suite",
>     >> +              FT_UINT32, BASE_HEX | BASE_EXT_STRING, &ws_nl80211_cipher_suites_vals_ext, 0x0,
>     >> +              NULL, HFILL },
>     >> +        },
>     >>   /* Extracted using tools/generate-nl80211-fields.py */
>     >>   /* Definitions from linux/nl80211.h {{{ */
>     >>           { &hf_nl80211_commands,
>     >> @@ -5371,6 +5448,7 @@ proto_register_netlink_nl80211(void)
>     >>           &ett_nl80211,
>     >>           &ett_nl80211_frame,
>     >>           &ett_nl80211_tag,
>     >> +        &ett_nl80211_cipher_suites,
>     >>   /* Extracted using tools/generate-nl80211-fields.py */
>     >>   /* Definitions from linux/nl80211.h {{{ */
>     >>           &ett_nl80211_commands,
>     >
>     > BR
>     >
>     > Pablo MG
>     >
>