| Message ID | 20191125125342.6189-1-vdronov@redhat.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | fs: fix use-after-free in __fput() when a chardev is removed but a file is still open | expand |
On Mon, Nov 25, 2019 at 01:53:42PM +0100, Vladis Dronov wrote: > 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); } > 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: And what's to prevent rmmod coming and freeing ->release code right as you are executing it?
On Sun, Dec 08, 2019 at 07:49:07PM +0000, Al Viro wrote: > On Mon, Nov 25, 2019 at 01:53:42PM +0100, Vladis Dronov wrote: > > 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); } > > > 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: > > And what's to prevent rmmod coming and freeing ->release code right as you > are executing it? FWIW, the bug here seems to be that the lifetime rules of cdev are fucked - if it can get freed while its ->kobj is still alive, we have something very wrong there. IOW, you have ptp lifetime controlled by *TWO* refcounts - that of clk and that of of cdev->kobj. That's doesn't work. Replace that kfree() with dropping a kobject reference, perhaps, so that freeing would've been done by its release callback?
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)
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@redhat.com> Analyzed-by: Vern Lovejoy <vlovejoy@redhat.com> Signed-off-by: Vladis Dronov <vdronov@redhat.com> --- fs/file_table.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)