diff mbox series

wifi: mac80211: add input validation to sta_stats_decode_rate()

Message ID CAAMvbhGyheFdWSrDzM_i10n9s06n3G2wX6O_S68VUZyP-a9p+A@mail.gmail.com
State New
Headers show
Series wifi: mac80211: add input validation to sta_stats_decode_rate() | expand

Commit Message

James Dutton May 26, 2024, 11:43 p.m. UTC
Validation is required as a result of parameters derived from
received wifi packets.

Signed-off-by: James Courtier-Dutton <james.dutton@gmail.com>
---
 net/mac80211/sta_info.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

             shift = 2;

Comments

Johannes Berg May 27, 2024, 5:41 a.m. UTC | #1
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
James Dutton May 27, 2024, 5:14 p.m. UTC | #2
On Mon, 27 May 2024 at 06:41, Johannes Berg <johannes@sipsolutions.net> 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
diff mbox series

Patch

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)