| Message ID | CAAMvbhGyheFdWSrDzM_i10n9s06n3G2wX6O_S68VUZyP-a9p+A@mail.gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: add input validation to sta_stats_decode_rate() | expand |
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
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
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
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)
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;