Hi Dan, Thanks for reviewing my patch! Comments below: On 1/3/2023 9:47 AM, Dan Williams wrote:
On Mon, 2023-01-02 at 15:47 -0800, Doug Brown wrote:The existing code only converts the first IE to a TLV, but it returns a value that takes the length of all IEs into account. When there is more than one IE (which happens with modern wpa_supplicant versions for example), the returned length is too long and extra junk TLVs get sent to the firmware, resulting in an association failure. Fix this by returning a length that only factors in the single IE that was converted. The firmware doesn't seem to support the additional IEs, so there is no value in trying to convert them to additional TLVs. Fixes: e86dc1ca4676 ("Libertas: cfg80211 support") Signed-off-by: Doug Brown <doug@xxxxxxxxxxxxx> --- drivers/net/wireless/marvell/libertas/cfg.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c index 3e065cbb0af9..fcc5420ec7ea 100644 --- a/drivers/net/wireless/marvell/libertas/cfg.c +++ b/drivers/net/wireless/marvell/libertas/cfg.c @@ -432,10 +432,9 @@ static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len) *tlv++ = 0; tlv_len = *tlv++ = *ie++; *tlv++ = 0; - while (tlv_len--) - *tlv++ = *ie++; - /* the TLV is two bytes larger than the IE */ - return ie_len + 2; + memcpy(tlv, ie, tlv_len); + /* the TLV has a four-byte header */ + return tlv_len + 4;Since you're removing ie_len usage in the function, you might as well remove it from the function's arguments.
That's an excellent point. Thinking about it further after your questions below, maybe we should keep it around and use it to validate how far we are allowed to go into "ie" though...technically the existing code could overflow the buffer with a malformed IE.
Can you also update the comments to say something like "only copy the first IE into the command buffer".
Will do.
Lastly, should you check the IE to make sure you're copying the WPA or WMM IE that the firmware expects? What other IEs does wpa_supplicant/cfg80211 add these days?
I was wondering about that too. I wasn't sure exactly which potential IEs are the ones I should be looking for during this check. I've seen "RSN Information" = 48 during my testing with WPA2, and assume based on the old Marvell driver code that "Vendor Specific" = 221 would be used with WPA. Going through the entire IE list and finding a match seems safer than just blindly grabbing the first one. This would also be a good time to add some bounds checking to make sure not to overrun "ie" as well... The other two IEs that are being added by modern wpa_supplicant are "Extended Capabilities" (127) with SCS and mirrored SCS set: 7f 0b 00 00 00 00 00 00 40 00 00 00 20 ...and "Supported Operating Classes" (59) with current = 81 and supported = 81 and 82: 3b 03 51 51 52 I tried converting these additional IEs to TLVs. It resulted in a successful connection, but the firmware didn't pass on these two IEs in the association request -- I verified by sniffing packets. So I was concerned about passing them onto the firmware if it's not making use of them, in case it's interpreting them in some other unexpected way. Do you have any guidance on which possible IEs I should be looking for other than 48 and 221, or where I could find that out? BTW, modern wpa_supplicant also doesn't work with libertas for one additional reason: it violates NL80211_ATTR_MAX_SCAN_IE_LEN on some older drivers including this one. But I believe that's a wpa_supplicant problem that I can't really address in the kernel... http://lists.infradead.org/pipermail/hostap/2022-January/040185.html Thanks! Doug
