netdev - Re: [PATCH net 2/2] r8152: Switch to using choose

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZYqpGqSgZBV800Or@mail.gmail.com>
Date: Tue, 26 Dec 2023 12:21:14 +0200
From: Maxim Mikityanskiy <maxtram95@...il.com>
To: Grant Grundler <grundler@...omium.org>
Cc: "David S. Miller" <davem@...emloft.net>,
	Bjørn Mork <bjorn@...k.no>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Hayes Wang <hayeswang@...ltek.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Douglas Anderson <dianders@...omium.org>, linux-usb@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH net 2/2] r8152: Switch to using choose_configuration

On Mon, 25 Dec 2023 at 23:42:08 -0800, Grant Grundler wrote:
> On Mon, Dec 25, 2023 at 11:18 PM Grant Grundler <grundler@...omium.org> wrote:
> >
> > On Sat, Dec 23, 2023 at 3:36 PM Maxim Mikityanskiy <maxtram95@...il.com> wrote:
> > >
> > > With the introduction of r8152-cfgselector, the following regression
> > > appeared on machines that use usbguard: the netdev appears only when the
> > > USB device is inserted the first time (before the module is loaded), but
> > > on the second and next insertions no netdev is registered.
> > >
> > > It happens because the device is probed as unauthorized, and usbguard
> > > gives it an authorization a moment later. If the module is not loaded,
> > > it's normally loaded slower than the authorization is given, and
> > > everything works. If the module is already loaded, the cfgselector's
> > > probe function runs first, but then usb_authorize_device kicks in and
> > > changes the configuration to something chosen by the standard
> > > usb_choose_configuration. rtl8152_probe refuses to probe non-vendor
> > > configurations, and the user ends up without a netdev.
> > >
> > > The previous commit added possibility to override
> > > usb_choose_configuration. Use it to fix the bug and pick the right
> > > configuration on both probe and authorization.
> > >
> > > Fixes: ec51fbd1b8a2 ("r8152: add USB device driver for config selection")
> > > Signed-off-by: Maxim Mikityanskiy <maxtram95@...il.com>
> >
> > Maxim,
> > Does this solve the same problem that Doug Anderson posted patches for
> > a few weeks ago?
> >
> > https://lore.kernel.org/all/20231201183113.343256-1-dianders@chromium.org/
> >
> > Those went through the USB tree, so that probably explains why you
> > didn't see them in the net.git tree.

Oh right, I didn't see these... Sorry for making noise then, Doug's
patches should solve my problem.

> Specifically, usb.git usb-next branch:
> "r8152: Choose our USB config with choose_configuration() rather than probe()"
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/net/usb/r8152.c?h=usb-next&id=aa4f2b3e418e8673e55145de8b8016a7a9920306
> 
> Given this has a fixes tag, and two people have now tracked this down
> and posted fixes for this problem, is there any chance of this landing
> in 6.7?
> 
> And it looks like Maxim's change could be rebased on top of Doug's
> patch and clean up a few more things. Do I see that correctly?

I only see some minor difference in error handling, not sure it's worth
chasing for.

> cheers,
> grant
> 
> >
> > cheers,
> > grant
> >
> > > ---
> > >  drivers/net/usb/r8152.c | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > > index 9bf2140fd0a1..f0ac31a94f3c 100644
> > > --- a/drivers/net/usb/r8152.c
> > > +++ b/drivers/net/usb/r8152.c
> > > @@ -10070,6 +10070,11 @@ static struct usb_driver rtl8152_driver = {
> > >  };
> > >
> > >  static int rtl8152_cfgselector_probe(struct usb_device *udev)
> > > +{
> > > +       return 0;
> > > +}
> > > +
> > > +static int rtl8152_cfgselector_choose_configuration(struct usb_device *udev)
> > >  {
> > >         struct usb_host_config *c;
> > >         int i, num_configs;
> > > @@ -10078,7 +10083,7 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
> > >          * driver supports it.
> > >          */
> > >         if (__rtl_get_hw_ver(udev) == RTL_VER_UNKNOWN)
> > > -               return 0;
> > > +               return -EOPNOTSUPP;
> > >
> > >         /* The vendor mode is not always config #1, so to find it out. */
> > >         c = udev->config;
> > > @@ -10094,20 +10099,15 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
> > >         }
> > >
> > >         if (i == num_configs)
> > > -               return -ENODEV;
> > > -
> > > -       if (usb_set_configuration(udev, c->desc.bConfigurationValue)) {
> > > -               dev_err(&udev->dev, "Failed to set configuration %d\n",
> > > -                       c->desc.bConfigurationValue);
> > > -               return -ENODEV;
> > > -       }
> > > +               return -EOPNOTSUPP;
> > >
> > > -       return 0;
> > > +       return c->desc.bConfigurationValue;
> > >  }
> > >
> > >  static struct usb_device_driver rtl8152_cfgselector_driver = {
> > >         .name =         MODULENAME "-cfgselector",
> > >         .probe =        rtl8152_cfgselector_probe,
> > > +       .choose_configuration = rtl8152_cfgselector_choose_configuration,
> > >         .id_table =     rtl8152_table,
> > >         .generic_subclass = 1,
> > >         .supports_autosuspend = 1,
> > > --
> > > 2.43.0
> > >