Date:   Wed, 04 Jan 2023 08:47:19 -0600
From:   Dan Williams <dcbw@...hat.com>
To:     Doug Brown <doug@...morgal.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 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.

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

Powered by blists - more mailing lists