Message-ID: <2025112721-savanna-overlabor-1269@gregkh>
Date: Thu, 27 Nov 2025 12:51:17 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Zhengqiao Xia <xiazhengqiao@...qin.corp-partner.google.com>
Cc: treapking@...omium.org, rafael@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] device_core: check null pointer

On Thu, Nov 27, 2025 at 07:37:34PM +0800, Zhengqiao Xia wrote:
> Greg KH <gregkh@...uxfoundation.org> 于2025年11月27日周四 17:54写道：
> >
> > 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?
> 
> When LTE enters QUSB__BULK mode, it indicates that the
> userspace(modemfwd) needs to update the firmware via usbfs.
> Therefore, this device cannot be disabled.

Then this should not be disabled in the firmware, please fix your dt.

But again, let's fix the real problem here.

> > 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.
> 
> this is why I submit a new patch
> https://lore.kernel.org/all/CAEXTbpeN0Mk+Y-UeV79JzE547UCa_Fhh7T75L+2mhoSq3ark8g@mail.gmail.com/

Does that really solve this problem?  How?

confused,

greg k-h

Powered by blists - more mailing lists