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 <[email protected]>
---
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
Hi Hyunwoo,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on soc/for-next linus/master v6.0-rc5 next-20220916]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hyunwoo-Kim/char-pcmcia-cm4040_cs-Fix-use-after-free-in-reader_fops/20220916-125917
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git ceecbbddbf549fe0b7ffa3804a6e255b3360030f
config: i386-randconfig-a011 (https://download.01.org/0day-ci/archive/20220916/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/578c8f062f3dcbc2fb85f060f74d0522bcf34815
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hyunwoo-Kim/char-pcmcia-cm4040_cs-Fix-use-after-free-in-reader_fops/20220916-125917
git checkout 578c8f062f3dcbc2fb85f060f74d0522bcf34815
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/char/pcmcia/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/char/pcmcia/cm4040_cs.c:472:17: warning: variable 'dev' is uninitialized when used here [-Wuninitialized]
mutex_unlock(&dev->lock);
^~~
drivers/char/pcmcia/cm4040_cs.c:460:24: note: initialize the variable 'dev' to silence this warning
struct reader_dev *dev;
^
= NULL
1 warning generated.
vim +/dev +472 drivers/char/pcmcia/cm4040_cs.c
457
458 static int cm4040_open(struct inode *inode, struct file *filp)
459 {
460 struct reader_dev *dev;
461 struct pcmcia_device *link;
462 int minor = iminor(inode);
463 int ret;
464
465 if (minor >= CM_MAX_DEV)
466 return -ENODEV;
467
468 mutex_lock(&cm4040_mutex);
469
470 link = dev_table[minor];
471 if (link == NULL || !pcmcia_dev_present(link)) {
> 472 mutex_unlock(&dev->lock);
473 mutex_unlock(&cm4040_mutex);
474 return -ENODEV;
475 }
476
477 if (link->open) {
478 mutex_unlock(&dev->lock);
479 mutex_unlock(&cm4040_mutex);
480 return -EBUSY;
481 }
482
483 dev = link->priv;
484 mutex_lock(&dev->lock);
485
486 filp->private_data = dev;
487
488 if (filp->f_flags & O_NONBLOCK) {
489 DEBUGP(4, dev, "filep->f_flags O_NONBLOCK set\n");
490 mutex_unlock(&dev->lock);
491 mutex_unlock(&cm4040_mutex);
492 return -EAGAIN;
493 }
494
495 link->open = 1;
496
497 mod_timer(&dev->poll_timer, jiffies + POLL_PERIOD);
498
499 DEBUGP(2, dev, "<- cm4040_open (successfully)\n");
500 ret = nonseekable_open(inode, filp);
501
502 kref_get(&dev->refcnt);
503
504 mutex_unlock(&dev->lock);
505 mutex_unlock(&cm4040_mutex);
506
507 return ret;
508 }
509
--
0-DAY CI Kernel Test Service
https://01.org/lkp