Re: [PATCH wireless] wifi: mac8021: fix possible oob access in ieee80211_get_rate_duration

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

> Lorenzo Bianconi <lorenzo@kernel.org> writes:
> 
> > Fix possible out-of-bound access in ieee80211_get_rate_duration routine
> > as reported by the following UBSAN report:
> >
> > UBSAN: array-index-out-of-bounds in net/mac80211/airtime.c:455:47
> > index 15 is out of range for type 'u16 [12]'
> > CPU: 2 PID: 217 Comm: kworker/u32:10 Not tainted 6.1.0-060100rc3-generic
> > Hardware name: Acer Aspire TC-281/Aspire TC-281, BIOS R01-A2 07/18/2017
> > Workqueue: mt76 mt76u_tx_status_data [mt76_usb]
> > Call Trace:
> >  <TASK>
> >  show_stack+0x4e/0x61
> >  dump_stack_lvl+0x4a/0x6f
> >  dump_stack+0x10/0x18
> >  ubsan_epilogue+0x9/0x43
> >  __ubsan_handle_out_of_bounds.cold+0x42/0x47
> > ieee80211_get_rate_duration.constprop.0+0x22f/0x2a0 [mac80211]
> >  ? ieee80211_tx_status_ext+0x32e/0x640 [mac80211]
> >  ieee80211_calc_rx_airtime+0xda/0x120 [mac80211]
> >  ieee80211_calc_tx_airtime+0xb4/0x100 [mac80211]
> >  mt76x02_send_tx_status+0x266/0x480 [mt76x02_lib]
> >  mt76x02_tx_status_data+0x52/0x80 [mt76x02_lib]
> >  mt76u_tx_status_data+0x67/0xd0 [mt76_usb]
> >  process_one_work+0x225/0x400
> >  worker_thread+0x50/0x3e0
> >  ? process_one_work+0x400/0x400
> >  kthread+0xe9/0x110
> >  ? kthread_complete_and_exit+0x20/0x20
> >  ret_from_fork+0x22/0x30
> >
> > Reported-by: bjlockie@lockie.ca
> > Fixes: db3e1c40cf2f ("mac80211: Import airtime calculation code from mt76")
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  net/mac80211/airtime.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/net/mac80211/airtime.c b/net/mac80211/airtime.c
> > index 2e66598fac79..4ed05988131d 100644
> > --- a/net/mac80211/airtime.c
> > +++ b/net/mac80211/airtime.c
> > @@ -452,6 +452,9 @@ static u32 ieee80211_get_rate_duration(struct ieee80211_hw *hw,
> >  			 (status->encoding == RX_ENC_HE && streams > 8)))
> >  		return 0;
> >  
> > +	if (WARN_ON_ONCE(idx >= MCS_GROUP_RATES))
> > +		return 0;
> > +
> 
> So presumably this is something that can actually happen in real usage,
> so should we really warn? Or was the driver also fixed to not trigger
> this?

looking at the mt76x02 support, MT_RATE_INDEX_VHT_IDX is GENMASK(3, 0) so the
hw can report rate_idx up to 15. Do you prefer to drop WARN_ON_ONCE()? I would
prefer to keep it since it informs us something nasty occurred (and at the end
it just runs ones), but I can live even w/o it :)

Regards,
Lorenzo

> 
> -Toke

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]