[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <63030af8-5849-34b3-10e6-b6ce32c3a5bf@kernel.org>
Date: Tue, 21 Feb 2023 07:51:46 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Hyunwoo Kim <imv4bel@...il.com>, laforge@...monks.org
Cc: linux-kernel@...r.kernel.org, arnd@...db.de,
gregkh@...uxfoundation.org, linux@...inikbrodowski.net
Subject: Re: [PATCH v3] char: pcmcia: cm4000_cs: Fix use-after-free in
cm4000_fops
Ping -- what's the status of these?
Should we mark cm4000_cs, cm4040_cs, and scr24x_cs as BROKEN instead?
Thanks.
On 19. 09. 22, 6:07, Hyunwoo Kim wrote:
> A race condition may occur if the user physically removes the pcmcia
> device while calling open() for this char device node.
>
> This is a race condition between the cmm_open() function and the
> cm4000_detach() function, which may eventually result in UAF.
>
> So, add a refcount check to cm4000_detach() to free the "dev" structure
> after the char device node is close()d.
>
> Signed-off-by: Hyunwoo Kim <imv4bel@...il.com>
> ---
> drivers/char/pcmcia/cm4000_cs.c | 58 +++++++++++++++++++++++----------
> 1 file changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
> index adaec8fd4b16..7103812b4019 100644
> --- a/drivers/char/pcmcia/cm4000_cs.c
> +++ b/drivers/char/pcmcia/cm4000_cs.c
> @@ -55,6 +55,7 @@
> } while (0)
>
> static DEFINE_MUTEX(cmm_mutex);
> +static DEFINE_MUTEX(remove_mutex);
>
> #define T_1SEC (HZ)
> #define T_10MSEC msecs_to_jiffies(10)
> @@ -103,7 +104,8 @@ static int major; /* major number we get from the kernel */
> #define REG_STOPBITS(x) (x + 7)
>
> struct cm4000_dev {
> - struct pcmcia_device *p_dev;
> + struct pcmcia_device *p_dev;
> + struct kref refcnt;
>
> unsigned char atr[MAX_ATR];
> unsigned char rbuf[512];
> @@ -146,6 +148,9 @@ struct cm4000_dev {
>
> #define ZERO_DEV(dev) memset(&((dev)->init), 0, sizeof((dev)->init))
>
> +static void stop_monitor(struct cm4000_dev *dev);
> +static void cm4000_delete(struct kref *kref);
> +
> static struct pcmcia_device *dev_table[CM4000_MAX_DEV];
> static struct class *cmm_class;
>
> @@ -416,6 +421,30 @@ static struct card_fixup card_fixups[] = {
> },
> };
>
> +
> +static void cm4000_delete(struct kref *kref)
> +{
> + struct cm4000_dev *dev = container_of(kref, struct cm4000_dev, refcnt);
> + struct pcmcia_device *link = dev->p_dev;
> + int devno;
> +
> + /* find device */
> + for (devno = 0; devno < CM4000_MAX_DEV; devno++)
> + if (dev_table[devno] == link)
> + break;
> + if (devno == CM4000_MAX_DEV)
> + return;
> +
> + stop_monitor(dev);
> +
> + cm4000_release(link);
> +
> + dev_table[devno] = NULL;
> + kfree(dev);
> +
> + device_destroy(cmm_class, MKDEV(major, devno));
> +}
> +
> static void set_cardparameter(struct cm4000_dev *dev)
> {
> int i;
> @@ -1629,6 +1658,7 @@ static int cmm_open(struct inode *inode, struct file *filp)
> if (minor >= CM4000_MAX_DEV)
> return -ENODEV;
>
> + mutex_lock(&remove_mutex);
> mutex_lock(&cmm_mutex);
> link = dev_table[minor];
> if (link == NULL || !pcmcia_dev_present(link)) {
> @@ -1673,8 +1703,12 @@ static int cmm_open(struct inode *inode, struct file *filp)
>
> DEBUGP(2, dev, "<- cmm_open\n");
> ret = stream_open(inode, filp);
> +
> + kref_get(&dev->refcnt);
> out:
> mutex_unlock(&cmm_mutex);
> + mutex_unlock(&remove_mutex);
> +
> return ret;
> }
>
> @@ -1703,6 +1737,8 @@ static int cmm_close(struct inode *inode, struct file *filp)
> link->open = 0; /* only one open per device */
> wake_up(&dev->devq); /* socket removed? */
>
> + kref_put(&dev->refcnt, cm4000_delete);
> +
> DEBUGP(2, dev, "cmm_close\n");
> return 0;
> }
> @@ -1808,6 +1844,7 @@ static int cm4000_probe(struct pcmcia_device *link)
> init_waitqueue_head(&dev->ioq);
> init_waitqueue_head(&dev->atrq);
> init_waitqueue_head(&dev->readq);
> + kref_init(&dev->refcnt);
>
> ret = cm4000_config(link, i);
> if (ret) {
> @@ -1824,23 +1861,10 @@ static int cm4000_probe(struct pcmcia_device *link)
> static void cm4000_detach(struct pcmcia_device *link)
> {
> struct cm4000_dev *dev = link->priv;
> - int devno;
> -
> - /* find device */
> - for (devno = 0; devno < CM4000_MAX_DEV; devno++)
> - if (dev_table[devno] == link)
> - break;
> - if (devno == CM4000_MAX_DEV)
> - return;
> -
> - stop_monitor(dev);
> -
> - cm4000_release(link);
>
> - dev_table[devno] = NULL;
> - kfree(dev);
> -
> - device_destroy(cmm_class, MKDEV(major, devno));
> + mutex_lock(&remove_mutex);
> + kref_put(&dev->refcnt, cm4000_delete);
> + mutex_unlock(&remove_mutex);
>
> return;
> }
--
js
suse labs
Powered by blists - more mailing lists