From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 03E712FE56B for ; Thu, 27 Nov 2025 09:54:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764237286; cv=none; b=qyf6t45uG/M3+sCD+LHTkB503rNyogzyd7GCpnNHVxdRuejERSi/yJ/XFfQwzOafWn54aLgU90c6coAr5ou/cbVk1rq8H4px/9xab+sIi5/9vS8PQfHvbhla0OEGCgjrNXmmPTZlVQQx1UZNXAH/UYhpV2H4t6Z9F8Y10Tj5jT8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764237286; c=relaxed/simple; bh=czRfuWNAsapu4UNPEPm3QHae0mwqmCCeICcsENOKvw8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=BXKqW/foiMNGMS7puOE7WN80F38cK6LheMLvMR0mkX4HyKAHypZEQ8uKmPmsayQFrUJb5tY03j70WMNmGgEMThcoYzJ88zVFv11Oxj3O8ZQ3Vb92nLlKvytILfacePrTejeBu2ftM62WSKkKbIfZg5NX34VkLIqWaxNYU4F5f04= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.b=mQhKLDCa; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.b="mQhKLDCa" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 594EBC4CEF8; Thu, 27 Nov 2025 09:54:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1764237285; bh=czRfuWNAsapu4UNPEPm3QHae0mwqmCCeICcsENOKvw8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mQhKLDCas9ODTqmDnGkpl+uVjEyzdOgtvRRzdW0BXT9SbXeHb9L5GDtAGUwhl3PP9 Hd6vNCveisliXLzM7wimGOfRc2htpS0q9tIznCNbeNyzv/OIflPfAUgAFHO131OK7O JvwIGFKnQ5Wro2QMvLIcLWjmuZdysHzHjv61mv24= Date: Thu, 27 Nov 2025 10:54:43 +0100 From: Greg KH To: Zhengqiao Xia Cc: rafael@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] device_core: check null pointer Message-ID: <2025112725-comrade-willfully-67f1@gregkh> References: <20251114141821.416835-1-xiazhengqiao@huaqin.corp-partner.google.com> <2025111523-preface-scoreless-b478@gregkh> <2025111720-sensitize-schnapps-3189@gregkh> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Tue, Nov 18, 2025 at 03:31:03PM +0800, Zhengqiao Xia wrote: > Greg KH 于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