WARNING: refcount bug in cdev_get

WARNING: refcount bug in cdev_get

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    2d63ba3e Merge tag 'pm-5.3-rc5' of git://git.kernel.org/pu..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=165d3302600000
kernel config:  https://syzkaller.appspot.com/x/.config?x=3ff364e429585cf2
dashboard link: https://syzkaller.appspot.com/bug?extid=82defefbbd8527e1c2cb
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16c8ab3c600000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16be0c4c600000

Bisection is inconclusive: the bug happens on the oldest tested release.

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=11de3622600000
console output: https://syzkaller.appspot.com/x/log.txt?x=15de3622600000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+82defefbbd8527e1c2cb@syzkaller.appspotmail.com

------------[ cut here ]------------
refcount_t: increment on 0; use-after-free.
WARNING: CPU: 1 PID: 11828 at lib/refcount.c:156 refcount_inc_checked  
lib/refcount.c:156 [inline]
WARNING: CPU: 1 PID: 11828 at lib/refcount.c:156  
refcount_inc_checked+0x61/0x70 lib/refcount.c:154
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 11828 Comm: syz-executor746 Not tainted 5.3.0-rc4+ #112
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x172/0x1f0 lib/dump_stack.c:113
  panic+0x2dc/0x755 kernel/panic.c:219
  __warn.cold+0x20/0x4c kernel/panic.c:576
  report_bug+0x263/0x2b0 lib/bug.c:186
  fixup_bug arch/x86/kernel/traps.c:179 [inline]
  fixup_bug arch/x86/kernel/traps.c:174 [inline]
  do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:272
  do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:291
  invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
RIP: 0010:refcount_inc_checked lib/refcount.c:156 [inline]
RIP: 0010:refcount_inc_checked+0x61/0x70 lib/refcount.c:154
Code: 1d 8e c6 64 06 31 ff 89 de e8 ab 9c 35 fe 84 db 75 dd e8 62 9b 35 fe  
48 c7 c7 00 05 c6 87 c6 05 6e c6 64 06 01 e8 67 26 07 fe <0f> 0b eb c1 90  
90 90 90 90 90 90 90 90 90 90 55 48 89 e5 41 57 41
RSP: 0018:ffff8880907d78b8 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff815c2466 RDI: ffffed10120faf09
RBP: ffff8880907d78c8 R08: ffff8880a771a200 R09: fffffbfff134ae48
R10: fffffbfff134ae47 R11: ffffffff89a5723f R12: ffff88809ea2bb80
R13: 0000000000000000 R14: ffff88809ff6cd40 R15: ffff8880a1c56480
  kref_get include/linux/kref.h:45 [inline]
  kobject_get+0x66/0xc0 lib/kobject.c:644
  cdev_get+0x60/0xb0 fs/char_dev.c:355
  chrdev_open+0xb0/0x6b0 fs/char_dev.c:400
  do_dentry_open+0x4df/0x1250 fs/open.c:797
  vfs_open+0xa0/0xd0 fs/open.c:906
  do_last fs/namei.c:3416 [inline]
  path_openat+0x10e9/0x4630 fs/namei.c:3533
  do_filp_open+0x1a1/0x280 fs/namei.c:3563
  do_sys_open+0x3fe/0x5d0 fs/open.c:1089
  __do_sys_open fs/open.c:1107 [inline]
  __se_sys_open fs/open.c:1102 [inline]
  __x64_sys_open+0x7e/0xc0 fs/open.c:1102
  do_syscall_64+0xfd/0x6a0 arch/x86/entry/common.c:296
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x406311
Code: 75 14 b8 02 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 a4 18 00 00 c3 48  
83 ec 08 e8 0a fc ff ff 48 89 04 24 b8 02 00 00 00 0f 05 <48> 8b 3c 24 48  
89 c2 e8 53 fc ff ff 48 89 d0 48 83 c4 08 48 3d 01
RSP: 002b:00007f047e1c0960 EFLAGS: 00000293 ORIG_RAX: 0000000000000002
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000406311
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00007f047e1c0970
RBP: 6666666666666667 R08: 000000000000000f R09: 00007f047e1c1700
R10: 00007f047e1c19d0 R11: 0000000000000293 R12: 00000000006dbc3c
R13: 0000000000000000 R14: 0000000000000000 R15: 00000000317a7973
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: WARNING: refcount bug in cdev_get

[Will Deacon]

Hi all,

[+Hillf, +akpm, +Greg]

On Tue, Aug 20, 2019 at 03:58:06PM -0700, syzbot wrote:
> syzbot found the following crash on:
> 
> HEAD commit:    2d63ba3e Merge tag 'pm-5.3-rc5' of git://git.kernel.org/pu..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=165d3302600000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=3ff364e429585cf2
> dashboard link: https://syzkaller.appspot.com/bug?extid=82defefbbd8527e1c2cb
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16c8ab3c600000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16be0c4c600000
> 
> Bisection is inconclusive: the bug happens on the oldest tested release.
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=11de3622600000
> console output: https://syzkaller.appspot.com/x/log.txt?x=15de3622600000
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+82defefbbd8527e1c2cb@syzkaller.appspotmail.com
> 
> ------------[ cut here ]------------
> refcount_t: increment on 0; use-after-free.
> WARNING: CPU: 1 PID: 11828 at lib/refcount.c:156 refcount_inc_checked
> lib/refcount.c:156 [inline]
> WARNING: CPU: 1 PID: 11828 at lib/refcount.c:156
> refcount_inc_checked+0x61/0x70 lib/refcount.c:154
> Kernel panic - not syncing: panic_on_warn set ...

[...]

> RIP: 0010:refcount_inc_checked lib/refcount.c:156 [inline]
> RIP: 0010:refcount_inc_checked+0x61/0x70 lib/refcount.c:154
> Code: 1d 8e c6 64 06 31 ff 89 de e8 ab 9c 35 fe 84 db 75 dd e8 62 9b 35 fe
> 48 c7 c7 00 05 c6 87 c6 05 6e c6 64 06 01 e8 67 26 07 fe <0f> 0b eb c1 90 90
> 90 90 90 90 90 90 90 90 90 55 48 89 e5 41 57 41
> RSP: 0018:ffff8880907d78b8 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffffffff815c2466 RDI: ffffed10120faf09
> RBP: ffff8880907d78c8 R08: ffff8880a771a200 R09: fffffbfff134ae48
> R10: fffffbfff134ae47 R11: ffffffff89a5723f R12: ffff88809ea2bb80
> R13: 0000000000000000 R14: ffff88809ff6cd40 R15: ffff8880a1c56480
>  kref_get include/linux/kref.h:45 [inline]
>  kobject_get+0x66/0xc0 lib/kobject.c:644
>  cdev_get+0x60/0xb0 fs/char_dev.c:355
>  chrdev_open+0xb0/0x6b0 fs/char_dev.c:400
>  do_dentry_open+0x4df/0x1250 fs/open.c:797
>  vfs_open+0xa0/0xd0 fs/open.c:906
>  do_last fs/namei.c:3416 [inline]
>  path_openat+0x10e9/0x4630 fs/namei.c:3533
>  do_filp_open+0x1a1/0x280 fs/namei.c:3563
>  do_sys_open+0x3fe/0x5d0 fs/open.c:1089

FWIW, we've run into this same crash on arm64 so it would be nice to see it
fixed upstream. It looks like Hillf's reply (which included a patch) didn't
make it to the kernel mailing lists for some reason, but it is available
here:

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

A simpler fix would just be to use kobject_get_unless_zero() directly in
cdev_get(), but that looks odd in this specific case because chrdev_open()
holds the 'cdev_lock' and you'd hope that finding the kobject in the inode
with that held would mean that it's not being freed at the same time.

Cheers,

Will

Re: WARNING: refcount bug in cdev_get

[Greg KH]

On Wed, Dec 04, 2019 at 11:50:56AM +0000, Will Deacon wrote:
> Hi all,
> 
> [+Hillf, +akpm, +Greg]
> 
> On Tue, Aug 20, 2019 at 03:58:06PM -0700, syzbot wrote:
> > syzbot found the following crash on:
> > 
> > HEAD commit:    2d63ba3e Merge tag 'pm-5.3-rc5' of git://git.kernel.org/pu..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=165d3302600000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=3ff364e429585cf2
> > dashboard link: https://syzkaller.appspot.com/bug?extid=82defefbbd8527e1c2cb
> > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16c8ab3c600000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16be0c4c600000
> > 
> > Bisection is inconclusive: the bug happens on the oldest tested release.
> > 
> > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=11de3622600000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=15de3622600000
> > 
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+82defefbbd8527e1c2cb@syzkaller.appspotmail.com
> > 
> > ------------[ cut here ]------------
> > refcount_t: increment on 0; use-after-free.
> > WARNING: CPU: 1 PID: 11828 at lib/refcount.c:156 refcount_inc_checked
> > lib/refcount.c:156 [inline]
> > WARNING: CPU: 1 PID: 11828 at lib/refcount.c:156
> > refcount_inc_checked+0x61/0x70 lib/refcount.c:154
> > Kernel panic - not syncing: panic_on_warn set ...
> 
> [...]
> 
> > RIP: 0010:refcount_inc_checked lib/refcount.c:156 [inline]
> > RIP: 0010:refcount_inc_checked+0x61/0x70 lib/refcount.c:154
> > Code: 1d 8e c6 64 06 31 ff 89 de e8 ab 9c 35 fe 84 db 75 dd e8 62 9b 35 fe
> > 48 c7 c7 00 05 c6 87 c6 05 6e c6 64 06 01 e8 67 26 07 fe <0f> 0b eb c1 90 90
> > 90 90 90 90 90 90 90 90 90 55 48 89 e5 41 57 41
> > RSP: 0018:ffff8880907d78b8 EFLAGS: 00010282
> > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> > RDX: 0000000000000000 RSI: ffffffff815c2466 RDI: ffffed10120faf09
> > RBP: ffff8880907d78c8 R08: ffff8880a771a200 R09: fffffbfff134ae48
> > R10: fffffbfff134ae47 R11: ffffffff89a5723f R12: ffff88809ea2bb80
> > R13: 0000000000000000 R14: ffff88809ff6cd40 R15: ffff8880a1c56480
> >  kref_get include/linux/kref.h:45 [inline]
> >  kobject_get+0x66/0xc0 lib/kobject.c:644
> >  cdev_get+0x60/0xb0 fs/char_dev.c:355
> >  chrdev_open+0xb0/0x6b0 fs/char_dev.c:400
> >  do_dentry_open+0x4df/0x1250 fs/open.c:797
> >  vfs_open+0xa0/0xd0 fs/open.c:906
> >  do_last fs/namei.c:3416 [inline]
> >  path_openat+0x10e9/0x4630 fs/namei.c:3533
> >  do_filp_open+0x1a1/0x280 fs/namei.c:3563
> >  do_sys_open+0x3fe/0x5d0 fs/open.c:1089
> 
> FWIW, we've run into this same crash on arm64 so it would be nice to see it
> fixed upstream. It looks like Hillf's reply (which included a patch) didn't
> make it to the kernel mailing lists for some reason, but it is available
> here:
> 
> https://groups.google.com/forum/#!original/syzkaller-bugs/PnQNxBrWv_8/X1ygj8d8DgAJ

No one is going to go and dig a patch out of google groups :(

> A simpler fix would just be to use kobject_get_unless_zero() directly in
> cdev_get(), but that looks odd in this specific case because chrdev_open()
> holds the 'cdev_lock' and you'd hope that finding the kobject in the inode
> with that held would mean that it's not being freed at the same time.

When using kref_get_unless_zero() that implies that a lock is not being
used and you are relying on the kobject only instead.

But I thought we had a lock in play here, so why would changing this
actually fix anything?

This code hasn't changed in 15+ years, what suddenly changed that causes
problems here?

thanks,

greg k-h

Re: WARNING: refcount bug in cdev_get

[Will Deacon]

Hi Greg,

On Wed, Dec 04, 2019 at 01:31:48PM +0100, Greg KH wrote:
> On Wed, Dec 04, 2019 at 11:50:56AM +0000, Will Deacon wrote:
> > On Tue, Aug 20, 2019 at 03:58:06PM -0700, syzbot wrote:
> > > syzbot found the following crash on:
> > > 
> > > HEAD commit:    2d63ba3e Merge tag 'pm-5.3-rc5' of git://git.kernel.org/pu..
> > > git tree:       upstream
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=165d3302600000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=3ff364e429585cf2
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=82defefbbd8527e1c2cb
> > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16c8ab3c600000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16be0c4c600000
> > > 
> > > Bisection is inconclusive: the bug happens on the oldest tested release.
> > > 
> > > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=11de3622600000
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=15de3622600000
> > > 
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+82defefbbd8527e1c2cb@syzkaller.appspotmail.com
> > > 
> > > ------------[ cut here ]------------
> > > refcount_t: increment on 0; use-after-free.
> > > WARNING: CPU: 1 PID: 11828 at lib/refcount.c:156 refcount_inc_checked
> > > lib/refcount.c:156 [inline]
> > > WARNING: CPU: 1 PID: 11828 at lib/refcount.c:156
> > > refcount_inc_checked+0x61/0x70 lib/refcount.c:154
> > > Kernel panic - not syncing: panic_on_warn set ...
> > 
> > [...]
> > 
> > > RIP: 0010:refcount_inc_checked lib/refcount.c:156 [inline]
> > > RIP: 0010:refcount_inc_checked+0x61/0x70 lib/refcount.c:154
> > > Code: 1d 8e c6 64 06 31 ff 89 de e8 ab 9c 35 fe 84 db 75 dd e8 62 9b 35 fe
> > > 48 c7 c7 00 05 c6 87 c6 05 6e c6 64 06 01 e8 67 26 07 fe <0f> 0b eb c1 90 90
> > > 90 90 90 90 90 90 90 90 90 55 48 89 e5 41 57 41
> > > RSP: 0018:ffff8880907d78b8 EFLAGS: 00010282
> > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> > > RDX: 0000000000000000 RSI: ffffffff815c2466 RDI: ffffed10120faf09
> > > RBP: ffff8880907d78c8 R08: ffff8880a771a200 R09: fffffbfff134ae48
> > > R10: fffffbfff134ae47 R11: ffffffff89a5723f R12: ffff88809ea2bb80
> > > R13: 0000000000000000 R14: ffff88809ff6cd40 R15: ffff8880a1c56480
> > >  kref_get include/linux/kref.h:45 [inline]
> > >  kobject_get+0x66/0xc0 lib/kobject.c:644
> > >  cdev_get+0x60/0xb0 fs/char_dev.c:355
> > >  chrdev_open+0xb0/0x6b0 fs/char_dev.c:400
> > >  do_dentry_open+0x4df/0x1250 fs/open.c:797
> > >  vfs_open+0xa0/0xd0 fs/open.c:906
> > >  do_last fs/namei.c:3416 [inline]
> > >  path_openat+0x10e9/0x4630 fs/namei.c:3533
> > >  do_filp_open+0x1a1/0x280 fs/namei.c:3563
> > >  do_sys_open+0x3fe/0x5d0 fs/open.c:1089
> > 
> > FWIW, we've run into this same crash on arm64 so it would be nice to see it
> > fixed upstream. It looks like Hillf's reply (which included a patch) didn't
> > make it to the kernel mailing lists for some reason, but it is available
> > here:
> > 
> > https://groups.google.com/forum/#!original/syzkaller-bugs/PnQNxBrWv_8/X1ygj8d8DgAJ
> 
> No one is going to go and dig a patch out of google groups :(

Sure, just thought it was worth mentioning after digging up the history.

> > A simpler fix would just be to use kobject_get_unless_zero() directly in
> > cdev_get(), but that looks odd in this specific case because chrdev_open()
> > holds the 'cdev_lock' and you'd hope that finding the kobject in the inode
> > with that held would mean that it's not being freed at the same time.
> 
> When using kref_get_unless_zero() that implies that a lock is not being
> used and you are relying on the kobject only instead.
> 
> But I thought we had a lock in play here, so why would changing this
> actually fix anything?

I don't think the lock is always used. For example, look at chrdev_open(),
which appears in the backtrace; the locked code is:

	spin_lock(&cdev_lock);
	p = inode->i_cdev;
	if (!p) {
		struct kobject *kobj;
		int idx;
		spin_unlock(&cdev_lock);
		kobj = kobj_lookup(cdev_map, inode->i_rdev, &idx);
		if (!kobj)
			return -ENXIO;
		new = container_of(kobj, struct cdev, kobj);
		spin_lock(&cdev_lock);
		/* Check i_cdev again in case somebody beat us to it while
		   we dropped the lock. */
		p = inode->i_cdev;
		if (!p) {
			inode->i_cdev = p = new;
			list_add(&inode->i_devices, &p->list);
			new = NULL;
		} else if (!cdev_get(p))
			ret = -ENXIO;
	} else if (!cdev_get(p))
		ret = -ENXIO;
	spin_unlock(&cdev_lock);
	cdev_put(new);

So the idea is that multiple threads serialise on the 'cdev_lock' and then
check 'inode->i_cdev' to figure out if the device has already been probed,
taking a reference to it if it's available or probing it via kobj_lookup()
otherwise. I think that's backwards with respect to things like cdev_put(),
where the refcount is dropped *before* 'inode->i_cdev' is cleared to NULL.
In which case, if a concurrent call to cdev_put() can drop the refcount
to zero without 'cdev_lock' held, then you could get a use-after-free on
this path thanks to a dangling pointer in 'inode->i_cdev'..

Looking slightly ahead in this same function, there are error paths which
appear to do exactly that:

	fops = fops_get(p->ops);
	if (!fops)
		goto out_cdev_put;

	replace_fops(filp, fops);
	if (filp->f_op->open) {
		ret = filp->f_op->open(inode, filp);
		if (ret)
			goto out_cdev_put;
	}

	return 0;

 out_cdev_put:
	cdev_put(p);
	return ret;

In which case the thread which installed 'inode->i_cdev' earlier on can
now drop its refcount to zero without the lock held if, for example, the
filp->f_op->open() call fails.

But note, this is purely based on code inspection -- the C reproducer from
syzkaller doesn't work for me, so I've not been able to test any fixes either.
It's also worth noting that cdev_put() is called from __fput(), but I think the
reference counting on the file means we're ok there.

> 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?

Will

Re: WARNING: refcount bug in cdev_get

[Will Deacon]

Hi all,

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:
> > But I thought we had a lock in play here, so why would changing this
> > actually fix anything?
> 
> I don't think the lock is always used. For example, look at chrdev_open(),
> which appears in the backtrace; the locked code is:
> 
> 	spin_lock(&cdev_lock);
> 	p = inode->i_cdev;
> 	if (!p) {
> 		struct kobject *kobj;
> 		int idx;
> 		spin_unlock(&cdev_lock);
> 		kobj = kobj_lookup(cdev_map, inode->i_rdev, &idx);
> 		if (!kobj)
> 			return -ENXIO;
> 		new = container_of(kobj, struct cdev, kobj);
> 		spin_lock(&cdev_lock);
> 		/* Check i_cdev again in case somebody beat us to it while
> 		   we dropped the lock. */
> 		p = inode->i_cdev;
> 		if (!p) {
> 			inode->i_cdev = p = new;
> 			list_add(&inode->i_devices, &p->list);
> 			new = NULL;
> 		} else if (!cdev_get(p))
> 			ret = -ENXIO;
> 	} else if (!cdev_get(p))
> 		ret = -ENXIO;
> 	spin_unlock(&cdev_lock);
> 	cdev_put(new);
> 
> So the idea is that multiple threads serialise on the 'cdev_lock' and then
> check 'inode->i_cdev' to figure out if the device has already been probed,
> taking a reference to it if it's available or probing it via kobj_lookup()
> otherwise. I think that's backwards with respect to things like cdev_put(),
> where the refcount is dropped *before* 'inode->i_cdev' is cleared to NULL.
> In which case, if a concurrent call to cdev_put() can drop the refcount
> to zero without 'cdev_lock' held, then you could get a use-after-free on
> this path thanks to a dangling pointer in 'inode->i_cdev'..
> 
> Looking slightly ahead in this same function, there are error paths which
> appear to do exactly that:
> 
> 	fops = fops_get(p->ops);
> 	if (!fops)
> 		goto out_cdev_put;
> 
> 	replace_fops(filp, fops);
> 	if (filp->f_op->open) {
> 		ret = filp->f_op->open(inode, filp);
> 		if (ret)
> 			goto out_cdev_put;
> 	}
> 
> 	return 0;
> 
>  out_cdev_put:
> 	cdev_put(p);
> 	return ret;
> 
> In which case the thread which installed 'inode->i_cdev' earlier on can
> now drop its refcount to zero without the lock held if, for example, the
> filp->f_op->open() call fails.
> 
> But note, this is purely based on code inspection -- the C reproducer from
> syzkaller doesn't work for me, so I've not been able to test any fixes either.
> It's also worth noting that cdev_put() is called from __fput(), but I think the
> reference counting on the file means we're ok there.
> 
> > 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.

Will

--->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
[   89.514943] Modules linked in:
[   89.515307] CPU: 2 PID: 6385 Comm: repro Not tainted 5.5.0-rc2+ #22
[   89.516039] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[   89.517047] RIP: 0010:refcount_warn_saturate+0x6d/0xf0
[   89.517647] Code: 05 55 9a 15 01 01 e8 9d aa c8 ff 0f 0b c3 80 3d 45 9a 15 01 00 75 ce 48 c7 c7 00 9c 62 b3 c6 08
[   89.519749] RSP: 0018:ffffb524c1b9bc70 EFLAGS: 00010282
[   89.520353] RAX: 0000000000000000 RBX: ffff9e9da1f71390 RCX: 0000000000000000
[   89.521184] RDX: ffff9e9dbbd27618 RSI: ffff9e9dbbd18798 RDI: ffff9e9dbbd18798
[   89.522020] RBP: 0000000000000000 R08: 000000000000095f R09: 0000000000000039
[   89.522854] R10: 0000000000000000 R11: ffffb524c1b9bb20 R12: ffff9e9da1e8c700
[   89.523689] R13: ffffffffb25ee8b0 R14: 0000000000000000 R15: ffff9e9da1e8c700
[   89.524512] FS:  00007f3b87d26700(0000) GS:ffff9e9dbbd00000(0000) knlGS:0000000000000000
[   89.525439] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   89.526105] CR2: 00007fc16909c000 CR3: 000000012df9c000 CR4: 00000000000006e0
[   89.526937] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   89.527759] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   89.528587] Call Trace:
[   89.528889]  kobject_get+0x5c/0x60
[   89.529290]  cdev_get+0x2b/0x60
[   89.529656]  chrdev_open+0x55/0x220
[   89.530060]  ? cdev_put.part.3+0x20/0x20
[   89.530515]  do_dentry_open+0x13a/0x390
[   89.530961]  path_openat+0x2c8/0x1470
[   89.531383]  do_filp_open+0x93/0x100
[   89.531797]  ? selinux_file_ioctl+0x17f/0x220
[   89.532297]  do_sys_open+0x186/0x220
[   89.532708]  do_syscall_64+0x48/0x150
[   89.533129]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   89.533704] RIP: 0033:0x7f3b87efcd0e
[   89.534115] Code: 89 54 24 08 e8 a3 f4 ff ff 8b 74 24 0c 48 8b 3c 24 41 89 c0 44 8b 54 24 08 b8 01 01 00 00 89 f4
[   89.536227] RSP: 002b:00007f3b87d259f0 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
[   89.537085] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f3b87efcd0e
[   89.537891] RDX: 0000000000000000 RSI: 00007f3b87d25a80 RDI: 00000000ffffff9c
[   89.538693] RBP: 00007f3b87d25e90 R08: 0000000000000000 R09: 0000000000000000
[   89.539493] R10: 0000000000000000 R11: 0000000000000293 R12: 00007ffe188f504e
[   89.540291] R13: 00007ffe188f504f R14: 00007f3b87d26700 R15: 0000000000000000
[   89.541090] ---[ end trace 24f53ca58db8180a ]---

Re: WARNING: refcount bug in cdev_get

[Greg KH]

On Wed, Dec 18, 2019 at 05:08:55PM +0000, Will Deacon wrote:
> Hi all,
> 
> 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:
> > > But I thought we had a lock in play here, so why would changing this
> > > actually fix anything?
> > 
> > I don't think the lock is always used. For example, look at chrdev_open(),
> > which appears in the backtrace; the locked code is:
> > 
> > 	spin_lock(&cdev_lock);
> > 	p = inode->i_cdev;
> > 	if (!p) {
> > 		struct kobject *kobj;
> > 		int idx;
> > 		spin_unlock(&cdev_lock);
> > 		kobj = kobj_lookup(cdev_map, inode->i_rdev, &idx);
> > 		if (!kobj)
> > 			return -ENXIO;
> > 		new = container_of(kobj, struct cdev, kobj);
> > 		spin_lock(&cdev_lock);
> > 		/* Check i_cdev again in case somebody beat us to it while
> > 		   we dropped the lock. */
> > 		p = inode->i_cdev;
> > 		if (!p) {
> > 			inode->i_cdev = p = new;
> > 			list_add(&inode->i_devices, &p->list);
> > 			new = NULL;
> > 		} else if (!cdev_get(p))
> > 			ret = -ENXIO;
> > 	} else if (!cdev_get(p))
> > 		ret = -ENXIO;
> > 	spin_unlock(&cdev_lock);
> > 	cdev_put(new);
> > 
> > So the idea is that multiple threads serialise on the 'cdev_lock' and then
> > check 'inode->i_cdev' to figure out if the device has already been probed,
> > taking a reference to it if it's available or probing it via kobj_lookup()
> > otherwise. I think that's backwards with respect to things like cdev_put(),
> > where the refcount is dropped *before* 'inode->i_cdev' is cleared to NULL.
> > In which case, if a concurrent call to cdev_put() can drop the refcount
> > to zero without 'cdev_lock' held, then you could get a use-after-free on
> > this path thanks to a dangling pointer in 'inode->i_cdev'..
> > 
> > Looking slightly ahead in this same function, there are error paths which
> > appear to do exactly that:
> > 
> > 	fops = fops_get(p->ops);
> > 	if (!fops)
> > 		goto out_cdev_put;
> > 
> > 	replace_fops(filp, fops);
> > 	if (filp->f_op->open) {
> > 		ret = filp->f_op->open(inode, filp);
> > 		if (ret)
> > 			goto out_cdev_put;
> > 	}
> > 
> > 	return 0;
> > 
> >  out_cdev_put:
> > 	cdev_put(p);
> > 	return ret;
> > 
> > In which case the thread which installed 'inode->i_cdev' earlier on can
> > now drop its refcount to zero without the lock held if, for example, the
> > filp->f_op->open() call fails.
> > 
> > But note, this is purely based on code inspection -- the C reproducer from
> > syzkaller doesn't work for me, so I've not been able to test any fixes either.
> > It's also worth noting that cdev_put() is called from __fput(), but I think the
> > reference counting on the file means we're ok there.
> > 
> > > 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.
> 
> Will
> 
> --->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
> [   89.514943] Modules linked in:
> [   89.515307] CPU: 2 PID: 6385 Comm: repro Not tainted 5.5.0-rc2+ #22
> [   89.516039] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
> [   89.517047] RIP: 0010:refcount_warn_saturate+0x6d/0xf0
> [   89.517647] Code: 05 55 9a 15 01 01 e8 9d aa c8 ff 0f 0b c3 80 3d 45 9a 15 01 00 75 ce 48 c7 c7 00 9c 62 b3 c6 08
> [   89.519749] RSP: 0018:ffffb524c1b9bc70 EFLAGS: 00010282
> [   89.520353] RAX: 0000000000000000 RBX: ffff9e9da1f71390 RCX: 0000000000000000
> [   89.521184] RDX: ffff9e9dbbd27618 RSI: ffff9e9dbbd18798 RDI: ffff9e9dbbd18798
> [   89.522020] RBP: 0000000000000000 R08: 000000000000095f R09: 0000000000000039
> [   89.522854] R10: 0000000000000000 R11: ffffb524c1b9bb20 R12: ffff9e9da1e8c700
> [   89.523689] R13: ffffffffb25ee8b0 R14: 0000000000000000 R15: ffff9e9da1e8c700
> [   89.524512] FS:  00007f3b87d26700(0000) GS:ffff9e9dbbd00000(0000) knlGS:0000000000000000
> [   89.525439] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   89.526105] CR2: 00007fc16909c000 CR3: 000000012df9c000 CR4: 00000000000006e0
> [   89.526937] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   89.527759] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [   89.528587] Call Trace:
> [   89.528889]  kobject_get+0x5c/0x60
> [   89.529290]  cdev_get+0x2b/0x60
> [   89.529656]  chrdev_open+0x55/0x220
> [   89.530060]  ? cdev_put.part.3+0x20/0x20
> [   89.530515]  do_dentry_open+0x13a/0x390
> [   89.530961]  path_openat+0x2c8/0x1470
> [   89.531383]  do_filp_open+0x93/0x100
> [   89.531797]  ? selinux_file_ioctl+0x17f/0x220
> [   89.532297]  do_sys_open+0x186/0x220
> [   89.532708]  do_syscall_64+0x48/0x150
> [   89.533129]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   89.533704] RIP: 0033:0x7f3b87efcd0e
> [   89.534115] Code: 89 54 24 08 e8 a3 f4 ff ff 8b 74 24 0c 48 8b 3c 24 41 89 c0 44 8b 54 24 08 b8 01 01 00 00 89 f4
> [   89.536227] RSP: 002b:00007f3b87d259f0 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
> [   89.537085] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f3b87efcd0e
> [   89.537891] RDX: 0000000000000000 RSI: 00007f3b87d25a80 RDI: 00000000ffffff9c
> [   89.538693] RBP: 00007f3b87d25e90 R08: 0000000000000000 R09: 0000000000000000
> [   89.539493] R10: 0000000000000000 R11: 0000000000000293 R12: 00007ffe188f504e
> [   89.540291] R13: 00007ffe188f504f R14: 00007f3b87d26700 R15: 0000000000000000
> [   89.541090] ---[ end trace 24f53ca58db8180a ]---

No hint as to _where_ you put the mdelay()?  :)

Re: WARNING: refcount bug in cdev_get

[Will Deacon]

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);
 }

Re: WARNING: refcount bug in cdev_get

[Prateek Sood]

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