Re: [PATCH v11 06/14] HP BIOSCFG driver - passwdobj-attributes

[Thomas Weißschuh]

On 2023-05-04 16:34:06-0500, Jorge Lopez wrote:
> On Thu, May 4, 2023 at 3:59 PM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
> >
> > On 2023-05-04 15:29:21-0500, Jorge Lopez wrote:
> > > On Sun, Apr 23, 2023 at 4:07 AM <thomas@xxxxxxxx> wrote:
> > > >
> > > > On 2023-04-20 11:54:46-0500, Jorge Lopez wrote:
> > > > > ---
> > > > >  .../x86/hp/hp-bioscfg/passwdobj-attributes.c  | 669 ++++++++++++++++++
> > > > >  1 file changed, 669 insertions(+)
> > > > >  create mode 100644 drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c

<snip>

> > > > > +
> > > > > +     id = get_password_instance_id(kobj);
> > > > > +
> > > > > +     if (id >= 0)
> > > > > +             ret = validate_password_input(id, buf_cp);
> > > > > +
> > > > > +     if (!ret) {
> > > > > +             strscpy(bioscfg_drv.password_data[id].current_password,
> > > > > +                     buf_cp,
> > > > > +                     sizeof(bioscfg_drv.password_data[id].current_password));
> > > > > +             /*
> > > > > +              * set pending reboot flag depending on
> > > > > +              * "RequiresPhysicalPresence" value
> > > > > +              */
> > > > > +             if (bioscfg_drv.password_data[id].common.requires_physical_presence)
> > > > > +                     bioscfg_drv.pending_reboot = true;
> > > >
> > > > Just setting this to true does not emit the necessary KOBJ_CHANGE event
> > > > on the class dev kobj which is necessary for userspace to be able to
> > > > react.
> > >
> > > This feature was added outside of the original design specification to
> > > be used at a later time.
> > > Changes to the value to true does not emit a KOBJ_CHANGE event.
> >
> > This contradicts the documentation:
> >
> >         A read-only attribute reads 1 if a reboot is necessary to apply
> >         pending BIOS attribute changes. Also, an uevent_KOBJ_CHANGE is
> >         generated when it changes to 1.
> >
> > This will confuse userspace, there are generic userspace applications
> > waiting for those events.
> > If there is a reason for not emitting them it should be good and
> > documented.
> >
> > Also according to the docs the authentication attributes should
> > KOBJ_CHANGE events. I think this also affects this driver and should be
> > implemented.
> >
> 
> HP-BIOSCFG initially is not intended for the use of fwupd tool nor was
> it tested.
> This does not mean the driver will interface with fwupd and other
> tools in the future.

There are probably more tools than fwupd using this ABI.

The driver is implementing a well-known ABI and users of this ABI expect
it to work as documented.

Emitting these events seems straigtforward and simple.

Maybe Hans can give more guidance on it.

> > > >
> > > > > +     }
> > > > > +
> > > > > +exit_password:
> > > > > +     kfree(buf_cp);
> > > > > +     return ret ? ret : count;
> > > > > +}
> > > > > +static struct kobj_attribute password_current_password = __ATTR_WO(current_password);
> > > > > +
> > > > > +static ssize_t new_password_store(struct kobject *kobj,
> > > > > +                               struct kobj_attribute *attr,
> > > > > +                               const char *buf, size_t count)
> > > > > +{
> > > > > +     char *p, *buf_cp = NULL;
> > > > > +     int id = 0;
> > > > > +     int ret = -EIO;
> > > > > +
> > > > > +     buf_cp = kstrdup(buf, GFP_KERNEL);
> > > > > +     if (!buf_cp) {
> > > > > +             ret = -ENOMEM;
> > > > > +             goto exit_password;
> > > > > +     }
> > > > > +
> > > > > +     p = memchr(buf_cp, '\n', count);
> > > > > +
> > > > > +     if (p != NULL)
> > > > > +             *p = '\0';
> > > >
> > > > Same as above.
> > >
> > > This is an expected behavior.  If the user enters '\n' as part of the
> > > password, the buffer data will be truncated since only one line per
> > > sysfs file is permitted.
> >
> > If a user accidentally presses enter before entering a password this
> > may set the password to the empty string; surprising.
> >
> > This should really use the helper.
> 
> Done!  enforce_single_line_input() function is used.
> >
> > > >
> <snip>
> > > > > +
> > > > > +ATTRIBUTE_V_COMMON_PROPERTY_SHOW(prerequisites, password);
> > > > > +static struct kobj_attribute  password_prerequisites_val =
> > > > > +             __ATTR_RO(prerequisites);
> > > > > +
> > > > > +ATTRIBUTE_N_PROPERTY_SHOW(encodings_size, password);
> > > > > +static struct kobj_attribute  password_encodings_size_val =
> > > > > +             __ATTR_RO(encodings_size);
> > > >
> > > > As before, these size attribute are fairly pointless for userspace as
> > > > they can't be relied on.
> > >
> > > I will remove the attribute from being reported in sysfs but they will
> > > be kept as part of the driver internal data
> > >
> > > >
> > > > > +
> > > > > +ATTRIBUTE_VALUES_PROPERTY_SHOW(encodings, password);
> > > > > +static struct kobj_attribute  password_encodings_val =
> > > > > +             __ATTR_RO(encodings);
> > > > > +
> > > > > +
> > > > > +static struct attribute *password_attrs[] = {
> > > > > +     &password_is_password_set.attr,
> > > > > +     &password_min_password_length.attr,
> > > > > +     &password_max_password_length.attr,
> > > > > +     &password_current_password.attr,
> > > > > +     &password_new_password.attr,
> > > > > +     &password_role.attr,
> > > > > +     &password_mechanism.attr,
> > > > > +     &password_type.attr,
> > > > > +     &password_display_name.attr,
> > > > > +     &password_display_langcode.attr,
> > > > > +     &password_prerequisites_size_val.attr,
> > > > > +     &password_prerequisites_val.attr,
> > > > > +     &password_encodings_val.attr,
> > > > > +     &password_encodings_size_val.attr,
> > > > > +     NULL
> > > > > +};
> > > >
> > > > Many of these attributes are not documented.
> > >
> > > Those attributes are documented under authentication section, lines 150-329
> > >
> > > What: /sys/class/firmware-attributes/*/authentication/
> > > Date: February 2021
> > > KernelVersion: 5.11
> > > Contact: Divya Bharathi <Divya.Bharathi@xxxxxxxx>,
> > > Prasanth KSR <prasanth.ksr@xxxxxxxx>
> > > Dell.Client.Kernel@xxxxxxxx
> > > Description:
> > > Devices support various authentication mechanisms which can be exposed
> > > as a separate configuration object.
> >
> > If I read that correctly the authentication attributes are not "normal"
> > attributes.
> > So they don't need "type", "display_name", "display_langcode".
> >
> 
> Type, display_name, and display_langcode are required and default settings.
> See documentation lines 15-52
> 
> type:
>     A file that can be read to obtain the type of attribute.
>     This attribute is mandatory.
> 
> display_name:
> A file that can be read to obtain a user friendly
> description of the at <attr>
> 
> display_name_language_code:
> A file that can be read to obtain
> the IETF language tag corresponding to the
> "display_name" of the <attr>

They are required for

/sys/class/firmware-attributes/*/attributes/*/

but here we implement 

/sys/class/firmware-attributes/*/authentication/*/

which is a different ABI.

> 
> > Do they need prerequisites?
> 
> Prerequisites is optional and not documented.  I will remove it from
> the list of items reported within sysfs
> >
> > >
> > >
> > > >
> > > > > +
> > > > > +static const struct attribute_group bios_password_attr_group = {
> > > > > +     .attrs = password_attrs
> > > > > +};
> > > > > +
> > > > > +static const struct attribute_group system_password_attr_group = {
> > > > > +     .attrs = password_attrs
> > > > > +};
> > > >
> > > > These groups are the same, are both needed?
> > >
> > > Yes.  They will show under  'Setup Password' and 'Power-on password'
> >
> > These are identical constant structures. It should be possible to have
> > only one and use it for both usecases.
> >
> 
> Both 'Setup Password' and 'Power-on password' need to coexist hence
> the reason for keeping them separate.
> Both attributes share the same helper routines.  Unifying  both
> passwords into a single structure adds unnecessary complexity.

They are already sharing the "password_attrs" array and all the
attributes listed in it.

It seems they could also share the attribute_group which does not really
contain any data.