| Message ID | tencent_0CCA1979CFA30DC8A5CF8DDC92365DCE5D07@qq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: sband's null check should precede params | expand |
On Wed, 2023-11-29 at 13:48 +0800, Edward Adam Davis wrote: > > [Analysis] > When ieee80211_get_link_sband() fails to find a valid sband and first checks > for params in sta_link_apply_parameters(), it will return 0 due to new_link > being 0, which will lead to an incorrect process after sta_apply_parameters(). > > [Fix] > First obtain sband and perform a non null check before checking the params. Not sure I can even disagree with that analysis, it seems right, but ... > + if (!link || !link_sta) > + return -EINVAL; > + > + sband = ieee80211_get_link_sband(link); > + if (!sband) > + return -EINVAL; > + > /* > * If there are no changes, then accept a link that doesn't exist, > * unless it's a new link. There's a comment here which is clearly not true after this change, since you've already returned for !link_sta? johannes
On Wed, 29 Nov 2023 07:57:07 +0100, Johannes Berg wrote: > > [Analysis] > > When ieee80211_get_link_sband() fails to find a valid sband and first checks > > for params in sta_link_apply_parameters(), it will return 0 due to new_link > > being 0, which will lead to an incorrect process after sta_apply_parameters(). > > > > [Fix] > > First obtain sband and perform a non null check before checking the params. > > Not sure I can even disagree with that analysis, it seems right, but ... > > > + if (!link || !link_sta) > > + return -EINVAL; > > + > > + sband = ieee80211_get_link_sband(link); > > + if (!sband) > > + return -EINVAL; > > + > > /* > > * If there are no changes, then accept a link that doesn't exist, > > * unless it's a new link. > > There's a comment here which is clearly not true after this change, > since you've already returned for !link_sta? No, after applying my patch, it will return due to !sband. Edward
On Wed, 2023-11-29 at 16:18 +0800, Edward Adam Davis wrote: > On Wed, 29 Nov 2023 07:57:07 +0100, Johannes Berg wrote: > > > [Analysis] > > > When ieee80211_get_link_sband() fails to find a valid sband and first checks > > > for params in sta_link_apply_parameters(), it will return 0 due to new_link > > > being 0, which will lead to an incorrect process after sta_apply_parameters(). > > > > > > [Fix] > > > First obtain sband and perform a non null check before checking the params. > > > > Not sure I can even disagree with that analysis, it seems right, but ... > > > > > + if (!link || !link_sta) > > > + return -EINVAL; > > > + > > > + sband = ieee80211_get_link_sband(link); > > > + if (!sband) > > > + return -EINVAL; > > > + > > > /* > > > * If there are no changes, then accept a link that doesn't exist, > > > * unless it's a new link. > > > > There's a comment here which is clearly not true after this change, > > since you've already returned for !link_sta? > No, after applying my patch, it will return due to !sband. > Right, OK, but the way I read the comment (now) is that it wanted to accept it in that case? That said, I just threw the patch into our internal testing machinery quickly (probably has more MLO tests than upstream hostap for now), and it worked just fine ... Maybe we should just remove the comment? johannes
On Wed, 29 Nov 2023 09:33:23 +0100, Johannes Berg wrote: > > > > [Analysis] > > > > When ieee80211_get_link_sband() fails to find a valid sband and first checks > > > > for params in sta_link_apply_parameters(), it will return 0 due to new_link > > > > being 0, which will lead to an incorrect process after sta_apply_parameters(). > > > > > > > > [Fix] > > > > First obtain sband and perform a non null check before checking the params. > > > > > > Not sure I can even disagree with that analysis, it seems right, but ... > > > > > > > + if (!link || !link_sta) > > > > + return -EINVAL; > > > > + > > > > + sband = ieee80211_get_link_sband(link); > > > > + if (!sband) > > > > + return -EINVAL; > > > > + > > > > /* > > > > * If there are no changes, then accept a link that doesn't exist, > > > > * unless it's a new link. > > > > > > There's a comment here which is clearly not true after this change, > > > since you've already returned for !link_sta? > > No, after applying my patch, it will return due to !sband. > > > > Right, OK, but the way I read the comment (now) is that it wanted to > accept it in that case? > > That said, I just threw the patch into our internal testing machinery > quickly (probably has more MLO tests than upstream hostap for now), and > it worked just fine ... > > Maybe we should just remove the comment? Do you mean to delete the comments below? 3 /* 2 * If there are no changes, then accept a link that doesn't exist, 1 * unless it's a new link. 1800 */ Edward
On Wed, 2023-11-29 at 16:48 +0800, Edward Adam Davis wrote: > On Wed, 29 Nov 2023 09:33:23 +0100, Johannes Berg wrote: > > > > > [Analysis] > > > > > When ieee80211_get_link_sband() fails to find a valid sband and first checks > > > > > for params in sta_link_apply_parameters(), it will return 0 due to new_link > > > > > being 0, which will lead to an incorrect process after sta_apply_parameters(). > > > > > > > > > > [Fix] > > > > > First obtain sband and perform a non null check before checking the params. > > > > > > > > Not sure I can even disagree with that analysis, it seems right, but ... > > > > > > > > > + if (!link || !link_sta) > > > > > + return -EINVAL; > > > > > + > > > > > + sband = ieee80211_get_link_sband(link); > > > > > + if (!sband) > > > > > + return -EINVAL; > > > > > + > > > > > /* > > > > > * If there are no changes, then accept a link that doesn't exist, > > > > > * unless it's a new link. > > > > > > > > There's a comment here which is clearly not true after this change, > > > > since you've already returned for !link_sta? > > > No, after applying my patch, it will return due to !sband. > > > > > > > Right, OK, but the way I read the comment (now) is that it wanted to > > accept it in that case? > > > > That said, I just threw the patch into our internal testing machinery > > quickly (probably has more MLO tests than upstream hostap for now), and > > it worked just fine ... > > > > Maybe we should just remove the comment? > Do you mean to delete the comments below? > 3 /* > 2 * If there are no changes, then accept a link that doesn't exist, > 1 * unless it's a new link. > 1800 */ > Right, it doesn't seem correct any more? johannes
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 606b1b2e4123..8012dcdbcb5f 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -1787,6 +1787,13 @@ static int sta_link_apply_parameters(struct ieee80211_local *local, rcu_dereference_protected(sta->link[link_id], lockdep_is_held(&local->hw.wiphy->mtx)); + if (!link || !link_sta) + return -EINVAL; + + sband = ieee80211_get_link_sband(link); + if (!sband) + return -EINVAL; + /* * If there are no changes, then accept a link that doesn't exist, * unless it's a new link. @@ -1799,13 +1806,6 @@ static int sta_link_apply_parameters(struct ieee80211_local *local, !params->opmode_notif_used) return 0; - if (!link || !link_sta) - return -EINVAL; - - sband = ieee80211_get_link_sband(link); - if (!sband) - return -EINVAL; - if (params->link_mac) { if (new_link) { memcpy(link_sta->addr, params->link_mac, ETH_ALEN);
[Syz report] WARNING: CPU: 1 PID: 5067 at net/mac80211/rate.c:48 rate_control_rate_init+0x540/0x690 net/mac80211/rate.c:48 Modules linked in: CPU: 1 PID: 5067 Comm: syz-executor413 Not tainted 6.7.0-rc3-syzkaller-00014-gdf60cee26a2e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023 RIP: 0010:rate_control_rate_init+0x540/0x690 net/mac80211/rate.c:48 Code: 48 c7 c2 00 46 0c 8c be 08 03 00 00 48 c7 c7 c0 45 0c 8c c6 05 70 79 0b 05 01 e8 1b a0 6f f7 e9 e0 fd ff ff e8 61 b3 8f f7 90 <0f> 0b 90 e9 36 ff ff ff e8 53 b3 8f f7 e8 5e 0b 78 f7 31 ff 89 c3 RSP: 0018:ffffc90003c57248 EFLAGS: 00010293 RAX: 0000000000000000 RBX: ffff888016bc4000 RCX: ffffffff89f7d519 RDX: ffff888076d43b80 RSI: ffffffff89f7d6df RDI: 0000000000000005 RBP: ffff88801daaae20 R08: 0000000000000005 R09: 0000000000000000 R10: 0000000000000001 R11: 0000000000000002 R12: 0000000000000001 R13: 0000000000000000 R14: ffff888020030e20 R15: ffff888078f08000 FS: 0000555556b94380(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000005fdeb8 CR3: 0000000076d22000 CR4: 00000000003506f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> sta_apply_auth_flags.constprop.0+0x4b7/0x510 net/mac80211/cfg.c:1674 sta_apply_parameters+0xaf1/0x16c0 net/mac80211/cfg.c:2002 ieee80211_add_station+0x3fa/0x6c0 net/mac80211/cfg.c:2068 rdev_add_station net/wireless/rdev-ops.h:201 [inline] nl80211_new_station+0x13ba/0x1a70 net/wireless/nl80211.c:7603 genl_family_rcv_msg_doit+0x1fc/0x2e0 net/netlink/genetlink.c:972 genl_family_rcv_msg net/netlink/genetlink.c:1052 [inline] genl_rcv_msg+0x561/0x800 net/netlink/genetlink.c:1067 netlink_rcv_skb+0x16b/0x440 net/netlink/af_netlink.c:2545 genl_rcv+0x28/0x40 net/netlink/genetlink.c:1076 netlink_unicast_kernel net/netlink/af_netlink.c:1342 [inline] netlink_unicast+0x53b/0x810 net/netlink/af_netlink.c:1368 netlink_sendmsg+0x93c/0xe40 net/netlink/af_netlink.c:1910 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0xd5/0x180 net/socket.c:745 ____sys_sendmsg+0x6ac/0x940 net/socket.c:2584 ___sys_sendmsg+0x135/0x1d0 net/socket.c:2638 __sys_sendmsg+0x117/0x1e0 net/socket.c:2667 do_syscall_x64 arch/x86/entry/common.c:51 [inline] do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82 entry_SYSCALL_64_after_hwframe+0x63/0x6b [Analysis] When ieee80211_get_link_sband() fails to find a valid sband and first checks for params in sta_link_apply_parameters(), it will return 0 due to new_link being 0, which will lead to an incorrect process after sta_apply_parameters(). [Fix] First obtain sband and perform a non null check before checking the params. Fixes: b303835dabe0 ("wifi: mac80211: accept STA changes without link changes") Reported-and-tested-by: syzbot+62d7eef57b09bfebcd84@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- net/mac80211/cfg.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)