debugfs vs. device removal

debugfs vs. device removal

[Omar Sandoval]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

Hi,

In the block layer, we abuse sysfs to export some per-device debugging
information. I was looking into moving this to debugfs, but I realized
that debugfs doesn't have a mechanism to ensure that a file associated
with a device is safe to use when the device is removed. 

At a quick glance, HID has some per-device information in debugfs.
However, I don't see any sort of protection against a device being
removed. I was easily able to trigger an oops by reading from
/sys/kernel/debug/hid/*/rdesc in a loop and removing the USB device
(trace attached).

How can I safely export per-device debugging information to debugfs?

Thanks,
Omar

[-- Attachment #2: oops.txt --]
[-- Type: text/plain, Size: 3192 bytes --]

[ 1409.427480] general protection fault: 0000 [#1] PREEMPT SMP
[ 1409.428078] Modules linked in: hid_generic usbhid hid cfg80211 rfkill joydev input_leds led_class mousedev ppdev uhci_hcd evdev mac_hid ehci_pci ehci_hcd psmouse serio_raw atkbd usbcore libps2 pcspkr parport_pc parport i2c_piix4 i6300esb acpi_cpufreq usb_common intel_agp tpm_tis intel_gtt tpm_tis_core qemu_fw_cfg tpm button sch_fq_codel ip_tables x_tables ext4 crc16 jbd2 fscrypto mbcache ata_generic pata_acpi virtio_blk virtio_net ata_piix i8042 libata virtio_pci serio virtio_ring floppy virtio scsi_mod
[ 1409.429869] CPU: 2 PID: 25689 Comm: cat Not tainted 4.8.13-1-ARCH #1
[ 1409.429869] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.1-20161122_114906-anatol 04/01/2014
[ 1409.429869] task: ffff88007ba25b00 task.stack: ffff8800780d8000
[ 1409.429869] RIP: 0010:[<ffffffffa035713c>]  [<ffffffffa035713c>] hid_dump_field+0x1c/0x5e0 [hid]
[ 1409.429869] RSP: 0018:ffff8800780dbc98  EFLAGS: 00010292
[ 1409.429869] RAX: 0000000000000000 RBX: ffff8800374e6b40 RCX: 0000000000000002
[ 1409.429869] RDX: ffff8800374e6b40 RSI: 0000000000000006 RDI: 7273752f00000000
[ 1409.429869] RBP: ffff8800780dbcd8 R08: 0000000000002000 R09: 000000000000000a
[ 1409.429869] R10: ffff880037a65974 R11: ffff880037a6597f R12: ffff8800374e6b40
[ 1409.429869] R13: 7273752f00000000 R14: ffff8800780f7000 R15: ffffffffa036251d
[ 1409.429869] FS:  00007f9824c49480(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
[ 1409.429869] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1409.429869] CR2: 000055e0a7b014f7 CR3: 000000007800e000 CR4: 00000000000006e0
[ 1409.429869] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1409.429869] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1409.429869] Stack:
[ 1409.429869]  ffff8800780dbca8 0000000017a5ac78 000000067b9e1800 0000000000000001
[ 1409.429869]  ffff8800374e6b40 ffff8800780f7020 ffff8800780f7000 ffffffffa036251d
[ 1409.429869]  ffff8800780dbd28 ffffffffa0357804 0000000000000000 ffffffffa03626e4
[ 1409.429869] Call Trace:
[ 1409.429869]  [<ffffffffa0357804>] hid_dump_device+0x104/0x160 [hid]
[ 1409.429869]  [<ffffffffa03578dd>] hid_debug_rdesc_show+0x7d/0x1d0 [hid]
[ 1409.429869]  [<ffffffff8122d940>] ? seq_buf_alloc+0x20/0x40
[ 1409.429869]  [<ffffffff8122dd42>] seq_read+0x102/0x3c0
[ 1409.429869]  [<ffffffff81299460>] full_proxy_read+0x60/0xa0
[ 1409.429869]  [<ffffffff81208667>] __vfs_read+0x37/0x130
[ 1409.429869]  [<ffffffff812b5013>] ? security_file_permission+0xa3/0xc0
[ 1409.429869]  [<ffffffff81209416>] vfs_read+0x96/0x130
[ 1409.429869]  [<ffffffff8120a925>] SyS_read+0x55/0xc0
[ 1409.429869]  [<ffffffff81067747>] ? trace_do_page_fault+0x37/0xf0
[ 1409.429869]  [<ffffffff815f8032>] entry_SYSCALL_64_fastpath+0x1a/0xa4
[ 1409.429869] Code: 36 a0 48 63 f0 e8 85 27 fb e0 e9 0d fe ff ff 0f 1f 44 00 00 55 48 89 e5 41 5741 56 41 55 41 54 49 89 fd 53 48 89 d3 48 83 ec 18 <44> 8b 07 89 75 d4 45 85 c0 0f 85 69 05 00 00 41 8b 7d 04 85 ff
[ 1409.429869] RIP  [<ffffffffa035713c>] hid_dump_field+0x1c/0x5e0 [hid]
[ 1409.429869]  RSP <ffff8800780dbc98>
[ 1409.473184] ---[ end trace 451c7935d39fca87 ]---

Re: debugfs vs. device removal

[Greg Kroah-Hartman]

On Thu, Jan 19, 2017 at 07:48:41AM -0800, Omar Sandoval wrote:
> Hi,
> 
> In the block layer, we abuse sysfs to export some per-device debugging
> information. I was looking into moving this to debugfs, but I realized
> that debugfs doesn't have a mechanism to ensure that a file associated
> with a device is safe to use when the device is removed. 

What do you mean by "safe"?  The race conditions where you remove a file
and still have it open should all now be resolved in 4.8 and 4.9, di dwe
miss something?

> At a quick glance, HID has some per-device information in debugfs.
> However, I don't see any sort of protection against a device being
> removed. I was easily able to trigger an oops by reading from
> /sys/kernel/debug/hid/*/rdesc in a loop and removing the USB device
> (trace attached).
> 
> How can I safely export per-device debugging information to debugfs?

Try 4.9 and see if you can still reproduce this, it should be fixed.  If
not, we might have forgotten to include the .owner field for the hid
debugfs file...

thanks,

greg k-h

Re: debugfs vs. device removal

[Jiri Kosina]

On Thu, 19 Jan 2017, Greg Kroah-Hartman wrote:

> > In the block layer, we abuse sysfs to export some per-device debugging
> > information. I was looking into moving this to debugfs, but I realized
> > that debugfs doesn't have a mechanism to ensure that a file associated
> > with a device is safe to use when the device is removed. 
> 
> What do you mean by "safe"?  The race conditions where you remove a file
> and still have it open should all now be resolved in 4.8 and 4.9, di dwe
> miss something?

This is something else -- Omar is right, hid-debugfs interface is buggy. 
It basically doesn't synchronize the data dumping with device removal, so 
if device is removed and deallocated and the race is hit, it tries to 
dereference struct hid_device which has already been freed.

I'll look into fixing this later today or tomorrow. Basically we'd need to 
synchronize between hid_remove_device() and anything in hid-debug and 
whenever removal is pending, not to try to get any data out of it any more 
and bail immediately. Something like rwlock (debugfs being the reader and 
device removal being the writer) should work.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: debugfs vs. device removal

[Omar Sandoval]

On Thu, Jan 19, 2017 at 05:03:48PM +0100, Jiri Kosina wrote:
> On Thu, 19 Jan 2017, Greg Kroah-Hartman wrote:
> 
> > > In the block layer, we abuse sysfs to export some per-device debugging
> > > information. I was looking into moving this to debugfs, but I realized
> > > that debugfs doesn't have a mechanism to ensure that a file associated
> > > with a device is safe to use when the device is removed. 
> > 
> > What do you mean by "safe"?  The race conditions where you remove a file
> > and still have it open should all now be resolved in 4.8 and 4.9, di dwe
> > miss something?
> 
> This is something else -- Omar is right, hid-debugfs interface is buggy. 
> It basically doesn't synchronize the data dumping with device removal, so 
> if device is removed and deallocated and the race is hit, it tries to 
> dereference struct hid_device which has already been freed.

Yup, I'm talking about the case where I create a debugfs file and the
data pointer is, say, a struct request_queue. If userspace calls open()
on a debugfs file, then the device goes away, the struct request_queue
is going to get freed and read() will blow up.

If we're talking about objects with a struct kobject (like struct
request_queue), can we just grab an extra reference in open() and drop
it in release()? This allows userspace to keep stuff pinned
indefinitely, but debugfs is root-only and the use-case is usually just
`cat`.

> I'll look into fixing this later today or tomorrow. Basically we'd need to 
> synchronize between hid_remove_device() and anything in hid-debug and 
> whenever removal is pending, not to try to get any data out of it any more 
> and bail immediately. Something like rwlock (debugfs being the reader and 
> device removal being the writer) should work.
> 
> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

Re: debugfs vs. device removal

[Greg Kroah-Hartman]

On Thu, Jan 19, 2017 at 09:33:50AM -0800, Omar Sandoval wrote:
> On Thu, Jan 19, 2017 at 05:03:48PM +0100, Jiri Kosina wrote:
> > On Thu, 19 Jan 2017, Greg Kroah-Hartman wrote:
> > 
> > > > In the block layer, we abuse sysfs to export some per-device debugging
> > > > information. I was looking into moving this to debugfs, but I realized
> > > > that debugfs doesn't have a mechanism to ensure that a file associated
> > > > with a device is safe to use when the device is removed. 
> > > 
> > > What do you mean by "safe"?  The race conditions where you remove a file
> > > and still have it open should all now be resolved in 4.8 and 4.9, di dwe
> > > miss something?
> > 
> > This is something else -- Omar is right, hid-debugfs interface is buggy. 
> > It basically doesn't synchronize the data dumping with device removal, so 
> > if device is removed and deallocated and the race is hit, it tries to 
> > dereference struct hid_device which has already been freed.
> 
> Yup, I'm talking about the case where I create a debugfs file and the
> data pointer is, say, a struct request_queue. If userspace calls open()
> on a debugfs file, then the device goes away, the struct request_queue
> is going to get freed and read() will blow up.
> 
> If we're talking about objects with a struct kobject (like struct
> request_queue), can we just grab an extra reference in open() and drop
> it in release()? This allows userspace to keep stuff pinned
> indefinitely, but debugfs is root-only and the use-case is usually just
> `cat`.

Again, debugfs got a bunch of changes in the 4.8 and 4.9 timeframe to
resolve this issue.  Try it and see with just a "normal" debugfs file
and see how it works.

thanks,

greg k-h

Re: debugfs vs. device removal

[Omar Sandoval]

On Thu, Jan 19, 2017 at 07:03:52PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 19, 2017 at 09:33:50AM -0800, Omar Sandoval wrote:
> > On Thu, Jan 19, 2017 at 05:03:48PM +0100, Jiri Kosina wrote:
> > > On Thu, 19 Jan 2017, Greg Kroah-Hartman wrote:
> > > 
> > > > > In the block layer, we abuse sysfs to export some per-device debugging
> > > > > information. I was looking into moving this to debugfs, but I realized
> > > > > that debugfs doesn't have a mechanism to ensure that a file associated
> > > > > with a device is safe to use when the device is removed. 
> > > > 
> > > > What do you mean by "safe"?  The race conditions where you remove a file
> > > > and still have it open should all now be resolved in 4.8 and 4.9, di dwe
> > > > miss something?
> > > 
> > > This is something else -- Omar is right, hid-debugfs interface is buggy. 
> > > It basically doesn't synchronize the data dumping with device removal, so 
> > > if device is removed and deallocated and the race is hit, it tries to 
> > > dereference struct hid_device which has already been freed.
> > 
> > Yup, I'm talking about the case where I create a debugfs file and the
> > data pointer is, say, a struct request_queue. If userspace calls open()
> > on a debugfs file, then the device goes away, the struct request_queue
> > is going to get freed and read() will blow up.
> > 
> > If we're talking about objects with a struct kobject (like struct
> > request_queue), can we just grab an extra reference in open() and drop
> > it in release()? This allows userspace to keep stuff pinned
> > indefinitely, but debugfs is root-only and the use-case is usually just
> > `cat`.
> 
> Again, debugfs got a bunch of changes in the 4.8 and 4.9 timeframe to
> resolve this issue.  Try it and see with just a "normal" debugfs file
> and see how it works.

The change in this area that I see is 49d200deaa68 ("debugfs: prevent
access to removed files' private data"). That went in for 4.7. I'm
pretty confused now since I can't reproduce the oops anymore on either
4.8 or 4.10-rc4. If I see it again I'll be sure to report it, but it
seems like debugfs should just work for what I need. Thanks for the
help, Greg.