linux-kernel - Re: [Linux kernel bug] general protection fault in disable

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]

Date: Tue, 16 Apr 2024 17:05:55 +0800
From: Sam Sun <samsun1006219@...il.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org, 
	Greg KH <gregkh@...uxfoundation.org>, swboyd@...omium.org, ricardo@...liere.net, 
	hkallweit1@...il.com, heikki.krogerus@...ux.intel.com, 
	mathias.nyman@...ux.intel.com, royluo@...gle.com, 
	syzkaller-bugs@...glegroups.com, xrivendell7@...il.com
Subject: Re: [Linux kernel bug] general protection fault in disable_store

On Mon, Apr 15, 2024 at 10:47 PM Alan Stern <stern@...land.harvard.edu> wrote:
>
> Actually, I've got a completely different patch which I think will fix
> the problem you encountered.  Instead of using mutual exclusion to
> avoid the race, it prevents the two routines from being called at the
> same time so the race can't occur in the first place.  It also should
> guarantee the usb_hub_to_struct_hub() doesn't return NULL when
> disable_store() calls it.
>
> Can you try the patch below, instead of (not along with) the first
> patch?  Thanks.
>
> Alan Stern
>
>
>
> Index: usb-devel/drivers/usb/core/hub.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/hub.c
> +++ usb-devel/drivers/usb/core/hub.c
> @@ -1788,16 +1788,15 @@ static void hub_disconnect(struct usb_in
>
>         mutex_lock(&usb_port_peer_mutex);
>
> +       for (port1 = hdev->maxchild; port1 > 0; --port1)
> +               usb_hub_remove_port_device(hub, port1);
> +
>         /* Avoid races with recursively_mark_NOTATTACHED() */
>         spin_lock_irq(&device_state_lock);
> -       port1 = hdev->maxchild;
>         hdev->maxchild = 0;
>         usb_set_intfdata(intf, NULL);
>         spin_unlock_irq(&device_state_lock);
>
> -       for (; port1 > 0; --port1)
> -               usb_hub_remove_port_device(hub, port1);
> -
>         mutex_unlock(&usb_port_peer_mutex);
>
>         if (hub->hdev->speed == USB_SPEED_HIGH)
>

I tried this patch and it worked. I agree this patch is better and it
avoids introducing new locks.

Best,
Yue