Messages in this thread | | | Date | Sun, 23 Oct 2022 07:26:14 -0700 | From | Hyunwoo Kim <> | Subject | Re: [PATCH] char: xillybus: Fix use-after-free in xillyusb_open() |
| |
On Sun, Oct 23, 2022 at 05:19:40PM +0300, Eli Billauer wrote: > Hello, Hyunwoo. > > > A race condition may occur if the user physically removes > > the USB device while calling open() for this device node. > > > > This is a race condition between the xillyusb_open() function and > > the xillyusb_disconnect() function, which may eventually result in UAF. > > Thanks a lot for pointing that out. In fact, this reveals two problems in > the existing code: > > (1) unit->private_data is accessed after the mutex has been released in > xillybus_find_inode(), so there's no guarantee that it will be valid. This > is what the test caught. This can however be fixed just by moving the > release of the lock a few rows down. > > (2) xillyusb_open() accesses @xdev without ensuring that it won't get freed. > > Both of these two issues have a negligible probability of causing a visible > problem, but this must be fixed, of course. > > > > > So, add a mutex to the xillyusb_open() and xillyusb_disconnect() > > functions to avoid race contidion. > > I'm not very fond of this solution, partially because this mutex protects > code and not data (There's this "Lock data, not code" rule, see [1]). Also, > xillyusb_disconnect() can take a significant time to run, during which > xillybus_open() for another (unrelated and still connected) XillyUSB device > has to wait. I guess this demonstrates why protecting code with a mutex is > considered bad practice. > > Besides, there are already three mechanisms in place for preventing > premature release of memory: > > (1) @unit_mutex in xillybus_class.c, which protects @unit_list. > (2) @kref inside struct xillyusb_dev (xillyusb.c), which protects the > structure it resides in. > (3) @error inside struct xillyusb_dev, which prevents xillybus_open() from > opening a file that belongs to a device that is about to be released. > > It's now apparent that they're not working well enough. Rather than adding > another mutex, the existing mechanisms should be fixed. Would you like to > do this, or should I?
Thanks for the detailed feedback. It's probably better for you to work on it.
Regards, Hyunwoo Kim.
| |