Message-ID: <CADYyEwQjkTNODMUM86iqvBnc0BQnO=z1D44Tg3GMwR-_2smqbA@mail.gmail.com>
Date: Thu, 27 Nov 2025 19:37:34 +0800
From: Zhengqiao Xia <xiazhengqiao@...qin.corp-partner.google.com>
To: Greg KH <gregkh@...uxfoundation.org>, treapking@...omium.org
Cc: rafael@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] device_core: check null pointer

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.

>
> 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/
 .

> thanks,
>
> greg k-h

Powered by blists - more mailing lists