Received-SPF: pass (google.com: domain of linux-kernel+bounces-148067-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <CAEkJfYON+ry7xPx=AiLR9jzUNT+i_Va68ACajOC3HoacOfL1ig@mail.gmail.com>
 <92fe8e95-bc01-4d7d-9678-8cfc55cc4a7b@rowland.harvard.edu>
 <CAEkJfYORHKO16xT3DCS04JFzkquz6oZ5CdC2USJ5-c0WihAMXg@mail.gmail.com>
 <45e246ab-01e8-40b7-8ede-b47957df0d7b@rowland.harvard.edu>
 <CAEkJfYMjO+vMBGPcaLa51gjeKxFAJBrSa0t_iJUtauQD3DaK8w@mail.gmail.com>
 <69a6f4c9-6470-40d1-99f1-aaf532497d02@rowland.harvard.edu>
 <CAEkJfYNJyyGhR9AAWc0V7o8i6pmS+OB=KSbh6XqVWAAGetS9hA@mail.gmail.com>
 <5704ac63-5e5b-416c-a2a1-57528e76a02f@rowland.harvard.edu>
 <CAEkJfYMSwuikpBJudOaFYrxgf9e=_O4nig6sTPLLAtpdEKQuyQ@mail.gmail.com> <5f3526a6-6ede-4181-a4ff-076e022cfb49@rowland.harvard.edu>
In-Reply-To: <5f3526a6-6ede-4181-a4ff-076e022cfb49@rowland.harvard.edu>
From: Sam Sun <samsun1006219@gmail.com>
Date: Wed, 17 Apr 2024 15:39:02 +0800
Message-ID: <CAEkJfYMPQJn+kYzxFwwix3fwKeu3aAdYKgp+Ksvq=o4CoTXEWQ@mail.gmail.com>
Subject: Re: [Linux kernel bug] general protection fault in disable_store
To: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, 
	Greg KH <gregkh@linuxfoundation.org>, swboyd@chromium.org, ricardo@marliere.net, 
	hkallweit1@gmail.com, heikki.krogerus@linux.intel.com, 
	mathias.nyman@linux.intel.com, royluo@google.com, 
	syzkaller-bugs@googlegroups.com, xrivendell7@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 17, 2024 at 12:35=E2=80=AFAM Alan Stern <stern@rowland.harvard.=
edu> wrote:
>
> On Tue, Apr 16, 2024 at 05:05:55PM +0800, Sam Sun wrote:
> > On Mon, Apr 15, 2024 at 10:47=E2=80=AFPM Alan Stern <stern@rowland.harv=
ard.edu> wrote:
> > >
> > > Actually, I've got a completely different patch which I think will fi=
x
> > > 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.
>

I checked the commit you mentioned. Maybe your first fix is all we
need to fix the problem? At least no race would occur for
hdev->maxchild and usb_set_intfdata().

> 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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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 =3D to_usb_port(dev);
>         struct usb_device *hdev =3D to_usb_device(dev->parent->parent);
>         struct usb_hub *hub =3D usb_hub_to_struct_hub(hdev);
> -       struct usb_interface *intf =3D to_usb_interface(hub->intfdev);
> +       struct usb_interface *intf =3D to_usb_interface(dev->parent);
>         int port1 =3D port_dev->portnum;
>         u16 portstatus, unused;
>         bool disabled;
>         int rc;
>         struct kernfs_node *kn;
>
> +       if (!hub)
> +               return -ENODEV;
>         hub_get(hub);
>         rc =3D usb_autopm_get_interface(intf);
>         if (rc < 0)
> @@ -101,12 +103,14 @@ static ssize_t disable_store(struct devi
>         struct usb_port *port_dev =3D to_usb_port(dev);
>         struct usb_device *hdev =3D to_usb_device(dev->parent->parent);
>         struct usb_hub *hub =3D usb_hub_to_struct_hub(hdev);
> -       struct usb_interface *intf =3D to_usb_interface(hub->intfdev);
> +       struct usb_interface *intf =3D to_usb_interface(dev->parent);
>         int port1 =3D port_dev->portnum;
>         bool disabled;
>         int rc;
>         struct kernfs_node *kn;
>
> +       if (!hub)
> +               return -ENODEV;
>         rc =3D kstrtobool(buf, &disabled);
>         if (rc)
>                 return rc;
>

I applied this patch and it also can fix the warning. I am not sure
which one is better.

Best,
Yue