| 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
| ||
|
Message-ID: <CADYyEwTw967qt2wmEp__1ji1ugarBfi+m9JWTozFivQGpPJq6A@mail.gmail.com>
Date: Thu, 27 Nov 2025 20:51:09 +0800
From: Zhengqiao Xia <xiazhengqiao@...qin.corp-partner.google.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: treapking@...omium.org, rafael@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] device_core: check null pointer
Greg KH <gregkh@...uxfoundation.org> 于2025年11月27日周四 20:43写道：
>
> On Thu, Nov 27, 2025 at 08:34:45PM +0800, Zhengqiao Xia wrote:
> > Greg KH <gregkh@...uxfoundation.org> 于2025年11月27日周四 19:51写道：
> > >
> > > 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.
> > As this（https://lore.kernel.org/all/CAEXTbpeN0Mk+Y-UeV79JzE547UCa_Fhh7T75L+2mhoSq3ark8g@mail.gmail.com/）
> > says：
> >
> > We have device A that has an on-board USB hub, so we describe it in the DTS.
> > Then, another device B is similar to device A but does not have the USB hub.
> > So, we inherit device A's DTS and disable the hub nodes.
> >
> > LTE device is connected to device B' port and no hub, so we disabled
> > the usb hub' node.
> > I think this is reasonable.
> >
> > >
> > > > > 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?
> >
> > yes, this prevents the LTE device's of_node from pointing to the USB hub.
> >
> > @@ -31,6 +31,9 @@ struct device_node *usb_of_get_device_node(struct
> > usb_device *hub, int port1)
> > if (of_property_read_u32(node, "reg", ®))
> > continue;
> >
> > + if (!of_device_is_available(node))
> > + continue;
> > +
> > if (reg == port1)
> > return node;
> > }
> >
> > 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;
> > }
> > so usb interface can continue to register.
>
> But where does this change usbfs's access to this interface?
Since the USB interface's of_node does not point to the USB hub, the
device can continue to register.
usbfs can access this interface normally, there will be no null
pointers. usbfs uses this interface to upgrade the firmware.
>
> thanks,
>
> greg k-h
Powered by blists - more mailing lists