Re: [PATCH 06/14] xhci: dbc: lock the minor IDR on registration failure

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Oct 09, 2026 at 02:16:44PM +0300, Mathias Nyman wrote:
> On 10/9/26 13:52, Greg KH wrote:
> > Resending with the proper people in cc...
> > 
> > On Fri, Oct 09, 2026 at 12:51:41PM +0200, Greg KH wrote:
> > > On Fri, Oct 09, 2026 at 10:13:10AM +0000, sashiko-bot@kernel.org wrote:
> > > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > > - [High] usb: xhci: dbc: UAF due to premature IDR exposure and missing TTY hangup on error path
> > > > --
> > > > 
> > > > commit 99c18a1b8449e8389ccac896ceadcc979dbd6a57
> > > > Author: Sang-Hoon Choi <csh0052@gmail.com>
> > > > 
> > > > xhci: dbc: lock the minor IDR on registration failure
> > > > 
> > > > This commit fixes a race condition during DbC TTY registration failure
> > > > by adding mutex protection around the removal of the minor IDR entry.
> > > > 
> > > > > diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
> > > > > index 3d51e8d82659d..2249cc16800cc 100644
> > > > > --- a/drivers/usb/host/xhci-dbgtty.c
> > > > > +++ b/drivers/usb/host/xhci-dbgtty.c
> > > > > @@ -535,7 +535,9 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc)
> > > > >   err_free_fifo:
> > > > >   	kfifo_free(&port->port.xmit_fifo);
> > > > >   err_exit_port:
> > > > > +	mutex_lock(&dbc_tty_minors_lock);
> > > > >   	idr_remove(&dbc_tty_minors, port->minor);
> > > > > +	mutex_unlock(&dbc_tty_minors_lock);
> > > > >   err_idr:
> > > > >   	xhci_dbc_tty_exit_port(port);
> > > > 
> > > > [Severity: High]
> > > > Does this error path leave concurrent TTY sessions pointing to freed memory?
> > > > 
> > > > Earlier in xhci_dbc_tty_register_device(), the port is added to the globally
> > > > visible dbc_tty_minors IDR before internal structures are fully allocated:
> > > > 
> > > > 	mutex_lock(&dbc_tty_minors_lock);
> > > > 	port->minor = idr_alloc(&dbc_tty_minors, port, 0, 64, GFP_KERNEL);
> > > > 	mutex_unlock(&dbc_tty_minors_lock);
> > > > 
> > > > A concurrent open() on the device node routes to dbc_tty_install(), which
> > > > retrieves the port and installs the TTY without verifying port->registered:
> > > 
> > > I don't think we need to worry about "concurrent open()" for this as
> > > that's not a real use case that I know of and this _should_ be root only
> > > accessable, right?
> > > 
> > > Or can normal users open this device node?
> > > 
> No risk of concurrent open as the tty device node is not yet created in this error
> path.
> 
> This error path is taken if tty_port_register_device() fails, of some other failure
> before it

Ah, yeah, no need to worry about this "issue" then.

thanks,

greg k-h