linux-kernel - Re: [PATCH v2] usb: core: Prevent null pointer dereference in update_port_device

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2d801dd7-93de-4323-a214-1a73cc5a8451@rowland.harvard.edu>
Date: Mon, 8 Jan 2024 10:26:07 -0500
From: Alan Stern <stern@...land.harvard.edu>
To: Udipto Goswami <quic_ugoswami@...cinc.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
  Krishna Kurapati <quic_kriskura@...cinc.com>, linux-usb@...r.kernel.org,
  linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] usb: core: Prevent null pointer dereference in
 update_port_device_state

On Mon, Jan 08, 2024 at 06:37:06PM +0530, Udipto Goswami wrote:
> Currently,the function update_port_device_state gets the usb_hub from
> udev->parent by calling usb_hub_to_struct_hub.
> However, in case the actconfig or the maxchild is 0, the usb_hub would
> be NULL and upon further accessing to get port_dev would result in null
> pointer dereference.
> 
> Fix this by introducing an if check after the usb_hub is populated.
> 
> Fixes: 83cb2604f641 ("usb: core: add sysfs entry for usb device state")
> Cc: stable@...r.kernel.org
> Signed-off-by: Udipto Goswami <quic_ugoswami@...cinc.com>
> ---
> v2: Introduced comment for the if check & CC'ed stable.
> 
>  drivers/usb/core/hub.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index ffd7c99e24a3..d40b5500f95b 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2053,9 +2053,18 @@ static void update_port_device_state(struct usb_device *udev)
>  
>  	if (udev->parent) {
>  		hub = usb_hub_to_struct_hub(udev->parent);
> -		port_dev = hub->ports[udev->portnum - 1];
> -		WRITE_ONCE(port_dev->state, udev->state);
> -		sysfs_notify_dirent(port_dev->state_kn);
> +
> +		/*
> +		 * usb_hub_to_struct_hub() if returns NULL can
> +		 * potentially cause NULL pointer dereference upon further
> +		 * access.
> +		 * Avoid this with an if check.
> +		 */

This is not what I meant.  It's perfectly obvious that if 
usb_hub_to_struct_hub() returns NULL then there will be a NULL-pointer 
dereference.  You don't need to explain that to anybody.

Instead, you need to explain why it is _possible_ for 
usb_hub_to_struct_hub() to return NULL.  The reason is because the 
lvstest driver messes around with usbcore internals without telling the 
hub driver, so hub will be NULL in cases where udev was created by 
lvstest.

Alan Stern