diff mbox series

wifi: mac80211: sband's null check should precede params

Message ID tencent_0CCA1979CFA30DC8A5CF8DDC92365DCE5D07@qq.com
State New
Headers show
Series wifi: mac80211: sband's null check should precede params | expand

Commit Message

Edward Adam Davis Nov. 29, 2023, 5:48 a.m. UTC
[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(-)

Comments

Johannes Berg Nov. 29, 2023, 6:57 a.m. UTC | #1
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
diff mbox series

Patch

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);