netdev - Re: [PATCH v5 8/8] r8152: Block future register access if register access fails

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: Wed, 25 Oct 2023 13:24:55 -0700
From: Doug Anderson <dianders@...omium.org>
To: Simon Horman <horms@...nel.org>
Cc: Jakub Kicinski <kuba@...nel.org>, Hayes Wang <hayeswang@...ltek.com>, 
	"David S . Miller" <davem@...emloft.net>, Edward Hill <ecgh@...omium.org>, 
	Laura Nao <laura.nao@...labora.com>, Alan Stern <stern@...land.harvard.edu>, 
	linux-usb@...r.kernel.org, Grant Grundler <grundler@...omium.org>, 
	Bjørn Mork <bjorn@...k.no>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH v5 8/8] r8152: Block future register access if register
 access fails

Hi,

On Wed, Oct 25, 2023 at 9:28 AM Simon Horman <horms@...nel.org> wrote:
>
> On Fri, Oct 20, 2023 at 02:06:59PM -0700, Douglas Anderson wrote:
>
> ...
>
> > @@ -9603,25 +9713,14 @@ static bool rtl8152_supports_lenovo_macpassthru(struct usb_device *udev)
> >       return 0;
> >  }
> >
> > -static int rtl8152_probe(struct usb_interface *intf,
> > -                      const struct usb_device_id *id)
> > +static int rtl8152_probe_once(struct usb_interface *intf,
> > +                           const struct usb_device_id *id, u8 version)
> >  {
> >       struct usb_device *udev = interface_to_usbdev(intf);
> >       struct r8152 *tp;
> >       struct net_device *netdev;
> > -     u8 version;
> >       int ret;
> >
> > -     if (intf->cur_altsetting->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC)
> > -             return -ENODEV;
> > -
> > -     if (!rtl_check_vendor_ok(intf))
> > -             return -ENODEV;
> > -
> > -     version = rtl8152_get_version(intf);
> > -     if (version == RTL_VER_UNKNOWN)
> > -             return -ENODEV;
> > -
> >       usb_reset_device(udev);
> >       netdev = alloc_etherdev(sizeof(struct r8152));
> >       if (!netdev) {
> > @@ -9784,10 +9883,20 @@ static int rtl8152_probe(struct usb_interface *intf,
> >       else
> >               device_set_wakeup_enable(&udev->dev, false);
> >
> > +     /* If we saw a control transfer error while probing then we may
> > +      * want to try probe() again. Consider this an error.
> > +      */
> > +     if (test_bit(PROBE_SHOULD_RETRY, &tp->flags))
> > +             goto out2;
>
> Sorry for being a bit slow here, but if this is an error condition,
> sould ret be set to an error value?
>
> As flagged by Smatch.

Thanks for the note. I think we're OK, though. If you look at the
"out:" label, which is right after "out1" it tests for the same bit.
That will set "ret = -EAGAIN" for us.

I'll admit it probably violates the principle of least astonishment,
but there's a method to my madness. Specifically:

a) We need a test here to make sure we don't return "success" if the
bit is set. The driver doesn't error check for success when it
modifies HW registers so it might _thnk_ it was successful but still
have this bit set. ...so we need this check right before we return
"success".

b) We also need to test for this bit if we're in the error handling
code. Even though the driver doesn't check for success in lots of
places, there still could be some places that notice an error. It may
return any kind of error here, so we need to override it to -EAGAIN.

...so I just set "ret = -EAGAIN" in one place.

Does that make sense? If you want to submit a patch adjusting the
comment to make this more obvious, I'm happy to review it.

-Doug