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: <657adc8e514d4486853ef90cdf97bd75f55b44fa.camel@redhat.com>
Subject: Re: [PATCH] wifi: libertas: return consistent length in
 lbs_add_wpa_tlv()
From:   Dan Williams <dcbw@redhat.com>
To:     Doug Brown <doug@schmorgal.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
Date:   Tue, 03 Jan 2023 11:47:09 -0600
In-Reply-To: <20230102234714.169831-1-doug@schmorgal.com>
References: <20230102234714.169831-1-doug@schmorgal.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.2 (3.46.2-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

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.
>=20
> 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.
>=20
> Fixes: e86dc1ca4676 ("Libertas: cfg80211 support")
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
> =C2=A0drivers/net/wireless/marvell/libertas/cfg.c | 7 +++----
> =C2=A01 file changed, 3 insertions(+), 4 deletions(-)
>=20
> 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)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*tlv++ =3D 0;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0tlv_len =3D *tlv++ =3D *i=
e++;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*tlv++ =3D 0;
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0while (tlv_len--)
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0*tlv++ =3D *ie++;
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* the TLV is two bytes larger=
 than the IE */
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return ie_len + 2;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0memcpy(tlv, ie, tlv_len);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* the TLV has a four-byte hea=
der */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 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

> =C2=A0}
> =C2=A0
> =C2=A0/*