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

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

[Lorenzo Bianconi]

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;
+
 	duration = airtime_mcs_groups[group].duration[idx];
 	duration <<= airtime_mcs_groups[group].shift;
 	*overhead = 36 + (streams << 2);
-- 
2.38.1

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

[Toke Høiland-Jørgensen]

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?

-Toke

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

[Lorenzo Bianconi]

[-- 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 --]

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

[Toke Høiland-Jørgensen]

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

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

[Lorenzo Bianconi]

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

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

I think in this case the hw just reports a wrong value, so we can limit the
value there too, anyway I would not assume each driver limits the rate_idx
value (as we already do for stream :))

Regards,
Lorenzo

> 
> -Toke

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

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

[Toke Høiland-Jørgensen]

Lorenzo Bianconi <lorenzo@kernel.org> writes:

>> 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? :)
>
> I think in this case the hw just reports a wrong value, so we can limit the
> value there too, anyway I would not assume each driver limits the rate_idx
> value (as we already do for stream :))

Right, I'm not disputing we should add the check itself :)

I guess we could phrase it like this: Do we expect drivers to be fixed
to clamp the index to < MCS_GROUP_RATES? If so, keep the warn; if we
don't expect drivers to be fixed to handle this, drop the warn and just
handle it gracefully...

-Toke