Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()

[Hyunwoo Kim]

The previous mailing list is here:
https://lore.kernel.org/lkml/20220913052020.GA85241@ubuntu/#r


There are 3 other pcmica drivers in the path "drivers/char/pcmcia/synclink_cs.c", 
the path of the "synclink_cs.c" driver I reported the UAF to.
A similar UAF occurs in the "cm4000_cs.c" and "cm4040_cs.c" drivers. 
(this does not happen in scr24x_cs.c)

The flow of UAF occurrence in cm4040_cs.c driver is as follows:
```
                cpu0                                                cpu1
       1. open()
          cm4040_open()
                                                             2. reader_detach()
                                                                reader_release()
                                                                cm4040_reader_release()
                                                                while (link->open) { ...
       3. link->open = 1;
                                                             4. kfree(dev);
                                                                device_destroy()
       5. read()   <- device_destroy() was called, but read() can be called because fd is open
          cm4040_read()
          int iobase = dev->p_dev->resource[0]->start;   <- UAF
```
In cm4040_open() function, link->open is set to 1.
And in the .remove callback reader_detach() function, if link->open is 1, 
cm4040_close() is called and wait()s until link->open becomes 0.
However, if the above race condition occurs in these two functions, 
the link->open check in reader_detach() can be bypassed.
After that, you can call read() on the task that acquired fd to raise a 
UAF for the kfree()d "dev".


The flow of UAF occurrence in cm4000_cs.c driver is as follows:

```
                cpu0                                                cpu1
       1. open()
          cmm_open()
                                                             2. cm4000_detach()
                                                                stop_monitor()
                                                                if (dev->monitor_running) { ...
       3. start_monitor()
          dev->monitor_running = 1;
                                                             4. cm4000_release()
                                                                cmm_cm4000_release()
                                                                while (link->open) { ...
       5. link->open = 1;
                                                             6. kfree(dev);
                                                                device_destroy()
       7. read()   <- device_destroy() was called, but read() can be called because fd is open
          cmm_read()
          unsigned int iobase = dev->p_dev->resource[0]->start;   <- UAF
```
In the cm4000_cs.c driver, the race condition flow is tricky because of 
the start/stop_monitor() functions.

The overall flow is similar to cm4040_cs.c.
Added one race condition to bypass the "dev->monitor_running" check.


So, should the above two drivers be removed from the kernel like the synclink_cs.c driver?

Or should I submit a patch that fixes the UAF?


Best Regards,
Hyunwoo Kim.