[PATCH] wifi: libertas: return consistent length in lbs_add_wpa_tlv()

[PATCH] wifi: libertas: return consistent length in lbs_add_wpa_tlv()

[Doug Brown]

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@schmorgal.com>
---
 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;
 }
 
 /*
-- 
2.34.1

Re: [PATCH] wifi: libertas: return consistent length in lbs_add_wpa_tlv()

[Dan Williams]

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@schmorgal.com>
> ---
>  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.

Can you also update the comments to say something like "only copy the
first IE into the command buffer".

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?

Dan

>  }
>  
>  /*

Re: [PATCH] wifi: libertas: return consistent length in lbs_add_wpa_tlv()

[Doug Brown]

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@schmorgal.com>
>> ---
>>   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

Re: [PATCH] wifi: libertas: return consistent length in lbs_add_wpa_tlv()

[Dan Williams]

On Tue, 2023-01-03 at 17:13 -0800, Doug Brown wrote:
> 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@schmorgal.com>
> > > ---
> > >   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.

Yeah, that's a good point, though I'd hope cfg80211 had already
validated the IE structure that gets put into sme->ie. If not, I'd
expect bigger problems. But doesn't hurt.

> 
> > 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...

Everything after CMD_802_11_ASSOCIATE's DTIM Period field is just a
bunch of IEs; the command only accepts certain IEs (at least it was
documented to do that, no idea what the actual firmware does). So I
wouldn't be surprised if it ignores some.

So I guess ignore the reasoning I had above, but there's one more good
reason to filter IEs passed to the firmware: space. We're probably not
close to overrunning the buffer, but we really don't want to do that
for security reasons.

> 
> 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.

Yeah, it might.

> 
> 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?

Only those two. The rest that are required get added specifically in
the driver. There is a way to push unrecognized IEs through
("passthrough IEs" ID 0x010A) but we never implemented that in the
driver because we never needed it.

> 
> 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...

That's lame... but Jouni's response was that not allowing extra IEs
would break some WPS stuff; CMD_802_11_SCAN does allow adding a TLV
(0x011B) for WPS Enrollee IE contents, so maybe you could just set
max_scan_ie_len to something larger than zero and ignore IEs that are
not valid in WPS Enrollee Probe Request frames, while adding the WPS
TLVs?

Dan

> 
> http://lists.infradead.org/pipermail/hostap/2022-January/040185.html
> 
> Thanks!
> Doug
> 

Re: [PATCH] wifi: libertas: return consistent length in lbs_add_wpa_tlv()

[Doug Brown]

On 1/4/2023 6:47 AM, Dan Williams wrote:
> On Tue, 2023-01-03 at 17:13 -0800, Doug Brown wrote:
>> 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@schmorgal.com>
>>>> ---
>>>>    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.
> 
> Yeah, that's a good point, though I'd hope cfg80211 had already
> validated the IE structure that gets put into sme->ie. If not, I'd
> expect bigger problems. But doesn't hurt.

Ah, I didn't even consider that cfg80211 might be validating it ahead of
time, but that would make a lot of sense. I'll do some more digging and
figure out if that much validation is really needed in this driver.

>>
>>> 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...
> 
> Everything after CMD_802_11_ASSOCIATE's DTIM Period field is just a
> bunch of IEs; the command only accepts certain IEs (at least it was
> documented to do that, no idea what the actual firmware does). So I
> wouldn't be surprised if it ignores some.
> 
> So I guess ignore the reasoning I had above, but there's one more good
> reason to filter IEs passed to the firmware: space. We're probably not
> close to overrunning the buffer, but we really don't want to do that
> for security reasons.


I like that idea a lot. I'll filter and only allow through 48 and 221
IEs in lbs_add_wpa_tlv() in the next version of the patch.

>>
>> 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.
> 
> Yeah, it might.
> 
>>
>> 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?
> 
> Only those two. The rest that are required get added specifically in
> the driver. There is a way to push unrecognized IEs through
> ("passthrough IEs" ID 0x010A) but we never implemented that in the
> driver because we never needed it.


Thanks. Good to know that passthrough is possible if any additional IEs
need to be supported in the future. I see that in the old Marvell source
code now, thanks.

>>
>> 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...
> 
> That's lame... but Jouni's response was that not allowing extra IEs
> would break some WPS stuff; CMD_802_11_SCAN does allow adding a TLV
> (0x011B) for WPS Enrollee IE contents, so maybe you could just set
> max_scan_ie_len to something larger than zero and ignore IEs that are
> not valid in WPS Enrollee Probe Request frames, while adding the WPS
> TLVs?


I love this idea, and I'm definitely interested in attempting it in
another patch in order to fully restore this driver's compatibility with
wpa_supplicant. I'll play around with it a bit.

Do you happen to have any more info on how to use this TLV? It isn't
documented in the old Marvell driver or this driver. Based on what I'm
seeing in wpa_supplicant, it looks like the WPS stuff is encapsulated in
Vendor Specific (221) IEs with special OUI/type values for WPS. There's
another OUI/type for P2P info that can potentially be added for WPS too.
Would I just need to directly convert these IEs into 0x011B TLVs
(obviously with a new TLV_TYPE_ #define added for it)?

Thanks,
Doug

> 
> Dan
> 
>>
>> http://lists.infradead.org/pipermail/hostap/2022-January/040185.html
>>
>> Thanks!
>> Doug
>>
> 

Re: [PATCH] wifi: libertas: return consistent length in lbs_add_wpa_tlv()

[Dan Williams]

On Wed, 2023-01-04 at 22:43 -0800, Doug Brown wrote:
> On 1/4/2023 6:47 AM, Dan Williams wrote:
> > On Tue, 2023-01-03 at 17:13 -0800, Doug Brown wrote:
> > > 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@schmorgal.com>
> > > > > ---
> > > > >    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.
> > 
> > Yeah, that's a good point, though I'd hope cfg80211 had already
> > validated the IE structure that gets put into sme->ie. If not, I'd
> > expect bigger problems. But doesn't hurt.
> 
> Ah, I didn't even consider that cfg80211 might be validating it ahead
> of
> time, but that would make a lot of sense. I'll do some more digging
> and
> figure out if that much validation is really needed in this driver.
> 
> > > 
> > > > 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...
> > 
> > Everything after CMD_802_11_ASSOCIATE's DTIM Period field is just a
> > bunch of IEs; the command only accepts certain IEs (at least it was
> > documented to do that, no idea what the actual firmware does). So I
> > wouldn't be surprised if it ignores some.
> > 
> > So I guess ignore the reasoning I had above, but there's one more
> > good
> > reason to filter IEs passed to the firmware: space. We're probably
> > not
> > close to overrunning the buffer, but we really don't want to do
> > that
> > for security reasons.
> 
> 
> I like that idea a lot. I'll filter and only allow through 48 and 221
> IEs in lbs_add_wpa_tlv() in the next version of the patch.
> 
> > > 
> > > 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.
> > 
> > Yeah, it might.
> > 
> > > 
> > > 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?
> > 
> > Only those two. The rest that are required get added specifically
> > in
> > the driver. There is a way to push unrecognized IEs through
> > ("passthrough IEs" ID 0x010A) but we never implemented that in the
> > driver because we never needed it.
> 
> 
> Thanks. Good to know that passthrough is possible if any additional
> IEs
> need to be supported in the future. I see that in the old Marvell
> source
> code now, thanks.
> 
> > > 
> > > 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...
> > 
> > That's lame... but Jouni's response was that not allowing extra IEs
> > would break some WPS stuff; CMD_802_11_SCAN does allow adding a TLV
> > (0x011B) for WPS Enrollee IE contents, so maybe you could just set
> > max_scan_ie_len to something larger than zero and ignore IEs that
> > are
> > not valid in WPS Enrollee Probe Request frames, while adding the
> > WPS
> > TLVs?
> 
> 
> I love this idea, and I'm definitely interested in attempting it in
> another patch in order to fully restore this driver's compatibility
> with
> wpa_supplicant. I'll play around with it a bit.
> 
> Do you happen to have any more info on how to use this TLV? It isn't
> documented in the old Marvell driver or this driver. Based on what
> I'm
> seeing in wpa_supplicant, it looks like the WPS stuff is encapsulated
> in
> Vendor Specific (221) IEs with special OUI/type values for WPS.
> There's
> another OUI/type for P2P info that can potentially be added for WPS
> too.
> Would I just need to directly convert these IEs into 0x011B TLVs
> (obviously with a new TLV_TYPE_ #define added for it)?

Kind of. Type = 0x11B, Length = size of the IEs buffer, and the data is
just all the IEs you want to shove into the Enrollee Probe Request
frames. So you don't need to convert anything, you're just wrapping the
IEs you get into a Marvell-specific TLV.

Dan

> 
> Thanks,
> Doug
> 
> > 
> > Dan
> > 
> > > 
> > > http://lists.infradead.org/pipermail/hostap/2022-January/040185.html
> > > 
> > > Thanks!
> > > Doug
> > > 
> > 
>