Messages in this thread | | | Subject | Re: WARNING: refcount bug in cdev_get | From | Prateek Sood <> | Date | Tue, 24 Dec 2019 18:29:18 +0530 |
| |
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
| |