Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <acdb4315780bde3481c44ba90fe341fea75a2929.camel@sipsolutions.net>
Subject: Re: mac80211 bugs
From:   Johannes Berg <johannes@sipsolutions.net>
To:     James Dutton <james.dutton@gmail.com>,
        linux-wireless@vger.kernel.org
Date:   Tue, 10 Oct 2023 16:22:26 +0200
In-Reply-To: <CAAMvbhHo8wo924YVZqFrLKQko59iSiLMhVAzb=tdSvf_-v1CCQ@mail.gmail.com>
References: <CAAMvbhGsA4A9qHrYdXcNXLzv7a7xZZq98f90Q-Btb-9vs9ayCw@mail.gmail.com>
         <CAAMvbhHo8wo924YVZqFrLKQko59iSiLMhVAzb=tdSvf_-v1CCQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-10-09 at 16:56 +0100, James Dutton wrote:
> /usr/src/linux/net/mac80211/sta_info.c:2424
>        case STA_STATS_RATE_TYPE_LEGACY: {
>                 struct ieee80211_supported_band *sband;
>                 u16 brate;
>                 unsigned int shift;
>                 int band =3D STA_STATS_GET(LEGACY_BAND, rate);
>                 int rate_idx =3D STA_STATS_GET(LEGACY_IDX, rate);
>=20
>                 sband =3D local->hw.wiphy->bands[band];
>=20
>                 if (WARN_ON_ONCE(!sband->bitrates))      <------It_crashe=
s_here
>                         break;
>=20
>                 brate =3D sband->bitrates[rate_idx].bitrate;
>                 if (rinfo->bw =3D=3D RATE_INFO_BW_5)
>                         shift =3D 2;
>                 else if (rinfo->bw =3D=3D RATE_INFO_BW_10)
>                         shift =3D 1;
>                 else
>                         shift =3D 0;
>                 rinfo->legacy =3D DIV_ROUND_UP(brate, 1 << shift);
>                 break;
>                 }
>=20
> Looking at this, it can be one of two things:
> 1) local->hw.wiphy->bands[band];    is NULL

Yes, I think that's it.

> 2) bands is an array of 6 items, making band valid for values 0-5.
> If band >=3D 6, it would cause problems.

Highly unlikely.

> So maybe something along these lines might help:
>=20
> Signed-off-by: James Courtier-Dutton <james.dutton@gmail.com>
> --- sta_info.c.org      2023-10-08 19:52:13.578270007 +0100
> +++ sta_info.c.new2     2023-10-08 19:52:09.450214070 +0100
> @@ -2420,7 +2420,26 @@
>                 int band =3D STA_STATS_GET(LEGACY_BAND, rate);
>                 int rate_idx =3D STA_STATS_GET(LEGACY_IDX, rate);
>=20
> +               if (band >=3D NUM_NL80211_BANDS) {
> +                       printk("ERROR: band=3D%d is too large.
> Returning\n", band);
> +                       break;
> +               }
> +
>                 sband =3D local->hw.wiphy->bands[band];
> +               if (!sband) {
> +                       printk("ERROR: sband NULL. Returning\n");
> +                       break;
> +               }


You'd really never want a plain printk, and anyway, that printk is
malformed (no severity string macro).


_Maybe_ change it to WARN_ON_ONCE(!sband || !sband->bitrates) there, but
really I think we should prevent this in the first place.


Is this, by any chance, a device without 2.4 GHz?

johannes