src/cyw43_ll: parse RSN/WPA IEs in wifi scans results

[r-c-n]

Add a function (cyw43_ll_wifi_parse_ie_auth) to parse the vendor-specific WPA IEs and RSN IEs received during wifi scans.

Define a more complete set of security and authentication flags and redefine the CYW43_AUTH_ macros in terms of these new flags.

Return the AP security and authentication capabilities as a 32 bit field in cyw43_ev_scan_result_t.auth_mode (previously an 8 bit field). Note that this change may break compatibility with drivers that use the scan feature.

[r-c-n]

Some notes:

• I could only test this on RSN-capable APs. Although the parsing of both RSN and WPA IEs should be similar, it'd be good to test on older APs as well to have proper validation.
• I couldn't test every authentication and security mode combination, but the more common ones (open, WPA2-PSK, WPA3-SAE) were tested ok.
• This possibly addresses issue auth_mode in cyw43_ev_scan_result_t is wrong #141

[peterharperuk]

Can you check this change doesn't break the rssi value? I seem to remember (my memory might be failing me) that the driver expects this structure to match the BSS info structure from the 43439 chip. auth_mode is actually uint8_t dtim_period, So changing this to uint32_t might trample over the rssi value (if my suspicions are correct)?

[r-c-n]

Good catch and thanks for reviewing! Yes, you're right. I just pushed an updated version that moves the auth_mode field to another place in the struct to keep the integrity of the data. I picked a 4-byte reserved region that shouldn't clash with anything currently used by the driver.

[peterharperuk]

Similarly the [7] here looks odd. I would make it [12] as the 4th word is reserved and so definitely unused.
You would earn brownie points for defining everything properly - here's my effort...

// This is a "better" version of cyw43_ev_scan_result_t
typedef struct detailed_scan_result {
uint32_t buf_len;
uint32_t version;
uint16_t sync_id;
uint16_t bss_count;
uint32_t bss_ver; // ie offset from here (12 bytes)
uint32_t length;
uint8_t bssid[6];
uint16_t beacon_period;
uint16_t capability;
uint8_t ssid_len;
uint8_t ssid[32];
uint8_t pad1[1];
uint32_t rates[5];
uint16_t channel;
uint16_t atim_win;
uint8_t dtim_period; // was auth_mode
uint8_t pad2[1];
int16_t rssi;
uint8_t phy_noise;
uint8_t n_cap;
uint8_t pad3[2];
uint32_t nbss_cap;
uint8_t ctl_ch;
uint8_t pad4[3];
uint32_t auth_mode; // using reserved word
uint8_t flags;
uint8_t reserved[3];
uint32_t basic_mcs[4];
uint16_t ie_offset; // offset of ie data from bss_ver
uint8_t pad5[2];
uint32_t ie_len;
uint16_t snr;
} detailed_scan_result_t;

[r-c-n]

That's interesting, the struct you proposed seems to be incompatible to the reference I'm using. Here's the cyw43_scan_result_internal_t struct in cyw43_ll.c:

cyw43-driver/src/cyw43_ll.c

Line 503 in 055d642

typedef struct _cyw43_scan_result_internal_t {

offset	cyw43 driver
0	version (4 bytes)
4	length (4 bytes)
8	bssid (6 bytes)
14	beacon_period (2 bytes)
16	capability (2 bytes)
18	ssid_len (1 byte)
19	ssid (32 bytes)
51	rateset_count (4 bytes)
55	rateset_rates (16 bytes)
71	chanspec (2 bytes)
73	atim_window (2 bytes)
75	dtim_period (1 byte)
76	rssi (2 bytes)
78	phy_noise (1 byte)
79	n_cap (1 byte)
80	nbss_cap (4 bytes)
84	ctl_ch (1 byte)
85	reserved32 (4 bytes)
89	flags (1 byte)
90	reserved (3 bytes)
93	basic_mcs (16 bytes)
109	ie_offset (2 bytes)
111	ie_length (4 bytes)
115	SNR (2 bytes)

Which matches the struct in the equivalent Linux driver: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h?h=v7.1#n314

The struct you suggested has a slightly different layout starting at offset 51:

offset	suggested
0	version (4 bytes)
4	length (4 bytes)
8	bssid (6 bytes)
14	beacon_period (2 bytes)
16	capability (2 bytes)
18	ssid_len (1 byte)
19	ssid (32 bytes)
51	pad1 (1 byte)
52	rates (20 bytes)
72	channel (2 bytes)
74	atim_win (2 bytes)
76	dtim_period (1 byte)
77	pad2 (1 byte)
78	rssi (2 bytes)
80	phy_noise (1 byte)
81	n_cap (1 byte)
82	pad3 (2 bytes)
84	nbss_cap (4 bytes)
88	ctl_ch (1 byte)
89	pad4 (3 bytes)
92	auth_mode / reserved (4 bytes)
96	flags (1 byte)
97	reserved (3 bytes)
100	basic_mcs (16 bytes)
116	ie_offset (2 bytes)
118	pad5 (2 bytes)
120	ie_len (4 bytes)
124	snr (2 bytes)

Which seems to be based on the Infineon WHD expansion driver: https://github.com/Infineon/whd-expansion/blob/0f13160971d02e7b1d4bd569da0d5eb96f2692d9/WHD/COMPONENT_WIFI5/inc/whd_types.h#L840

Maybe different versions of the wifi firmware provide different BSS info struct formats? I've only checked this driver with the provided firmware, have you tested it on more recent versions (newer than Version: 7.95.49)?

[r-c-n]

In any case, probably a cleaner solution would be to provide the scan result info in a separate driver struct instead of fitting additional data in unused parts of the buffer given by the hardware, although that change will have greater impact on the driver. For this particular use case, finding 4 unused bytes in the buffer will do.

[peterharperuk]

Even in your reference - I'd expect rateset_count (a uint32_t?) to be have to be aligned on word boundary. So the compiler will add a byte of padding to move it to an offset of 52? (I could still be wrong of course).

[r-c-n]

Yeah, probably the paddings are there to account for field alignment... just checking now, the structs aren't packed. I'll push a fix. Thanks!

[r-c-n]

By the way, wrt defining a fully-detailed version of cyw43_ev_scan_result_t. The driver purposedly keeps a detailed cyw43_scan_result_internal_t version for parsing and publishes a reduced cyw43_ev_scan_result_t struct with only the fields relevant to consumers of the driver API.

I don't mind defining a single complete version as the public-facing struct, but in principle I'd keep the original intention of the driver and avoid adding all the bss info fields to the cyw43_ev_scan_result_t struct. Do you really prefer to provide all the fields in the public struct?

[peterharperuk]

It's probably better to keep things simple - I'm complicating things unnecessarily :)