[PATCH] char: pcmcia: cm4040_cs: Fix use-after-free in reader_fops

* [PATCH] char: pcmcia: cm4040_cs: Fix use-after-free in reader_fops
@ 2022-09-16  4:58 Hyunwoo Kim
  2022-09-16 12:07 ` kernel test robot
  0 siblings, 1 reply; 2+ messages in thread
From: Hyunwoo Kim @ 2022-09-16  4:58 UTC (permalink / raw)
  To: laforge, arnd, gregkh; +Cc: linux-kernel, imv4bel

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 cm4040_open() function and the
reader_detach() function, which may eventually result in UAF.

So, add a refcount check to reader_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/cm4040_cs.c | 88 ++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 39 deletions(-)

diff --git a/drivers/char/pcmcia/cm4040_cs.c b/drivers/char/pcmcia/cm4040_cs.c
index 827711911da4..4cc6df93be2c 100644
--- a/drivers/char/pcmcia/cm4040_cs.c
+++ b/drivers/char/pcmcia/cm4040_cs.c
@@ -59,6 +59,7 @@ static DEFINE_MUTEX(cm4040_mutex);
 /* how often to poll for fifo status change */
 #define POLL_PERIOD 				msecs_to_jiffies(10)
 
+static void cm4040_delete(struct kref *kref);
 static void reader_release(struct pcmcia_device *link);
 
 static int major;
@@ -69,15 +70,16 @@ static struct class *cmx_class;
 
 struct reader_dev {
 	struct pcmcia_device	*p_dev;
-	wait_queue_head_t	devq;
 	wait_queue_head_t	poll_wait;
 	wait_queue_head_t	read_wait;
 	wait_queue_head_t	write_wait;
+	struct kref		refcnt;
 	unsigned long 	  	buffer_status;
 	unsigned long     	timeout;
 	unsigned char     	s_buf[READ_WRITE_BUFFER_SIZE];
 	unsigned char     	r_buf[READ_WRITE_BUFFER_SIZE];
 	struct timer_list 	poll_timer;
+	struct mutex		lock;
 };
 
 static struct pcmcia_device *dev_table[CM_MAX_DEV];
@@ -102,6 +104,28 @@ static inline unsigned char xinb(unsigned short port)
 }
 #endif
 
+static void cm4040_delete(struct kref *kref)
+{
+	struct reader_dev *dev = container_of(kref, struct reader_dev, refcnt);
+	struct pcmcia_device *link = dev->p_dev;
+	int devno;
+
+	/* find device */
+	for (devno = 0; devno < CM_MAX_DEV; devno++) {
+		if (dev_table[devno] == link)
+			break;
+	}
+	if (devno == CM_MAX_DEV)
+		return;
+
+	reader_release(link);
+
+	dev_table[devno] = NULL;
+	kfree(dev);
+
+	device_destroy(cmx_class, MKDEV(major, devno));
+}
+
 /* poll the device fifo status register.  not to be confused with
  * the poll syscall. */
 static void cm4040_do_poll(struct timer_list *t)
@@ -442,24 +466,30 @@ static int cm4040_open(struct inode *inode, struct file *filp)
 		return -ENODEV;
 
 	mutex_lock(&cm4040_mutex);
+
 	link = dev_table[minor];
 	if (link == NULL || !pcmcia_dev_present(link)) {
-		ret = -ENODEV;
-		goto out;
+		mutex_unlock(&dev->lock);
+		mutex_unlock(&cm4040_mutex);
+		return -ENODEV;
 	}
 
 	if (link->open) {
-		ret = -EBUSY;
-		goto out;
+		mutex_unlock(&dev->lock);
+		mutex_unlock(&cm4040_mutex);
+		return -EBUSY;
 	}
 
 	dev = link->priv;
+	mutex_lock(&dev->lock);
+
 	filp->private_data = dev;
 
 	if (filp->f_flags & O_NONBLOCK) {
 		DEBUGP(4, dev, "filep->f_flags O_NONBLOCK set\n");
-		ret = -EAGAIN;
-		goto out;
+		mutex_unlock(&dev->lock);
+		mutex_unlock(&cm4040_mutex);
+		return -EAGAIN;
 	}
 
 	link->open = 1;
@@ -468,8 +498,12 @@ static int cm4040_open(struct inode *inode, struct file *filp)
 
 	DEBUGP(2, dev, "<- cm4040_open (successfully)\n");
 	ret = nonseekable_open(inode, filp);
-out:
+
+	kref_get(&dev->refcnt);
+
+	mutex_unlock(&dev->lock);
 	mutex_unlock(&cm4040_mutex);
+
 	return ret;
 }
 
@@ -492,24 +526,12 @@ static int cm4040_close(struct inode *inode, struct file *filp)
 	cm4040_stop_poll(dev);
 
 	link->open = 0;
-	wake_up(&dev->devq);
 
 	DEBUGP(2, dev, "<- cm4040_close\n");
-	return 0;
-}
 
-static void cm4040_reader_release(struct pcmcia_device *link)
-{
-	struct reader_dev *dev = link->priv;
+	kref_put(&dev->refcnt, cm4040_delete);
 
-	DEBUGP(3, dev, "-> cm4040_reader_release\n");
-	while (link->open) {
-		DEBUGP(3, dev, MODULE_NAME ": delaying release "
-		       "until process has terminated\n");
- 		wait_event(dev->devq, (link->open == 0));
-	}
-	DEBUGP(3, dev, "<- cm4040_reader_release\n");
-	return;
+	return 0;
 }
 
 static int cm4040_config_check(struct pcmcia_device *p_dev, void *priv_data)
@@ -550,7 +572,6 @@ static int reader_config(struct pcmcia_device *link, int devno)
 
 static void reader_release(struct pcmcia_device *link)
 {
-	cm4040_reader_release(link);
 	pcmcia_disable_device(link);
 }
 
@@ -579,11 +600,12 @@ static int reader_probe(struct pcmcia_device *link)
 
 	dev_table[i] = link;
 
-	init_waitqueue_head(&dev->devq);
 	init_waitqueue_head(&dev->poll_wait);
 	init_waitqueue_head(&dev->read_wait);
 	init_waitqueue_head(&dev->write_wait);
 	timer_setup(&dev->poll_timer, cm4040_do_poll, 0);
+	kref_init(&dev->refcnt);
+	mutex_init(&dev->lock);
 
 	ret = reader_config(link, i);
 	if (ret) {
@@ -600,22 +622,10 @@ static int reader_probe(struct pcmcia_device *link)
 static void reader_detach(struct pcmcia_device *link)
 {
 	struct reader_dev *dev = link->priv;
-	int devno;
-
-	/* find device */
-	for (devno = 0; devno < CM_MAX_DEV; devno++) {
-		if (dev_table[devno] == link)
-			break;
-	}
-	if (devno == CM_MAX_DEV)
-		return;
 
-	reader_release(link);
-
-	dev_table[devno] = NULL;
-	kfree(dev);
-
-	device_destroy(cmx_class, MKDEV(major, devno));
+	mutex_lock(&dev->lock);
+	kref_put(&dev->refcnt, cm4040_delete);
+	mutex_unlock(&dev->lock);
 
 	return;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread