linux-kernel - Re: [PATCH] usb: core: Prevent infinite loops when usb_reset

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] [day] [month] [year] [list]

Message-ID: <CAD=FV=Vv+xiJu0MwEgF9VpdzcQVmmCE+m2Msa09QXB_gh4U=Hg@mail.gmail.com>
Date:   Fri, 20 Oct 2023 09:27:48 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, Grant Grundler <grundler@...omium.org>,
        Hayes Wang <hayeswang@...ltek.com>,
        Bastien Nocera <hadess@...ess.net>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Flavio Suligoi <f.suligoi@...m.it>,
        Hans de Goede <hdegoede@...hat.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Ivan Orlov <ivan.orlov0322@...il.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Ray Chi <raychi@...gle.com>,
        Ricardo Cañuelo <ricardo.canuelo@...labora.com>,
        Rob Herring <robh@...nel.org>, Roy Luo <royluo@...gle.com>,
        Stanley Chang <stanley_chang@...ltek.com>,
        Vincent Mailhol <mailhol.vincent@...adoo.fr>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: core: Prevent infinite loops when usb_reset_device() unbinds/binds

Hi,

On Fri, Oct 20, 2023 at 9:23 AM Alan Stern <stern@...land.harvard.edu> wrote:
>
> On Fri, Oct 20, 2023 at 08:59:42AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Oct 20, 2023 at 8:46 AM Alan Stern <stern@...land.harvard.edu> wrote:
> > >
> > > On Fri, Oct 20, 2023 at 08:31:38AM -0700, Douglas Anderson wrote:
> > > > When we call usb_reset_device() and a driver doesn't implement
> > > > pre_reset() and post_reset() methods then the USB core will attempt to
> > > > unbind and rebind the driver in order to make reset work. This is a
> > > > great general solution, but it has the potential to loop forever.
> > > > Specifically, if the USB device is in a state that the USB device
> > > > driver issues another usb_reset_device() after each rebind then we'll
> > > > just continually unbind and rebind with no end.
> > > >
> > > > It's difficult to address this condition in a USB device driver
> > > > because it's hard for the driver to keep state across each
> > > > unbind/bind.
> > >
> > > How about just adding appropriate pre_reset() and post_reset() methods?
> > > This is precisely what they are meant for.  Then the the unbind/rebind
> > > loop wouldn't ever get started.
> >
> > Right, and we have pre_reset() and post_reset() in the r1852 driver.
> > The issue is that we are seeing occasional control transfer errors
> > while the r8152 driver is still running its probe() routine and we
> > want to reset in response to those. It is relatively difficult to have
> > the pre_reset() and post_reset() methods work properly if failures
> > happen and probe() hasn't finished yet.
>
> Why is that?
>
> >  The current proposal I have
> > for the r8152 driver is to have the pre_reset() routine return -EIO if
> > we saw errors during probe, which tells the USB core to unbind/rebind
> > us. This gets us to a known good state.
>
> Don't you also get to a known good state if pre_reset() and post_reset()
> both return 0?  Then there's no unbinding, so the driver can just jump
> back to the start of its probe() routine.  Or fail the probe, if there
> have been too many errors.
>
> >  If we need to do a reset later
> > on (after probe finished successfully) then pre_reset() and
> > post_reset() can avoid the unbind/bind.
> >
> > The worry was that this could cause an infinite loop. Hence this patch. ;-)
>
> With no unbinding/rebinding, any loops that occur will be entirely under
> the driver's control.  Then it should easily be able to avoid looping
> forever.

Hmmm, OK. Let's see. I guess the most annoying thing would be dealing
with anything "devm". I guess in the case of the r8152 driver, though,
there is no use of devm. Thus I should be able to do this. Let me give
it a shot and see how it looks.

-Doug