Message-ID: <133a4655-bafa-a4f1-b9f4-df43cf443e83@schmorgal.com>
Date:   Wed, 4 Jan 2023 22:43:16 -0800
From:   Doug Brown <doug@...morgal.com>
To:     Dan Williams <dcbw@...hat.com>, Kalle Valo <kvalo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     libertas-dev@...ts.infradead.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH] wifi: libertas: return consistent length in
 lbs_add_wpa_tlv()

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

Powered by blists - more mailing lists