* [PATCH wireless] wifi: mac8021: fix possible oob access in ieee80211_get_rate_duration
@ 2022-11-06 10:30 Lorenzo Bianconi
2022-11-06 12:38 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Bianconi @ 2022-11-06 10:30 UTC (permalink / raw)
To: linux-wireless; +Cc: bjlockie, toke, johannes, nbd
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH wireless] wifi: mac8021: fix possible oob access in ieee80211_get_rate_duration 2022-11-06 10:30 [PATCH wireless] wifi: mac8021: fix possible oob access in ieee80211_get_rate_duration Lorenzo Bianconi @ 2022-11-06 12:38 ` Toke Høiland-Jørgensen 2022-11-06 13:32 ` Lorenzo Bianconi 0 siblings, 1 reply; 6+ messages in thread From: Toke Høiland-Jørgensen @ 2022-11-06 12:38 UTC (permalink / raw) To: Lorenzo Bianconi, linux-wireless; +Cc: bjlockie, johannes, nbd 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH wireless] wifi: mac8021: fix possible oob access in ieee80211_get_rate_duration 2022-11-06 12:38 ` Toke Høiland-Jørgensen @ 2022-11-06 13:32 ` Lorenzo Bianconi 2022-11-06 13:43 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 6+ messages in thread From: Lorenzo Bianconi @ 2022-11-06 13:32 UTC (permalink / raw) To: Toke Høiland-Jørgensen; +Cc: linux-wireless, bjlockie, johannes, nbd [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH wireless] wifi: mac8021: fix possible oob access in ieee80211_get_rate_duration 2022-11-06 13:32 ` Lorenzo Bianconi @ 2022-11-06 13:43 ` Toke Høiland-Jørgensen 2022-11-06 14:11 ` Lorenzo Bianconi 0 siblings, 1 reply; 6+ messages in thread From: Toke Høiland-Jørgensen @ 2022-11-06 13:43 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: linux-wireless, bjlockie, johannes, nbd 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH wireless] wifi: mac8021: fix possible oob access in ieee80211_get_rate_duration 2022-11-06 13:43 ` Toke Høiland-Jørgensen @ 2022-11-06 14:11 ` Lorenzo Bianconi 2022-11-06 22:56 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 6+ messages in thread From: Lorenzo Bianconi @ 2022-11-06 14:11 UTC (permalink / raw) To: Toke Høiland-Jørgensen; +Cc: linux-wireless, bjlockie, johannes, nbd [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH wireless] wifi: mac8021: fix possible oob access in ieee80211_get_rate_duration 2022-11-06 14:11 ` Lorenzo Bianconi @ 2022-11-06 22:56 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 6+ messages in thread From: Toke Høiland-Jørgensen @ 2022-11-06 22:56 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: linux-wireless, bjlockie, johannes, nbd 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-06 22:56 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-06 10:30 [PATCH wireless] wifi: mac8021: fix possible oob access in ieee80211_get_rate_duration Lorenzo Bianconi 2022-11-06 12:38 ` Toke Høiland-Jørgensen 2022-11-06 13:32 ` Lorenzo Bianconi 2022-11-06 13:43 ` Toke Høiland-Jørgensen 2022-11-06 14:11 ` Lorenzo Bianconi 2022-11-06 22:56 ` Toke Høiland-Jørgensen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).