Messages in this thread Patch in this message | | | Date | Sun, 18 Sep 2022 21:08:54 -0700 | From | Hyunwoo Kim <> | Subject | [PATCH v3] char: pcmcia: scr24x_cs: Fix use-after-free in scr24x_fops |
| |
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 scr24x_open() function and the scr24x_remove() function, which may eventually result in UAF.
So, add a mutex to the scr24x_open() and scr24x_remove() functions to avoid race contidion of krefs.
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> --- drivers/char/pcmcia/scr24x_cs.c | 72 +++++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 21 deletions(-)
diff --git a/drivers/char/pcmcia/scr24x_cs.c b/drivers/char/pcmcia/scr24x_cs.c index 1bdce08fae3d..57fe08b3c03a 100644 --- a/drivers/char/pcmcia/scr24x_cs.c +++ b/drivers/char/pcmcia/scr24x_cs.c @@ -33,6 +33,7 @@ struct scr24x_dev { struct device *dev; + struct pcmcia_device *p_dev; struct cdev c_dev; unsigned char buf[CCID_MAX_LEN]; int devno; @@ -42,15 +43,31 @@ struct scr24x_dev { }; #define SCR24X_DEVS 8 -static DECLARE_BITMAP(scr24x_minors, SCR24X_DEVS); +static struct pcmcia_device *dev_table[SCR24X_DEVS]; +static DEFINE_MUTEX(remove_mutex); static struct class *scr24x_class; static dev_t scr24x_devt; static void scr24x_delete(struct kref *kref) { - struct scr24x_dev *dev = container_of(kref, struct scr24x_dev, - refcnt); + struct scr24x_dev *dev = container_of(kref, struct scr24x_dev, refcnt); + struct pcmcia_device *link = dev->p_dev; + int devno; + + for (devno = 0; devno < SCR24X_DEVS; devno++) { + if (dev_table[devno] == link) + break; + } + if (devno == SCR24X_DEVS) + return; + + device_destroy(scr24x_class, MKDEV(MAJOR(scr24x_devt), dev->devno)); + mutex_lock(&dev->lock); + pcmcia_disable_device(link); + cdev_del(&dev->c_dev); + dev->dev = NULL; + mutex_unlock(&dev->lock); kfree(dev); } @@ -73,11 +90,23 @@ static int scr24x_wait_ready(struct scr24x_dev *dev) static int scr24x_open(struct inode *inode, struct file *filp) { - struct scr24x_dev *dev = container_of(inode->i_cdev, - struct scr24x_dev, c_dev); + struct scr24x_dev *dev; + struct pcmcia_device *link; + int minor = iminor(inode); + + if (minor >= SCR24X_DEVS) + return -ENODEV; + + mutex_lock(&remove_mutex); + link = dev_table[minor]; + if (link == NULL) { + mutex_unlock(&remove_mutex); + return -ENODEV; + } kref_get(&dev->refcnt); filp->private_data = dev; + mutex_unlock(&remove_mutex); return stream_open(inode, filp); } @@ -232,24 +261,31 @@ static int scr24x_config_check(struct pcmcia_device *link, void *priv_data) static int scr24x_probe(struct pcmcia_device *link) { struct scr24x_dev *dev; - int ret; + int i, ret; + + for (i = 0; i < SCR24X_DEVS; i++) { + if (dev_table[i] == NULL) + break; + } + + if (i == SCR24X_DEVS) + return -ENODEV; dev = kzalloc(sizeof(*dev), GFP_KERNEL); if (!dev) return -ENOMEM; - dev->devno = find_first_zero_bit(scr24x_minors, SCR24X_DEVS); - if (dev->devno >= SCR24X_DEVS) { - ret = -EBUSY; - goto err; - } + dev->devno = i; mutex_init(&dev->lock); kref_init(&dev->refcnt); link->priv = dev; + dev->p_dev = link; link->config_flags |= CONF_ENABLE_IRQ | CONF_AUTO_SET_IO; + dev_table[i] = link; + ret = pcmcia_loop_config(link, scr24x_config_check, NULL); if (ret < 0) goto err; @@ -282,8 +318,8 @@ static int scr24x_probe(struct pcmcia_device *link) return 0; err: - if (dev->devno < SCR24X_DEVS) - clear_bit(dev->devno, scr24x_minors); + dev_table[i] = NULL; + kfree (dev); return ret; } @@ -292,15 +328,9 @@ static void scr24x_remove(struct pcmcia_device *link) { struct scr24x_dev *dev = (struct scr24x_dev *)link->priv; - device_destroy(scr24x_class, MKDEV(MAJOR(scr24x_devt), dev->devno)); - mutex_lock(&dev->lock); - pcmcia_disable_device(link); - cdev_del(&dev->c_dev); - clear_bit(dev->devno, scr24x_minors); - dev->dev = NULL; - mutex_unlock(&dev->lock); - + mutex_lock(&remove_mutex); kref_put(&dev->refcnt, scr24x_delete); + mutex_unlock(&remove_mutex); } static const struct pcmcia_device_id scr24x_ids[] = { -- 2.25.1
Dear,
I fixed the wrong patch referencing "dev" after kref_put() in the previous version of the patch.
Regards, Hyunwoo Kim.
| |