[Devel] [PATCH RHEL8 COMMIT] chardev: Avoid potential use-after-free in 'chrdev_open()'

[Konstantin Khorenko]

The commit is pushed to "branch-rh8-4.18.0-193.6.3.vz8.4.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh8-4.18.0-193.6.3.vz8.4.4
------>
commit 7baa3b78ee9d349d33e6c125a61ab0e3adca9072
Author: Will Deacon <will at kernel.org>
Date:   Fri Jul 24 19:49:36 2020 +0300

    chardev: Avoid potential use-after-free in 'chrdev_open()'
    
    [ML commit 68faa679b8be1a74e6663c21c3a9d25d32f1c079]
    
    'chrdev_open()' calls 'cdev_get()' to obtain a reference to the
    'struct cdev *' stashed in the 'i_cdev' field of the target inode
    structure. If the pointer is NULL, then it is initialised lazily by
    looking up the kobject in the 'cdev_map' and so the whole procedure is
    protected by the 'cdev_lock' spinlock to serialise initialisation of
    the shared pointer.
    
    Unfortunately, it is possible for the initialising thread to fail *after*
    installing the new pointer, for example if the subsequent '->open()' call
    on the file fails. In this case, 'cdev_put()' is called, the reference
    count on the kobject is dropped and, if nobody else has taken a reference,
    the release function is called which finally clears 'inode->i_cdev' from
    'cdev_purge()' before potentially freeing the object. The problem here
    is that a racing thread can happily take the 'cdev_lock' and see the
    non-NULL pointer in the inode, which can result in a refcount increment
    from zero and a warning:
    
      |  ------------[ cut here ]------------
      |  refcount_t: addition on 0; use-after-free.
      |  WARNING: CPU: 2 PID: 6385 at lib/refcount.c:25 refcount_warn_saturate+0x6d/0xf0
      |  Modules linked in:
      |  CPU: 2 PID: 6385 Comm: repro Not tainted 5.5.0-rc2+ #22
      |  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
      |  RIP: 0010:refcount_warn_saturate+0x6d/0xf0
      |  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
      |  RSP: 0018:ffffb524c1b9bc70 EFLAGS: 00010282
      |  RAX: 0000000000000000 RBX: ffff9e9da1f71390 RCX: 0000000000000000
      |  RDX: ffff9e9dbbd27618 RSI: ffff9e9dbbd18798 RDI: ffff9e9dbbd18798
      |  RBP: 0000000000000000 R08: 000000000000095f R09: 0000000000000039
      |  R10: 0000000000000000 R11: ffffb524c1b9bb20 R12: ffff9e9da1e8c700
      |  R13: ffffffffb25ee8b0 R14: 0000000000000000 R15: ffff9e9da1e8c700
      |  FS:  00007f3b87d26700(0000) GS:ffff9e9dbbd00000(0000) knlGS:0000000000000000
      |  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
      |  CR2: 00007fc16909c000 CR3: 000000012df9c000 CR4: 00000000000006e0
      |  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
      |  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
      |  Call Trace:
      |   kobject_get+0x5c/0x60
      |   cdev_get+0x2b/0x60
      |   chrdev_open+0x55/0x220
      |   ? cdev_put.part.3+0x20/0x20
      |   do_dentry_open+0x13a/0x390
      |   path_openat+0x2c8/0x1470
      |   do_filp_open+0x93/0x100
      |   ? selinux_file_ioctl+0x17f/0x220
      |   do_sys_open+0x186/0x220
      |   do_syscall_64+0x48/0x150
      |   entry_SYSCALL_64_after_hwframe+0x44/0xa9
      |  RIP: 0033:0x7f3b87efcd0e
      |  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
      |  RSP: 002b:00007f3b87d259f0 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
      |  RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f3b87efcd0e
      |  RDX: 0000000000000000 RSI: 00007f3b87d25a80 RDI: 00000000ffffff9c
      |  RBP: 00007f3b87d25e90 R08: 0000000000000000 R09: 0000000000000000
      |  R10: 0000000000000000 R11: 0000000000000293 R12: 00007ffe188f504e
      |  R13: 00007ffe188f504f R14: 00007f3b87d26700 R15: 0000000000000000
      |  ---[ end trace 24f53ca58db8180a ]---
    
    Since 'cdev_get()' can already fail to obtain a reference, simply move
    it over to use 'kobject_get_unless_zero()' instead of 'kobject_get()',
    which will cause the racing thread to return -ENXIO if the initialising
    thread fails unexpectedly.
    
    Cc: Hillf Danton <hdanton at sina.com>
    Cc: Andrew Morton <akpm at linux-foundation.org>
    Cc: Al Viro <viro at zeniv.linux.org.uk>
    Reported-by: syzbot+82defefbbd8527e1c2cb at syzkaller.appspotmail.com
    Signed-off-by: Will Deacon <will at kernel.org>
    Cc: stable <stable at vger.kernel.org>
    Link: https://lore.kernel.org/r/20191219120203.32691-1-will@kernel.org
    Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
    
    CVE-2020-0305
    https://jira.sw.ru/browse/PSBM-106236
    
    Signed-off-by: Evgenii Shatokhin <eshatokhin at virtuozzo.com>
---
 fs/char_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 1c4f1db545f9..b6b83faa3ffe 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -359,7 +359,7 @@ static struct kobject *cdev_get(struct cdev *p)
 
 	if (owner && !try_module_get(owner))
 		return NULL;
-	kobj = kobject_get(&p->kobj);
+	kobj = kobject_get_unless_zero(&p->kobj);
 	if (!kobj)
 		module_put(owner);
 	return kobj;