Received-SPF: pass (google.com: domain of linux-wireless+bounces-4257-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <7e38b0496e8f2b8ef18d69e4e07db8fdc29f303b.camel@sipsolutions.net>
Subject: Re: [bug report] wifi: mac80211: clean up connection process
From: Johannes Berg <johannes@sipsolutions.net>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: linux-wireless@vger.kernel.org
Date: Thu, 29 Feb 2024 16:08:22 +0100
In-Reply-To: <3151f5d0-c18f-413d-b34b-b94f095b947c@moroto.mountain>
References: <3151f5d0-c18f-413d-b34b-b94f095b947c@moroto.mountain>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

Hi Dan,

> net/mac80211/mlme.c:8176 ieee80211_mgd_assoc() error: uninitialized symbo=
l 'match_auth'.
> net/mac80211/mlme.c:8220 ieee80211_mgd_assoc() error: uninitialized symbo=
l 'match_auth'.

Huh, that's interesting, how did we not notice this ... I'll fix it, but
I think I'll just make the assignment unconditional and add the part of
"ifmgd->auth_data &&" to the statement, rather than having the if.

> net/mac80211/mlme.c:8177 ieee80211_mgd_assoc() error: we previously assum=
ed 'link' could be null (see line 8169)

>     8168                         link =3D sdata_dereference(sdata->link[i=
], sdata);
>     8169                         if (link)
>                                      ^^^^
> link checked for NULL

Right.


> --> 8176                         if (match_auth && i =3D=3D assoc_link_id=
)
>                                      ^^^^^^^^^^
> Potentially uninitialized
>=20
>     8177                                 assoc_data->link[i].conn =3D lin=
k->u.mgd.conn;
>                                                                     ^^^^^=
^
> Unchecked dereference.  This one is probably a false positive, but I
> just thought I would report it for completeness.

Yeah, hm. It's a bit tricky. I was going to say the link must be there,
but actually, that's not even entirely guaranteed; we could have been
doing FT-OTA without previous auth, and then the link might only be
assigned later in this code, in

        if (req->ap_mld_addr) {
                /* if there was no authentication, set up the link */
                err =3D ieee80211_vif_set_links(sdata, BIT(assoc_link_id), =
0);


So I'll add a && link there, otherwise we anyway don't have data yet and
need to use the unlimited thing.

Thanks!

johannes