Re: WARNING: refcount bug in cdev_get

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Will,

I am facing same issue while syzkaller fault injection code is causing

failure in filp->f_op->open() from chrdev_open().

I believe we need to rely on refcount as cdev_lock() is not sufficient

in this case.

Patch mentioned in https://groups.google.com/forum/#!original/syzkaller-bugs/PnQNxBrWv_8/X1ygj8d8DgAJ

seems good.


Please share your opinion the same.


Regards

Prateek



On 12/19/2019 5:29 PM, Will Deacon wrote:
On Wed, Dec 18, 2019 at 07:20:26PM +0100, Greg KH wrote:
On Wed, Dec 18, 2019 at 05:08:55PM +0000, Will Deacon wrote:
On Tue, Dec 10, 2019 at 11:44:45AM +0000, Will Deacon wrote:
On Wed, Dec 04, 2019 at 01:31:48PM +0100, Greg KH wrote:
This code hasn't changed in 15+ years, what suddenly changed that causes
problems here?
I suppose one thing to consider is that the refcount code is relatively new,
so it could be that the actual use-after-free is extremely rare, but we're
now seeing that it's at least potentially an issue.

Thoughts?
FWIW, I added some mdelay()s to make this race more likely, and I can now
trigger it reasonably reliably. See below.

--->8

[   89.512353] ------------[ cut here ]------------
[   89.513350] refcount_t: addition on 0; use-after-free.
[   89.513977] WARNING: CPU: 2 PID: 6385 at lib/refcount.c:25 refcount_warn_saturate+0x6d/0xf0
[...]

No hint as to _where_ you put the mdelay()?  :)
I threw it in the release function to maximise the period where the refcount
is 0 but the inode 'i_cdev' pointer is non-NULL. I also hacked chrdev_open()
so that the fops->open() call appears to fail most of the time (I guess
syzkaller uses error injection to do something similar). Nasty hack below.

I'll send a patch, given that I've managed to "reproduce" this.

Will

--->8

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 00dfe17871ac..e2e48fcd0435 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -375,7 +375,7 @@ static int chrdev_open(struct inode *inode, struct file *filp)
  	const struct file_operations *fops;
  	struct cdev *p;
  	struct cdev *new = NULL;
-	int ret = 0;
+	int ret = 0, first = 0;
spin_lock(&cdev_lock);
  	p = inode->i_cdev;
@@ -395,6 +395,7 @@ static int chrdev_open(struct inode *inode, struct file *filp)
  			inode->i_cdev = p = new;
  			list_add(&inode->i_devices, &p->list);
  			new = NULL;
+			first = 1;
  		} else if (!cdev_get(p))
  			ret = -ENXIO;
  	} else if (!cdev_get(p))
@@ -411,6 +412,10 @@ static int chrdev_open(struct inode *inode, struct file *filp)
replace_fops(filp, fops);
  	if (filp->f_op->open) {
+		if (first && (get_cycles() & 0x3)) {
+			ret = -EINTR;
+			goto out_cdev_put;
+		}
  		ret = filp->f_op->open(inode, filp);
  		if (ret)
  			goto out_cdev_put;
@@ -594,12 +599,14 @@ void cdev_del(struct cdev *p)
  	kobject_put(&p->kobj);
  }
+#include <linux/delay.h> static void cdev_default_release(struct kobject *kobj)
  {
  	struct cdev *p = container_of(kobj, struct cdev, kobj);
  	struct kobject *parent = kobj->parent;
+ mdelay(50);
  	cdev_purge(p);
  	kobject_put(parent);
  }

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux