| Message ID | 20240517145420.8891-1-concord@gentoo.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v2] wifi: mac80211: Avoid address calculations via out of bounds array indexing | expand |
On Fri, May 17, 2024 at 10:54:20AM -0400, Kenton Groombridge wrote: > 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 u> [ 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> Thanks Kenton, 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. But in any case, I think this is an improvement and seems correct to me. Reviewed-by: Simon Horman <horms@kernel.org>
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 :)
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(-)