Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 14 Sep 2022 19:08:34 -0700
From:   Hyunwoo Kim <imv4bel@gmail.com>
To:     Arnd Bergmann <arnd@arndb.de>, laforge@gnumonks.org,
        gregkh@linuxfoundation.org
Cc:     Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        linux-kernel@vger.kernel.org,
        Dominik Brodowski <linux@dominikbrodowski.net>,
        Paul Fulghum <paulkf@microgate.com>, akpm@osdl.org,
        imv4bel@gmail.com
Subject: Re: [PATCH] pcmcia: synclink_cs: Fix use-after-free in mgslpc_ioctl()
Message-ID: <20220915020834.GA110086@ubuntu>
References: <20220913052020.GA85241@ubuntu>
 <a8a9fd74-4ee5-4619-8492-be7139e6d48e@www.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a8a9fd74-4ee5-4619-8492-be7139e6d48e@www.fastmail.com>
Precedence: bulk

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.