diff mbox series

wifi: virt_wifi: avoid reporting connection success with wrong SSID

Message ID 20240704030304.16213-1-en-wei.wu@canonical.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series wifi: virt_wifi: avoid reporting connection success with wrong SSID | expand

Commit Message

En-Wei WU July 4, 2024, 3:03 a.m. UTC 
When user issues a connection with a different SSID than the one
virt_wifi has advertised, the __cfg80211_connect_result() will
trigger the warning: WARN_ON(bss_not_found).

The issue is because the connection code in virt_wifi does not
check the SSID from user space (it only checks the BSSID), and
virt_wifi will call cfg80211_connect_result() with WLAN_STATUS_SUCCESS
even if the SSID is different from the one virt_wifi has advertised.
Eventually cfg80211 won't be able to find the cfg80211_bss and generate
the warning.

Fixed it by checking the SSID (from user space) in the connection code.

Fixes: c7cdba31ed8b ("mac80211-next: rtnetlink wifi simulation device")
Reported-by: syzbot+d6eb9cee2885ec06f5e3@syzkaller.appspotmail.com
Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
---
 drivers/net/wireless/virtual/virt_wifi.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

Comments

En-Wei WU July 4, 2024, 3:12 a.m. UTC | #1 
Hi,

After applying this patch and testing on the machines of syzbot, the
original warning is gone. However, syzbot reported that there is
another warning generated:

     create_new_namespaces+0x425/0x7b0 kernel/nsproxy.c:110
     unshare_nsproxy_namespaces+0x124/0x180 kernel/nsproxy.c:228
     ksys_unshare+0x619/0xc10 kernel/fork.c:3323
     __do_sys_unshare kernel/fork.c:3394 [inline]
     __se_sys_unshare kernel/fork.c:3392 [inline]
     __x64_sys_unshare+0x38/0x40 kernel/fork.c:3392
     do_syscall_x64 arch/x86/entry/common.c:52 [inline]
     do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
     entry_SYSCALL_64_after_hwframe+0x77/0x7f
------------[ cut here ]------------
WARNING: CPU: 1 PID: 11 at lib/ref_tracker.c:179
ref_tracker_dir_exit+0x411/0x550 lib/ref_tracker.c:179
Modules linked in:
CPU: 1 PID: 11 Comm: kworker/u8:0 Not tainted
6.10.0-rc5-syzkaller-01200-gcda91d5b911a-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 06/07/2024
Workqueue: netns cleanup_net
RIP: 0010:ref_tracker_dir_exit+0x411/0x550 lib/ref_tracker.c:179
Code: 48 8b 1c 24 48 89 df 48 8b 74 24 20 e8 88 f2 ab 06 eb 1a e8 f1
b7 ad fc 48 8b 1c 24 48 89 df 48 8b 74 24 20 e8 70 f2 ab 06 90 <0f> 0b
90 48 83 c3 44 48 89 df be 04 00 00 00 e8 5b 7f 13 fd 48 89
RSP: 0018:ffffc900001079e0 EFLAGS: 00010246
RAX: 2ba763440467a200 RBX: ffff8880296b9ed8 RCX: 0000000000000001
RDX: dffffc0000000000 RSI: ffffffff8bcabb40 RDI: 0000000000000001
RBP: ffffc90000107ab0 R08: ffffffff92fbc66f R09: 1ffffffff25f78cd
R10: dffffc0000000000 R11: fffffbfff25f78ce R12: 1ffff1100da640d8
R13: dead000000000100 R14: ffff8880296b9f28 R15: dffffc0000000000
FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055b2700a0950 CR3: 000000002c8b0000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 net_free net/core/net_namespace.c:465 [inline]
 cleanup_net+0xbf3/0xcc0 net/core/net_namespace.c:661
 process_one_work kernel/workqueue.c:3248 [inline]
 process_scheduled_works+0xa2c/0x1830 kernel/workqueue.c:3329
 worker_thread+0x86d/0xd50 kernel/workqueue.c:3409
 kthread+0x2f0/0x390 kernel/kthread.c:389
 ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
 </TASK>

It looks like it's not related to the changes I have made, since the
new warning is probably describing a leak when doing net_free. I will
look at the new warning soon, and maybe we can just focus on the
original warning now.

En-Wei.


On Thu, 4 Jul 2024 at 11:03, En-Wei Wu <en-wei.wu@canonical.com> wrote:
>
> When user issues a connection with a different SSID than the one
> virt_wifi has advertised, the __cfg80211_connect_result() will
> trigger the warning: WARN_ON(bss_not_found).
>
> The issue is because the connection code in virt_wifi does not
> check the SSID from user space (it only checks the BSSID), and
> virt_wifi will call cfg80211_connect_result() with WLAN_STATUS_SUCCESS
> even if the SSID is different from the one virt_wifi has advertised.
> Eventually cfg80211 won't be able to find the cfg80211_bss and generate
> the warning.
>
> Fixed it by checking the SSID (from user space) in the connection code.
>
> Fixes: c7cdba31ed8b ("mac80211-next: rtnetlink wifi simulation device")
> Reported-by: syzbot+d6eb9cee2885ec06f5e3@syzkaller.appspotmail.com
> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> ---
>  drivers/net/wireless/virtual/virt_wifi.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/virtual/virt_wifi.c b/drivers/net/wireless/virtual/virt_wifi.c
> index 6a84ec58d618..86a8f74be654 100644
> --- a/drivers/net/wireless/virtual/virt_wifi.c
> +++ b/drivers/net/wireless/virtual/virt_wifi.c
> @@ -136,6 +136,9 @@ static struct ieee80211_supported_band band_5ghz = {
>  /* Assigned at module init. Guaranteed locally-administered and unicast. */
>  static u8 fake_router_bssid[ETH_ALEN] __ro_after_init = {};
>
> +#define VIRT_WIFI_SSID_LEN 8
> +#define VIRT_WIFI_SSID "VirtWifi"
> +
>  static void virt_wifi_inform_bss(struct wiphy *wiphy)
>  {
>         u64 tsf = div_u64(ktime_get_boottime_ns(), 1000);
> @@ -146,8 +149,8 @@ static void virt_wifi_inform_bss(struct wiphy *wiphy)
>                 u8 ssid[8];
>         } __packed ssid = {
>                 .tag = WLAN_EID_SSID,
> -               .len = 8,
> -               .ssid = "VirtWifi",
> +               .len = VIRT_WIFI_SSID_LEN,
> +               .ssid = VIRT_WIFI_SSID,
>         };
>
>         informed_bss = cfg80211_inform_bss(wiphy, &channel_5ghz,
> @@ -213,6 +216,8 @@ struct virt_wifi_netdev_priv {
>         struct net_device *upperdev;
>         u32 tx_packets;
>         u32 tx_failed;
> +       u32 connect_requested_ssid_len;
> +       u8 connect_requested_ssid[IEEE80211_MAX_SSID_LEN];
>         u8 connect_requested_bss[ETH_ALEN];
>         bool is_up;
>         bool is_connected;
> @@ -224,11 +229,21 @@ static int virt_wifi_connect(struct wiphy *wiphy, struct net_device *netdev,
>                              struct cfg80211_connect_params *sme)
>  {
>         struct virt_wifi_netdev_priv *priv = netdev_priv(netdev);
> +       u32 ssid_len;
>         bool could_schedule;
>
>         if (priv->being_deleted || !priv->is_up)
>                 return -EBUSY;
>
> +       if (!sme->ssid) {
> +               wiphy_err(wiphy, "invalid SSID\n");
> +               return -EINVAL;
> +       }
> +
> +       ssid_len = min_t(u32, sme->ssid_len, IEEE80211_MAX_SSID_LEN);
> +       priv->connect_requested_ssid_len = ssid_len;
> +       memcpy(priv->connect_requested_ssid, sme->ssid, ssid_len);
> +
>         could_schedule = schedule_delayed_work(&priv->connect, HZ * 2);
>         if (!could_schedule)
>                 return -EBUSY;
> @@ -252,12 +267,15 @@ static void virt_wifi_connect_complete(struct work_struct *work)
>                 container_of(work, struct virt_wifi_netdev_priv, connect.work);
>         u8 *requested_bss = priv->connect_requested_bss;
>         bool right_addr = ether_addr_equal(requested_bss, fake_router_bssid);
> +       bool right_ssid = (priv->connect_requested_ssid_len == VIRT_WIFI_SSID_LEN ?
> +                         !memcmp(priv->connect_requested_ssid, VIRT_WIFI_SSID,
> +                                 priv->connect_requested_ssid_len) : false);
>         u16 status = WLAN_STATUS_SUCCESS;
>
>         if (is_zero_ether_addr(requested_bss))
>                 requested_bss = NULL;
>
> -       if (!priv->is_up || (requested_bss && !right_addr))
> +       if (!priv->is_up || (requested_bss && !right_addr) || !right_ssid)
>                 status = WLAN_STATUS_UNSPECIFIED_FAILURE;
>         else
>                 priv->is_connected = true;
> --
> 2.43.0
>
Johannes Berg July 4, 2024, 8:52 a.m. UTC | #2 
On Thu, 2024-07-04 at 11:03 +0800, En-Wei Wu wrote:
> 
> +#define VIRT_WIFI_SSID_LEN 8
> +#define VIRT_WIFI_SSID "VirtWifi"

Use strlen(VIRT_WIFI_SSID) for VIRT_WIFI_SSID_LEN maybe? It should be
constant "enough" for the compiler.

> +	if (!sme->ssid) {
> +		wiphy_err(wiphy, "invalid SSID\n");
> +		return -EINVAL;

I wouldn't print an error here

> +	ssid_len = min_t(u32, sme->ssid_len, IEEE80211_MAX_SSID_LEN);

The min_t() is unnecessary, cfg80211 won't give you too long SSIDs.

>  	bool right_addr = ether_addr_equal(requested_bss, fake_router_bssid);
> +	bool right_ssid = (priv->connect_requested_ssid_len == VIRT_WIFI_SSID_LEN ?
> +			  !memcmp(priv->connect_requested_ssid, VIRT_WIFI_SSID,
> +				  priv->connect_requested_ssid_len) : false);

the ternary seems odd, why not just

 priv->connect_requested_ssid_len == VIRT_WIFI_SSID_LEN &&
 !memcmp(...);

?

johannes
diff mbox series

Patch

diff --git a/drivers/net/wireless/virtual/virt_wifi.c b/drivers/net/wireless/virtual/virt_wifi.c
index 6a84ec58d618..86a8f74be654 100644
--- a/drivers/net/wireless/virtual/virt_wifi.c
+++ b/drivers/net/wireless/virtual/virt_wifi.c
@@ -136,6 +136,9 @@  static struct ieee80211_supported_band band_5ghz = {
 /* Assigned at module init. Guaranteed locally-administered and unicast. */
 static u8 fake_router_bssid[ETH_ALEN] __ro_after_init = {};
 
+#define VIRT_WIFI_SSID_LEN 8
+#define VIRT_WIFI_SSID "VirtWifi"
+
 static void virt_wifi_inform_bss(struct wiphy *wiphy)
 {
 	u64 tsf = div_u64(ktime_get_boottime_ns(), 1000);
@@ -146,8 +149,8 @@  static void virt_wifi_inform_bss(struct wiphy *wiphy)
 		u8 ssid[8];
 	} __packed ssid = {
 		.tag = WLAN_EID_SSID,
-		.len = 8,
-		.ssid = "VirtWifi",
+		.len = VIRT_WIFI_SSID_LEN,
+		.ssid = VIRT_WIFI_SSID,
 	};
 
 	informed_bss = cfg80211_inform_bss(wiphy, &channel_5ghz,
@@ -213,6 +216,8 @@  struct virt_wifi_netdev_priv {
 	struct net_device *upperdev;
 	u32 tx_packets;
 	u32 tx_failed;
+	u32 connect_requested_ssid_len;
+	u8 connect_requested_ssid[IEEE80211_MAX_SSID_LEN];
 	u8 connect_requested_bss[ETH_ALEN];
 	bool is_up;
 	bool is_connected;
@@ -224,11 +229,21 @@  static int virt_wifi_connect(struct wiphy *wiphy, struct net_device *netdev,
 			     struct cfg80211_connect_params *sme)
 {
 	struct virt_wifi_netdev_priv *priv = netdev_priv(netdev);
+	u32 ssid_len;
 	bool could_schedule;
 
 	if (priv->being_deleted || !priv->is_up)
 		return -EBUSY;
 
+	if (!sme->ssid) {
+		wiphy_err(wiphy, "invalid SSID\n");
+		return -EINVAL;
+	}
+
+	ssid_len = min_t(u32, sme->ssid_len, IEEE80211_MAX_SSID_LEN);
+	priv->connect_requested_ssid_len = ssid_len;
+	memcpy(priv->connect_requested_ssid, sme->ssid, ssid_len);
+
 	could_schedule = schedule_delayed_work(&priv->connect, HZ * 2);
 	if (!could_schedule)
 		return -EBUSY;
@@ -252,12 +267,15 @@  static void virt_wifi_connect_complete(struct work_struct *work)
 		container_of(work, struct virt_wifi_netdev_priv, connect.work);
 	u8 *requested_bss = priv->connect_requested_bss;
 	bool right_addr = ether_addr_equal(requested_bss, fake_router_bssid);
+	bool right_ssid = (priv->connect_requested_ssid_len == VIRT_WIFI_SSID_LEN ?
+			  !memcmp(priv->connect_requested_ssid, VIRT_WIFI_SSID,
+				  priv->connect_requested_ssid_len) : false);
 	u16 status = WLAN_STATUS_SUCCESS;
 
 	if (is_zero_ether_addr(requested_bss))
 		requested_bss = NULL;
 
-	if (!priv->is_up || (requested_bss && !right_addr))
+	if (!priv->is_up || (requested_bss && !right_addr) || !right_ssid)
 		status = WLAN_STATUS_UNSPECIFIED_FAILURE;
 	else
 		priv->is_connected = true;