| 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: <2025112744-unpaired-spruce-854b@gregkh>
Date: Thu, 27 Nov 2025 13:42:59 +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 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?
thanks,
greg k-h
Powered by blists - more mailing lists