Message-ID: <2025112725-comrade-willfully-67f1@gregkh>
Date: Thu, 27 Nov 2025 10:54:43 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Zhengqiao Xia <xiazhengqiao@...qin.corp-partner.google.com>
Cc: rafael@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] device_core: check null pointer

On Tue, Nov 18, 2025 at 03:31:03PM +0800, Zhengqiao Xia wrote:
> Greg KH <gregkh@...uxfoundation.org> 于2025年11月17日周一 20:37写道：
> 
> >
> > On Mon, Nov 17, 2025 at 10:55:05AM +0800, Zhengqiao Xia wrote:
> > > Hi Greg,
> > >
> > > Based on my analysis,
> >
> > Please do not top-post :(
> 
> got it
> 
> >
> > >
> > > usb_new_device ->  device_add -> device_private_init
> > >                                                     ->
> > > bus_probe_device -> usb_generic_driver_probe  ->
> > > usb_set_configuration  -> "add each interface"  ->
> > > device_add(&intf->dev);
> > >
> > >
> > >                                     --> if of_node status is disabled
> > > -> of_device_is_available is false -> not running  device_add ->
> >
> > Why is of_node related to USB interfaces?
> 
> you can see usb/core/message.c
> ```
> for (i = 0; i < nintf; ++i) {
> struct usb_interface *intf = cp->interface[i];
> 
> dump_usb_intf_info(intf);
> 
>    if (intf->dev.of_node &&
>                !of_device_is_available(intf->dev.of_node)) {
>           dev_info(&dev->dev, "skipping disabled interface %d\n",
>           intf->cur_altsetting->desc.bInterfaceNumber);
>           continue;
>      }
>      ...
>      ret = device_add(&intf->dev);

Yes, device_add() is skipped, so that means we should not ever be seeing
this device in the device tree anywhere, so that the driver core
shouldn't be seeing it anywhere else.

This device_add() call is not made, so what is attempting to register
this "disabled" interface?

And the more and more I see this "tacked on" ability to disable
interfaces based on firmware node, the more I dislike it (we are having
other discussions on the linux-usb list about it.)

So perhaps we just missed a path where the interface is not really known
to be disabled and we try to do something with it later on?

>      ...
> }
> ```
> 
> >
> > > interface dev->p is null
> > > usbdev_ioctl -> proc_ioctl -> device_attach -> __device_attach ->
> > > device_attach ->  if (dev->p->dead)
> > >
> > > Since dev->p has not been created at this time, dev->p is NULL,  So I
> > > think we should add a check here.
> > >
> > > Here is my current analysis, but there may be some misunderstandings
> > > or mistakes in it. Could you please help clarify my confusion?
> >
> > As this code is very old, USB was one of the first subsystems to get
> > driver core support decades ago, what has changed recently to require
> > this check?  Have you seen crashes here?  If so, what is the traceback
> > for them and with what hardware/drivers?
> 
> ```
> [   93.530302] usb 4-1: USB disconnect, device number 2
> [   93.535637] cdc_mbim 4-1:1.0 wwan0: unregister 'cdc_mbim'
> usb-11260000.usb-1, CDC MBIM
> [   93.564041] option1 ttyUSB0: GSM modem (1-port) converter now
> disconnected from ttyUSB0
> [   93.574619] option 4-1:1.2: device disconnected
> [   93.948062] usb 4-1: new high-speed USB device number 3 using xhci-mtk
> [   94.084037] usb 4-1: New USB device found, idVendor=05c6,
> idProduct=9008, bcdDevice= 0.00
> [   94.092275] usb 4-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> [   94.099455] usb 4-1: Product: QUSB__BULK
> [   94.103407] usb 4-1: Manufacturer: Qualcomm CDMA Technologies MSM
> [   94.114020] [USB-ATTACH] dev=4-1 dev->p=ffffff811faec600
> [   94.122394] ===== USB Interface Dump =====
> [   94.122394] dev name:        4-1:1.0
> [   94.122394] dev of_node:     hub
> [   94.122394] dev->p:          0000000000000000 (NULL)
> [   94.122394] parent name:     4-1
> [   94.122394] parent of_node:  hub
> [   94.122394] ==============================
> [   94.148881] usb 4-1: skipping disabled interface 0
> 
> --> 4-1:1.0 doesn't run device_add

Good.

> [   94.161987] [USB-ATTACH] dev=4-1:1.0 dev->p=0000000000000000
> 
> -->  usbdev_ioctl -> __device_attach -->

Wait, why are you attempting to talk to this interface through usbfs?

Maybe that's the path here, we need to disable that "for real" and
prevent usbfs from accessing the interface as well.

What userspace tool attempts to call this "by default" or are you just
running fuzzing tools?

Again, I think this needs to be fixed properly, not papered over in the
driver core.  So let's either rip the ability to disable interfaces out
of the usb core, or fix up where we missed that a device was not
registered.

thanks,

greg k-h

Powered by blists - more mailing lists