| Message ID | 20230116202126.50400-3-doug@schmorgal.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: libertas: IE handling fixes | expand |
On Mon, Jan 16, 2023 at 12:21:24PM -0800, Doug Brown wrote: > [You don't often get email from doug@schmorgal.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > 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 finding the first RSN or WPA IE and only adding that. This > has the extra benefit of working properly if the RSN/WPA IE isn't the > first one in the IE buffer. > > While we're at it, clean up the code to use the available structs like > the other lbs_add_* functions instead of directly manipulating the TLV > buffer. > > Signed-off-by: Doug Brown <doug@schmorgal.com> > --- > drivers/net/wireless/marvell/libertas/cfg.c | 28 +++++++++++++-------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c > index 3e065cbb0af9..3f35dc7a1d7d 100644 > --- a/drivers/net/wireless/marvell/libertas/cfg.c > +++ b/drivers/net/wireless/marvell/libertas/cfg.c ... > @@ -428,14 +438,12 @@ static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len) > * __le16 len > * u8[] data > */ > - *tlv++ = *ie++; > - *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; > + wpatlv->header.type = cpu_to_le16(wpaie->id); > + wpatlv->header.len = cpu_to_le16(wpaie->datalen); > + memcpy(wpatlv->data, wpaie->data, wpaie->datalen); Hi Doug, Thanks for fixing the endiness issues with cpu_to_le16() This part looks good to me now. Likewise for patch 4/4. One suggestion I have, which is probably taking things to far, is a helper for what seems to be repeated code-pattern. But I don't feel strongly about that. > + > + /* Return the total number of bytes added to the TLV buffer */ > + return sizeof(struct mrvl_ie_header) + wpaie->datalen; > } > > /* > -- > 2.34.1 >
On 1/17/2023 12:59 AM, Simon Horman wrote: > On Mon, Jan 16, 2023 at 12:21:24PM -0800, Doug Brown wrote: >> [You don't often get email from doug@schmorgal.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] >> >> 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 finding the first RSN or WPA IE and only adding that. This >> has the extra benefit of working properly if the RSN/WPA IE isn't the >> first one in the IE buffer. >> >> While we're at it, clean up the code to use the available structs like >> the other lbs_add_* functions instead of directly manipulating the TLV >> buffer. >> >> Signed-off-by: Doug Brown <doug@schmorgal.com> >> --- >> drivers/net/wireless/marvell/libertas/cfg.c | 28 +++++++++++++-------- >> 1 file changed, 18 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c >> index 3e065cbb0af9..3f35dc7a1d7d 100644 >> --- a/drivers/net/wireless/marvell/libertas/cfg.c >> +++ b/drivers/net/wireless/marvell/libertas/cfg.c > > ... > >> @@ -428,14 +438,12 @@ static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len) >> * __le16 len >> * u8[] data >> */ >> - *tlv++ = *ie++; >> - *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; >> + wpatlv->header.type = cpu_to_le16(wpaie->id); >> + wpatlv->header.len = cpu_to_le16(wpaie->datalen); >> + memcpy(wpatlv->data, wpaie->data, wpaie->datalen); > > Hi Doug, > > Thanks for fixing the endiness issues with cpu_to_le16() > This part looks good to me now. Likewise for patch 4/4. > > One suggestion I have, which is probably taking things to far, > is a helper for what seems to be repeated code-pattern. > But I don't feel strongly about that. Thanks Simon. Is this basically what you're suggesting for a helper? static int lbs_add_ie_tlv(u8 *tlvbuf, const struct element *ie, u16 tlvtype) { struct mrvl_ie_data *tlv = (struct mrvl_ie_data *)tlvbuf; tlv->header.type = cpu_to_le16(tlvtype); tlv->header.len = cpu_to_le16(ie->datalen); memcpy(tlv->data, ie->data, ie->datalen); return sizeof(struct mrvl_ie_header) + ie->datalen; } And then in the two functions where I'm doing that, at the bottom: return lbs_add_ie_tlv(tlv, wpaie, wpaie->id); return lbs_add_ie_tlv(tlv, wpsie, TLV_TYPE_WPS_ENROLLEE); I could definitely do that to avoid repeating the chunk of code that fills out the struct in the two functions. A lot of the other lbs_add_*_tlv functions follow a similar pattern of setting up a struct pointer and filling out the header, so I don't think it's too crazy to just repeat the code twice. On the other hand, the example above does look pretty darn clean. I don't feel strongly either way myself. > >> + >> + /* Return the total number of bytes added to the TLV buffer */ >> + return sizeof(struct mrvl_ie_header) + wpaie->datalen; >> } >> >> /* >> -- >> 2.34.1 >>
On Tue, Jan 17, 2023 at 10:35:56PM -0800, Doug Brown wrote: > On 1/17/2023 12:59 AM, Simon Horman wrote: > > On Mon, Jan 16, 2023 at 12:21:24PM -0800, Doug Brown wrote: > > > [You don't often get email from doug@schmorgal.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > > > > > 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 finding the first RSN or WPA IE and only adding that. This > > > has the extra benefit of working properly if the RSN/WPA IE isn't the > > > first one in the IE buffer. > > > > > > While we're at it, clean up the code to use the available structs like > > > the other lbs_add_* functions instead of directly manipulating the TLV > > > buffer. > > > > > > Signed-off-by: Doug Brown <doug@schmorgal.com> > > > --- > > > drivers/net/wireless/marvell/libertas/cfg.c | 28 +++++++++++++-------- > > > 1 file changed, 18 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c > > > index 3e065cbb0af9..3f35dc7a1d7d 100644 > > > --- a/drivers/net/wireless/marvell/libertas/cfg.c > > > +++ b/drivers/net/wireless/marvell/libertas/cfg.c > > > > ... > > > > > @@ -428,14 +438,12 @@ static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len) > > > * __le16 len > > > * u8[] data > > > */ > > > - *tlv++ = *ie++; > > > - *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; > > > + wpatlv->header.type = cpu_to_le16(wpaie->id); > > > + wpatlv->header.len = cpu_to_le16(wpaie->datalen); > > > + memcpy(wpatlv->data, wpaie->data, wpaie->datalen); > > > > Hi Doug, > > > > Thanks for fixing the endiness issues with cpu_to_le16() > > This part looks good to me now. Likewise for patch 4/4. > > > > One suggestion I have, which is probably taking things to far, > > is a helper for what seems to be repeated code-pattern. > > But I don't feel strongly about that. > > Thanks Simon. Is this basically what you're suggesting for a helper? > > static int lbs_add_ie_tlv(u8 *tlvbuf, const struct element *ie, u16 tlvtype) > { > struct mrvl_ie_data *tlv = (struct mrvl_ie_data *)tlvbuf; > tlv->header.type = cpu_to_le16(tlvtype); > tlv->header.len = cpu_to_le16(ie->datalen); > memcpy(tlv->data, ie->data, ie->datalen); > return sizeof(struct mrvl_ie_header) + ie->datalen; > } > > And then in the two functions where I'm doing that, at the bottom: > > return lbs_add_ie_tlv(tlv, wpaie, wpaie->id); > return lbs_add_ie_tlv(tlv, wpsie, TLV_TYPE_WPS_ENROLLEE); > > I could definitely do that to avoid repeating the chunk of code that > fills out the struct in the two functions. A lot of the other > lbs_add_*_tlv functions follow a similar pattern of setting up a struct > pointer and filling out the header, so I don't think it's too crazy to > just repeat the code twice. On the other hand, the example above does > look pretty darn clean. I don't feel strongly either way myself. Hi Doug, yes, I was thinking about something like that. And wondering if it might be reused elsewhere (in the same file). But again, I don't feel strongly about this. So perhaps it's something to consider in future.
diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c index 3e065cbb0af9..3f35dc7a1d7d 100644 --- a/drivers/net/wireless/marvell/libertas/cfg.c +++ b/drivers/net/wireless/marvell/libertas/cfg.c @@ -416,10 +416,20 @@ static int lbs_add_cf_param_tlv(u8 *tlv) static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len) { - size_t tlv_len; + struct mrvl_ie_data *wpatlv = (struct mrvl_ie_data *)tlv; + const struct element *wpaie; + + /* Find the first RSN or WPA IE to use */ + wpaie = cfg80211_find_elem(WLAN_EID_RSN, ie, ie_len); + if (!wpaie) + wpaie = cfg80211_find_vendor_elem(WLAN_OUI_MICROSOFT, + WLAN_OUI_TYPE_MICROSOFT_WPA, + ie, ie_len); + if (!wpaie || wpaie->datalen > 128) + return 0; /* - * We need just convert an IE to an TLV. IEs use u8 for the header, + * Convert the found IE to a TLV. IEs use u8 for the header, * u8 type * u8 len * u8[] data @@ -428,14 +438,12 @@ static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len) * __le16 len * u8[] data */ - *tlv++ = *ie++; - *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; + wpatlv->header.type = cpu_to_le16(wpaie->id); + wpatlv->header.len = cpu_to_le16(wpaie->datalen); + memcpy(wpatlv->data, wpaie->data, wpaie->datalen); + + /* Return the total number of bytes added to the TLV buffer */ + return sizeof(struct mrvl_ie_header) + wpaie->datalen; } /*
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 finding the first RSN or WPA IE and only adding that. This has the extra benefit of working properly if the RSN/WPA IE isn't the first one in the IE buffer. While we're at it, clean up the code to use the available structs like the other lbs_add_* functions instead of directly manipulating the TLV buffer. Signed-off-by: Doug Brown <doug@schmorgal.com> --- drivers/net/wireless/marvell/libertas/cfg.c | 28 +++++++++++++-------- 1 file changed, 18 insertions(+), 10 deletions(-)