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

[Jorge Lopez]

On Thu, May 4, 2023 at 5:21 PM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>
> 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.
>

Let me see how I can implement it by following the sample of other drivers.
I will reachout to Hans as my last resource.

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

Fair enough.  I will remove "type", "display_name" and "display_langcode".
The same three entries will be removed from spmobj-attributes (SPM)
which belong to authentication attributes.

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

Ah.  That makes sense.  Thank you for the clarification.
Done!