netdev - [PATCH] fs: fix use-after-free in __fput() when a chardev is removed but a file is still open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Nov 2019 13:53:42 +0100
From:   Vladis Dronov <vdronov@...hat.com>
To:     Alexander Viro <viro@...iv.linux.org.uk>,
        Richard Cochran <richardcochran@...il.com>,
        linux-fsdevel@...r.kernel.org
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        vdronov@...hat.com
Subject: [PATCH] fs: fix use-after-free in __fput() when a chardev is removed but a file is still open

In a case when a chardev file (like /dev/ptp0) is open but an underlying
device is removed, closing this file leads to a use-after-free. This
reproduces easily in a KVM virtual machine:

# cat openptp0.c
int main() { ... fp = fopen("/dev/ptp0", "r"); ... sleep(10); }

# uname -r
5.4.0-219d5433
# cat /proc/cmdline
... slub_debug=FZP
# modprobe ptp_kvm
# ./openptp0 &
[1] 670
opened /dev/ptp0, sleeping 10s...
# rmmod ptp_kvm
# ls /dev/ptp*
ls: cannot access '/dev/ptp*': No such file or directory
# ...woken up
[  102.375849] general protection fault: 0000 [#1] SMP
[  102.377372] CPU: 1 PID: 670 Comm: openptp0 Not tainted 5.4.0-219d5433 #1
[  102.379163] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), ...
[  102.381129] RIP: 0010:module_put.part.0+0x7/0x80
[  102.383019] RSP: 0018:ffff9ba440687e00 EFLAGS: 00010202
[  102.383451] RAX: 0000000000002000 RBX: 6b6b6b6b6b6b6b6b RCX: ffff91e736800ad0
[  102.384030] RDX: ffffcf6408bc2808 RSI: 0000000000000247 RDI: 6b6b6b6b6b6b6b6b
[  102.386032] ...                                              ^^^ a slub poison
[  102.389866] Call Trace:
[  102.390086]  __fput+0x21f/0x240
[  102.390363]  task_work_run+0x79/0x90
[  102.390671]  do_exit+0x2c9/0xad0
[  102.390931]  ? vfs_write+0x16a/0x190
[  102.391241]  do_group_exit+0x35/0x90
[  102.391549]  __x64_sys_exit_group+0xf/0x10
[  102.391898]  do_syscall_64+0x3d/0x110
[  102.392240]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  102.392695] RIP: 0033:0x7f0fa7016246
[  102.396615] ...
[  102.397225] Modules linked in: [last unloaded: ptp_kvm]
[  102.410323] Fixing recursive fault but reboot is needed!

This happens in:

static void __fput(struct file *file)
{   ...
    if (file->f_op->release)
        file->f_op->release(inode, file); <<< cdev is kfree'd here
    if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
             !(mode & FMODE_PATH))) {
        cdev_put(inode->i_cdev); <<< cdev fields are accessed here

because of:

__fput()
  posix_clock_release()
    kref_put(&clk->kref, delete_clock) <<< the last reference
      delete_clock()
        delete_ptp_clock()
          kfree(ptp) <<< cdev is embedded in ptp
  cdev_put
    module_put(p->owner) <<< *p is kfree'd

The fix is to call cdev_put() before file->f_op->release(). This fix the
class of bugs when a chardev device is removed when its file is open, for
example:

# lspci
00:09.0 System peripheral: Intel Corporation 6300ESB Watchdog Timer
# ./openwdog0 &
[1] 672
opened /dev/watchdog0, sleeping 10s...
# echo 1 > /sys/devices/pci0000:00/0000:00:09.0/remove
# ls /dev/watch*
ls: cannot access '/dev/watch*': No such file or directory
# ...woken up
[   63.500271] general protection fault: 0000 [#1] SMP
[   63.501757] CPU: 1 PID: 672 Comm: openwdog0 Not tainted 5.4.0-219d5433 #4
[   63.503605] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), ...
[   63.507064] RIP: 0010:module_put.part.0+0x7/0x80
[   63.513841] RSP: 0018:ffffb96b00667e00 EFLAGS: 00010202
[   63.515376] RAX: 0000000000002000 RBX: 6b6b6b6b6b6b6b6b RCX: 0000000000150013
[   63.517478] RDX: 0000000000000246 RSI: 0000000000000000 RDI: 6b6b6b6b6b6b6b6b

Analyzed-by: Stephen Johnston <sjohnsto@...hat.com>
Analyzed-by: Vern Lovejoy <vlovejoy@...hat.com>
Signed-off-by: Vladis Dronov <vdronov@...hat.com>
---
 fs/file_table.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 30d55c9a1744..21ba35024950 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -276,12 +276,12 @@ static void __fput(struct file *file)
 		if (file->f_op->fasync)
 			file->f_op->fasync(-1, file, 0);
 	}
-	if (file->f_op->release)
-		file->f_op->release(inode, file);
 	if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
 		     !(mode & FMODE_PATH))) {
 		cdev_put(inode->i_cdev);
 	}
+	if (file->f_op->release)
+		file->f_op->release(inode, file);
 	fops_put(file->f_op);
 	put_pid(file->f_owner.pid);
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
-- 
2.20.1