Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <cc785f92-587c-c260-6369-c2dde9a392ca@schmorgal.com>
Date:   Tue, 3 Jan 2023 17:13:12 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Content-Language: en-US
To:     Dan Williams <dcbw@redhat.com>, Kalle Valo <kvalo@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
Cc:     libertas-dev@lists.infradead.org, linux-wireless@vger.kernel.org,
        netdev@vger.kernel.org
References: <20230102234714.169831-1-doug@schmorgal.com>
 <657adc8e514d4486853ef90cdf97bd75f55b44fa.camel@redhat.com>
From:   Doug Brown <doug@schmorgal.com>
Subject: Re: [PATCH] wifi: libertas: return consistent length in
 lbs_add_wpa_tlv()
In-Reply-To: <657adc8e514d4486853ef90cdf97bd75f55b44fa.camel@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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