linux-kernel - Re: [PATCH] char: xillybus: Fix use-after-free in xillyusb

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221023142614.GA385798@ubuntu>
Date:   Sun, 23 Oct 2022 07:26:14 -0700
From:   Hyunwoo Kim <imv4bel@...il.com>
To:     Eli Billauer <eli.billauer@...il.com>
Cc:     linux-kernel@...r.kernel.org, arnd@...db.de,
        gregkh@...uxfoundation.org, imv4bel@...il.com
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.