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]

Message-ID: <5f3526a6-6ede-4181-a4ff-076e022cfb49@rowland.harvard.edu>
Date: Tue, 16 Apr 2024 12:35:51 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Sam Sun <samsun1006219@...il.com>
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 Tue, Apr 16, 2024 at 05:05:55PM +0800, Sam Sun wrote:
> 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

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

It turns out that patch is no good.  The reason is mentioned in the 
changelog for commit 543d7784b07f ("USB: fix race between hub_disconnect 
and recursively_mark_NOTATTACHED"); it says that the port devices have to 
be removed _after_ maxchild has been set to 0.

So okay, here's a third attempt to fix the problem.  This doesn't try to 
avoid the race or anything like that.  Instead it just adds checks for 
usb_hub_to_struct_hub() returning NULL.  That should be enough to prevent 
the invalid pointer dereference you encountered.

This should be tested by itself, without either of the first two patches.

Alan Stern



Index: usb-devel/drivers/usb/core/port.c
===================================================================
--- usb-devel.orig/drivers/usb/core/port.c
+++ usb-devel/drivers/usb/core/port.c
@@ -51,13 +51,15 @@ static ssize_t disable_show(struct devic
 	struct usb_port *port_dev = to_usb_port(dev);
 	struct usb_device *hdev = to_usb_device(dev->parent->parent);
 	struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
-	struct usb_interface *intf = to_usb_interface(hub->intfdev);
+	struct usb_interface *intf = to_usb_interface(dev->parent);
 	int port1 = port_dev->portnum;
 	u16 portstatus, unused;
 	bool disabled;
 	int rc;
 	struct kernfs_node *kn;
 
+	if (!hub)
+		return -ENODEV;
 	hub_get(hub);
 	rc = usb_autopm_get_interface(intf);
 	if (rc < 0)
@@ -101,12 +103,14 @@ static ssize_t disable_store(struct devi
 	struct usb_port *port_dev = to_usb_port(dev);
 	struct usb_device *hdev = to_usb_device(dev->parent->parent);
 	struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
-	struct usb_interface *intf = to_usb_interface(hub->intfdev);
+	struct usb_interface *intf = to_usb_interface(dev->parent);
 	int port1 = port_dev->portnum;
 	bool disabled;
 	int rc;
 	struct kernfs_node *kn;
 
+	if (!hub)
+		return -ENODEV;
 	rc = kstrtobool(buf, &disabled);
 	if (rc)
 		return rc;