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

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Lorenzo Bianconi <lorenzo@kernel.org> writes:

>> 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 :)

Well, what I mean is that the purpose of WARN_ON is, as you say, to
catch if "something nasty occurred", so we can fix it. But if we already
know that something nasty does, indeed, occur, shouldn't we just fix the
cause instead of putting in a warn so that we'll get a spat the next
time it happens? :)

-Toke