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

[Jorge Lopez]

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
> > > >
> > > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
> > > > new file mode 100644
> > > > index 000000000000..c03b3a71e9c4
> > > > --- /dev/null
> > > > +++ b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
> > > > @@ -0,0 +1,669 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> >
> > <snip>
> >
> > > > +int validate_password_input(int instance_id, const char *buf)
> > > > +{
> > > > +     int length;
> > > > +
> > > > +     length = strlen(buf);
> > > > +     if (buf[length-1] == '\n')
> > > > +             length--;
> > > > +
> > > > +     if (length > MAX_PASSWD_SIZE)
> > > > +             return INVALID_BIOS_AUTH;
> > > > +
> > > > +     if (bioscfg_drv.password_data[instance_id].min_password_length > length ||
> > > > +         bioscfg_drv.password_data[instance_id].max_password_length < length)
> > > > +             return INVALID_BIOS_AUTH;
> > > > +     return SUCCESS;
> > > > +}
> > > > +
> > > > +int password_is_set(const char *name)
> > >
> > > bool is_password_set(const char *name)
> >
> > Function is invoked nowhere.  It will be removed.
> > >
> > > > +{
> > > > +     int id;
> > > > +
> > > > +     id = get_password_instance_for_type(name);
> > > > +     if (id < 0)
> > > > +             return 0;
> > > > +
> > > > +     return bioscfg_drv.password_data[id].is_enabled;
> > > > +}
> > > > +
> > > > +ATTRIBUTE_N_PROPERTY_SHOW(is_enabled, password);
> > > > +static struct kobj_attribute password_is_password_set = __ATTR_RO(is_enabled);
> > > > +
> > > > +static ssize_t current_password_store(struct kobject *kobj,
> > > > +                                   struct kobj_attribute *attr,
> > > > +                                   const char *buf, size_t count)
> > > > +{
> > > > +     char *p, *buf_cp;
> > > > +     int id, ret = 0;
> > > > +
> > > > +     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';
> > >
> > > This will also accept input like "foo\nbar" and truncate away the "bar".
> > >
> > > For something like a password it seems errorprone to try to munge the
> > > value.
> >
> > 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.
>
> As discussed in another patch this would silently truncate the input on
> the first newline character; even if it is not the last character of the
> input string.
>
> This should use the same helper as the other files to only strip a
> newline at the end of the input.
>

Done!  enforce_single_line_input() function is used.

> > >
> > > > +
> > > > +     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.

> > >
> > > > +     }
> > > > +
> > > > +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>

> 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.



> <snip>