2024-05-26 23:44:32

by James Dutton

[permalink] [raw]
Subject: [PATCH] wifi: mac80211: add input validation to sta_stats_decode_rate()

Validation is required as a result of parameters derived from
received wifi packets.

Signed-off-by: James Courtier-Dutton <[email protected]>
---
net/mac80211/sta_info.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index da5fdd6f5c85..bab05c6b1bcc 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -2437,11 +2437,26 @@ static void sta_stats_decode_rate(struct
ieee80211_local *local, u32 rate,
int band = STA_STATS_GET(LEGACY_BAND, rate);
int rate_idx = STA_STATS_GET(LEGACY_IDX, rate);

+ if (WARN_ON_ONCE(!local))
+ break;
+
+ if (WARN_ON_ONCE(!rinfo))
+ break;
+
+ if (WARN_ON_ONCE(band >= NUM_NL80211_BANDS))
+ break;
+
sband = local->hw.wiphy->bands[band];

+ if (WARN_ON_ONCE(!sband))
+ break;
+
if (WARN_ON_ONCE(!sband->bitrates))
break;

+ if (WARN_ON_ONCE(rate_idx >= sband->n_bitrates))
+ break;
+
brate = sband->bitrates[rate_idx].bitrate;
if (rinfo->bw == RATE_INFO_BW_5)
shift = 2;
--
2.43.0


2024-05-27 05:43:13

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] wifi: mac80211: add input validation to sta_stats_decode_rate()

On Mon, 2024-05-27 at 00:43 +0100, James Dutton wrote:
> Validation is required as a result of parameters derived from
> received wifi packets.

I don't think I fully agree with that. First of all, this data is never
actually directly derived from the wifi packet (certainly not any
pointers or the band enum!), even the PLCP contains different encodings.
Thus there's always already a translation in driver or firmware.

Now of course we shouldn't trust firmware either, but even then there
are a lot of places, I'd think this is better done at the driver level.

johannes

2024-05-27 17:15:38

by James Dutton

[permalink] [raw]
Subject: Re: [PATCH] wifi: mac80211: add input validation to sta_stats_decode_rate()

On Mon, 27 May 2024 at 06:41, Johannes Berg <[email protected]> wrote:
>
> On Mon, 2024-05-27 at 00:43 +0100, James Dutton wrote:
> > Validation is required as a result of parameters derived from
> > received wifi packets.
>
> I don't think I fully agree with that. First of all, this data is never
> actually directly derived from the wifi packet (certainly not any
> pointers or the band enum!), even the PLCP contains different encodings.
> Thus there's always already a translation in driver or firmware.
>
> Now of course we shouldn't trust firmware either, but even then there
> are a lot of places, I'd think this is better done at the driver level.
>
Hi johannes,

You mention "certainly not any pointers or the band enum!".
How certain are you about that statement? I ask because received wifi
packets can and do result in unwelcome values for the pointers here.
I did not say "directly derived".

Kind Regards

James

2024-05-27 17:44:12

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] wifi: mac80211: add input validation to sta_stats_decode_rate()

On Mon, 2024-05-27 at 18:14 +0100, James Dutton wrote:
>
> You mention "certainly not any pointers or the band enum!".
> How certain are you about that statement? I ask because received wifi
> packets can and do result in unwelcome values for the pointers here.
>

I really don't see how? Certainly the pointer is _always_ going to be
defined by the driver calling this, and I've yet to see any hardware
that actually uses nl80211 band enums directly.

johannes