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: <CANFp7mU0h0=UimV_tCDw=YuQcdPZsSZ0sJEMv1kNJKiDjs1KHg@mail.gmail.com>
Date: Fri, 8 Nov 2024 10:28:52 -0800
From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: heikki.krogerus@...ux.intel.com, tzungbi@...nel.org, 
	linux-usb@...r.kernel.org, chrome-platform@...ts.linux.dev, jthies@...gle.com, 
	akuchynski@...gle.com, pmalani@...omium.org, dmitry.baryshkov@...aro.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/7] usb: typec: Check port is active before enter mode
 on probe

On Fri, Nov 8, 2024 at 5:17 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote:
> > Enforce the same requirement as when we attempt to activate altmode via
> > sysfs (do not enter partner mode if port mode is not active). In order
> > to set a default value for this field, also introduce the inactive field
> > in struct typec_altmode_desc.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> > ---
> >
> > Changes in v3:
> > - Use port.active instead of introducing auto-enter field
> > - Introduce inactive field to typec_altmode_desc to set default value
> >   for active.
> > - Always make port 'active' field writable
> >
> >  drivers/usb/typec/altmodes/displayport.c | 7 +++++--
> >  drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++-
> >  drivers/usb/typec/class.c                | 5 +++--
> >  include/linux/usb/typec.h                | 2 ++
> >  4 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> > index 3245e03d59e6..f4116e96f6a1 100644
> > --- a/drivers/usb/typec/altmodes/displayport.c
> > +++ b/drivers/usb/typec/altmodes/displayport.c
> > @@ -767,8 +767,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
> >       if (plug)
> >               typec_altmode_set_drvdata(plug, dp);
> >
> > -     dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> > -     schedule_work(&dp->work);
> > +     /* Only attempt to enter altmode if port is active. */
> > +     if (port->active) {
> > +             dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> > +             schedule_work(&dp->work);
> > +     }
>
> What prevents active from changing right after you checked it?

There's another check in `typec_altmode_enter` for the port itself:
https://github.com/torvalds/linux/blob/master/drivers/usb/typec/bus.c#L138

If I leave this out, it will just fail in `dp_altmode_work` when it
calls `typec_altmode_enter` and will leave a dev_err("failed to enter
mode"). This might be more user friendly since it's visible to the
user that the partner supports the mode but the port blocked entry.
I'll update the message on mode entry to also print the return value
(-EPERM) in the next patch.

>
> > @@ -150,6 +151,7 @@ struct typec_altmode_desc {
> >       u32                     vdo;
> >       /* Only used with ports */
> >       enum typec_port_data    roles;
> > +     bool                    inactive : 1;
>
> A boolean bitfield?  That's not needed, please just do this properly.

Ack - will remove the bitfield. `struct typec_altmode` also does this
-- I'll remove that in a cleanup patch too.

>
> thanks,
>
> greg k-h

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ