Messages in this thread Patch in this message | | | Date | Sun, 18 Sep 2022 21:07:01 -0700 | From | Hyunwoo Kim <> | Subject | [PATCH v3] char: pcmcia: cm4000_cs: Fix use-after-free in cm4000_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 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@gmail.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; } -- 2.25.1
Dear,
I fixed the wrong patch referencing "dev" after kref_put() in the previous version of the patch.
Regards, Hyunwoo Kim.
| |