| Message ID | 20240517145420.8891-1-concord@gentoo.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] wifi: mac80211: Avoid address calculations via out of bounds array indexing | expand |
On Fri, 2024-05-17 at 21:45 +0100, Simon Horman wrote: > > FWWIW, it seems unfortunate to me that the __counted_by field (n_channels) > is set some distance away from the allocation of the flex-array (channels) > whose bounds it checks. It seems it would be pretty easy for a bug in the > code being updated here to result in an overrun. > In a way, this is a more general problem, this allocates the max we know we might need, but then filter it down. It'd have to iterate twice to actually allocate the "correct" size, but then you could still have bugs by having different filter conditions in the two loops ... Don't see any good solutions to this kind of code? johannes
On Fri, 2024-05-17 at 10:54 -0400, Kenton Groombridge wrote: > req->n_channels must be set before req->channels[] can be used. > I don't know why, but this patch breaks a number of hwsim test cases. https://w1.fi/cgit/hostap/tree/tests/hwsim/ johannes
On Thu, May 23, 2024 at 11:35:37AM +0200, Johannes Berg wrote: > On Fri, 2024-05-17 at 21:45 +0100, Simon Horman wrote: > > > > FWWIW, it seems unfortunate to me that the __counted_by field (n_channels) > > is set some distance away from the allocation of the flex-array (channels) > > whose bounds it checks. It seems it would be pretty easy for a bug in the > > code being updated here to result in an overrun. > > > > In a way, this is a more general problem, this allocates the max we know > we might need, but then filter it down. It'd have to iterate twice to > actually allocate the "correct" size, but then you could still have bugs > by having different filter conditions in the two loops ... Yes, I agree this problem is more general than this patch or the code it updates. > Don't see any good solutions to this kind of code? I was hoping you might :)
On 24/05/29 04:54PM, Johannes Berg wrote: > On Fri, 2024-05-17 at 10:54 -0400, Kenton Groombridge wrote: > > req->n_channels must be set before req->channels[] can be used. > > > > I don't know why, but this patch breaks a number of hwsim test cases. > > https://w1.fi/cgit/hostap/tree/tests/hwsim/ > > johannes Pardon my absence. I'm also not sure why these tests are failing. Unless I'm missing something, the runtime behavior of these code paths shouldn't have changed significantly. FWIW, I have been running this patch on real hardware and connecting to Wi-Fi networks with it for the past couple weeks without problems. Though, I see that these tests are probably testing Wi-Fi modes that I am not using.
On Tue, 2024-06-04 at 14:53 -0400, Kenton Groombridge wrote: > On 24/05/29 04:54PM, Johannes Berg wrote: > > On Fri, 2024-05-17 at 10:54 -0400, Kenton Groombridge wrote: > > > req->n_channels must be set before req->channels[] can be used. > > > > > > > I don't know why, but this patch breaks a number of hwsim test cases. > > > > https://w1.fi/cgit/hostap/tree/tests/hwsim/ > > > > johannes > > Pardon my absence. > > I'm also not sure why these tests are failing. Unless I'm missing > something, the runtime behavior of these code paths shouldn't have > changed significantly. > Looking at your patch again, this seems wrong? > + local->hw_scan_req->req.channels[*n_chans++] = > req->channels[i]; > This will increment n_chans rather than *n_chans, no? johannes
On 24/06/04 09:29PM, Johannes Berg wrote: > Looking at your patch again, this seems wrong? > > > + local->hw_scan_req->req.channels[*n_chans++] = > > req->channels[i]; > > > > This will increment n_chans rather than *n_chans, no? > Ah ha! A silly mistake that I missed. V3 to follow soon.
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c index 73850312580f..b88e99c211ff 100644 --- a/net/mac80211/scan.c +++ b/net/mac80211/scan.c @@ -358,7 +358,8 @@ static bool ieee80211_prep_hw_scan(struct ieee80211_sub_if_data *sdata) struct cfg80211_scan_request *req; struct cfg80211_chan_def chandef; u8 bands_used = 0; - int i, ielen, n_chans; + int i, ielen; + u32 *n_chans; u32 flags = 0; req = rcu_dereference_protected(local->scan_req, @@ -368,34 +369,34 @@ static bool ieee80211_prep_hw_scan(struct ieee80211_sub_if_data *sdata) return false; if (ieee80211_hw_check(&local->hw, SINGLE_SCAN_ON_ALL_BANDS)) { + local->hw_scan_req->req.n_channels = req->n_channels; + for (i = 0; i < req->n_channels; i++) { local->hw_scan_req->req.channels[i] = req->channels[i]; bands_used |= BIT(req->channels[i]->band); } - - n_chans = req->n_channels; } else { do { if (local->hw_scan_band == NUM_NL80211_BANDS) return false; - n_chans = 0; + n_chans = &local->hw_scan_req->req.n_channels; + *n_chans = 0; for (i = 0; i < req->n_channels; i++) { if (req->channels[i]->band != local->hw_scan_band) continue; - local->hw_scan_req->req.channels[n_chans] = + local->hw_scan_req->req.channels[*n_chans++] = req->channels[i]; - n_chans++; + bands_used |= BIT(req->channels[i]->band); } local->hw_scan_band++; - } while (!n_chans); + } while (!*n_chans); } - local->hw_scan_req->req.n_channels = n_chans; ieee80211_prepare_scan_chandef(&chandef); if (req->flags & NL80211_SCAN_FLAG_MIN_PREQ_CONTENT)
req->n_channels must be set before req->channels[] can be used. This patch fixes one of the issues encountered in [1]. [ 83.964252] ------------[ cut here ]------------ [ 83.964255] UBSAN: array-index-out-of-bounds in net/mac80211/scan.c:364:4 [ 83.964258] index 0 is out of range for type 'struct ieee80211_channel *[]' [ 83.964260] CPU: 0 PID: 1695 Comm: iwd Tainted: G O T 6.8.9-gentoo-hardened1 #1 [ 83.964262] Hardware name: System76 Pangolin/Pangolin, BIOS ARB928_V00.01_T0025ASY1_ms 04/20/2023 [ 83.964264] Call Trace: [ 83.964267] <TASK> [ 83.964269] dump_stack_lvl+0x3f/0xc0 [ 83.964274] __ubsan_handle_out_of_bounds+0xec/0x110 [ 83.964278] ieee80211_prep_hw_scan+0x2db/0x4b0 [ 83.964281] __ieee80211_start_scan+0x601/0x990 [ 83.964284] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964287] ? cfg80211_scan+0x149/0x250 [ 83.964291] nl80211_trigger_scan+0x874/0x980 [ 83.964295] genl_family_rcv_msg_doit+0xe8/0x160 [ 83.964298] genl_rcv_msg+0x240/0x270 [ 83.964301] ? __cfi_nl80211_trigger_scan+0x10/0x10 [ 83.964302] ? __cfi_nl80211_post_doit+0x10/0x10 [ 83.964304] ? __cfi_nl80211_pre_doit+0x10/0x10 [ 83.964307] ? __cfi_genl_rcv_msg+0x10/0x10 [ 83.964309] netlink_rcv_skb+0x102/0x130 [ 83.964312] genl_rcv+0x23/0x40 [ 83.964314] netlink_unicast+0x23b/0x340 [ 83.964316] netlink_sendmsg+0x3a9/0x450 [ 83.964319] __sys_sendto+0x3ae/0x3c0 [ 83.964324] __x64_sys_sendto+0x21/0x40 [ 83.964326] do_syscall_64+0x90/0x150 [ 83.964329] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964331] ? syscall_exit_work+0xc2/0xf0 [ 83.964333] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964335] ? syscall_exit_to_user_mode+0x74/0xa0 [ 83.964337] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964339] ? do_syscall_64+0x9c/0x150 [ 83.964340] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964342] ? syscall_exit_to_user_mode+0x74/0xa0 [ 83.964344] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964346] ? do_syscall_64+0x9c/0x150 [ 83.964347] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964349] ? do_syscall_64+0x9c/0x150 [ 83.964351] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964353] ? syscall_exit_work+0xc2/0xf0 [ 83.964354] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964356] ? syscall_exit_to_user_mode+0x74/0xa0 [ 83.964358] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964359] ? do_syscall_64+0x9c/0x150 [ 83.964361] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964362] ? do_user_addr_fault+0x488/0x620 [ 83.964366] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964367] ? srso_alias_return_thunk+0x5/0xfbef5 [ 83.964369] entry_SYSCALL_64_after_hwframe+0x55/0x5d [ 83.964372] RIP: 0033:0x6200808578d7 [ 83.964374] Code: 00 00 90 f3 0f 1e fa 41 56 55 41 89 ce 48 83 ec 28 80 3d 7b f7 0d 00 00 74 29 45 31 c9 45 31 c0 41 89 ca b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 71 48 83 c4 28 5d 41 5e c3 66 0f 1f 84 00 00 [ 83.964375] RSP: 002b:0000730c4e821530 EFLAGS: 00000246 ORIG_RAX: 000000000000002c [ 83.964378] RAX: ffffffffffffffda RBX: 000006dbc456c570 RCX: 00006200808578d7 [ 83.964379] RDX: 000000000000005c RSI: 000006dbc45884f0 RDI: 0000000000000004 [ 83.964381] RBP: 0000000000000004 R08: 0000000000000000 R09: 0000000000000000 [ 83.964382] R10: 0000000000000000 R11: 0000000000000246 R12: 000006dbc456c480 [ 83.964383] R13: 000006dbc456c450 R14: 0000000000000000 R15: 000006dbc456c610 [ 83.964386] </TASK> [ 83.964386] ---[ end trace ]--- [1] https://bugzilla.kernel.org/show_bug.cgi?id=218810 v1->v2: - Drop changes in cfg80211 as requested by Johannes Co-authored-by: Kees Cook <keescook@chromium.org> Signed-off-by: Kenton Groombridge <concord@gentoo.org> --- net/mac80211/scan.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)