Re: [PATCH v6 4/4] Introduction of HP-BIOSCFG driver [4]

[Jorge Lopez]

Hi Thomas,

Please see my comments below.

On Sat, Apr 1, 2023 at 6:58 AM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>
> Hi Jorge,
>
> Hans asked me to do a review of your series, so this is it.
>
> I'll start with patch 4 because it is the one with the docs and build
> system changes.
> Reviews of the other patches and the code of this patch will follow.
>
> In my opinion the best way forward is to drop some of the non-core
> and duplicated functionality.
> The reduced scope will make review and rework easier and therefore speed
> up the process.
>
> Please also Cc the general kernel mailing list
> linux-kernel@xxxxxxxxxxxxxxx for future revisions.
> This will make sure the patchset is picked up and tested by the bots.
>
Will do.

> On 2023-03-09 14:10:22-0600, Jorge Lopez wrote:
> > The purpose for this patch is submit HP BIOSCFG driver to be list of
> > HP Linux kernel drivers.  The driver include a total of 12 files
> > broken in several patches.
>
> No need for this paragraph.

I will remove it in the next submission.

>
> > HP BIOS Configuration driver purpose is to provide a driver supporting
> > the latest sysfs class firmware attributes framework allowing the user
> > to change BIOS settings and security solutions on HP Inc.’s commercial
> > notebooks.
>
> Here it says "notebooks", below "PC's". Does it also support
> non-notebook machines?

The initial release of the driver will be supported for business notebooks.
Although the driver is not targeted for non-notebooks machines, the
driver was tested on non-notebooks in the event a decision is made to
targets them

> > --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> > +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> > @@ -22,6 +22,13 @@ Description:
> >                       - integer: a range of numerical values
> >                       - string
> >
> > +             HP specific types
> > +             -----------------
> > +                     - ordered-list - a set of ordered list valid values
> > +                     - sure-admin
>
> Does sure-admin still exist?

I will remove that entry.   Sure-admin no longer exist as part of the driver

>> > +             HP specific class extensions
> > +             ------------------------------
> > +
> > +             On HP systems the following additional attributes are available:
> > +
> > +             "ordered-list"-type specific properties:
> > +
> > +             elements:
> > +                                     A file that can be read to obtain the possible
> > +                                     list of values of the <attr>. Values are separated using
> > +                                     semi-colon (``;``). The order individual elements are listed
> > +                                     according to their priority.  An Element listed first has the
> > +                                     hightest priority. Writing the list in a different order to
> > +                                     current_value alters the priority order for the particular
> > +                                     attribute.
> > +
> > +             "sure-start"-type specific properties:
> > +
> > +             audit_log_entries:
> > +                                     A read-only file that returns the events in the log.
> > +
> > +                                     Audit log entry format
> > +
> > +                                     Byte 0-15:   Requested Audit Log entry  (Each Audit log is 16 bytes)
> > +                                     Byte 16-127: Unused
> > +
> > +             audit_log_entry_count:
> > +                                     A read-only file that returns the number of existing audit log events available to be read.
> > +
> > +                                     [No of entries],[log entry size],[Max number of entries supported]
>
> sysfs is based on the idea of "one-value-per-file".
> The two properties above violate this idea.
> Maybe a different interface is needed.
>

Both properties report a single string separated by semicolon.  This
is not different from listing all elements in a single string
separated by semicolon.

> Are these properties very important for the first version of this
> driver? If not I would propose to drop them for now and resubmit them
> as separate patches after the main driver has been merged.
>
We want the initial driver to have all predefined properties available
first.   There are plans to add future properties and features which
will be submitted as patches.

> > +
> > +
> >  What:                /sys/class/firmware-attributes/*/authentication/
> >  Date:                February 2021
> >  KernelVersion:       5.11
> > @@ -206,7 +245,7 @@ Description:
> >               Drivers may emit a CHANGE uevent when a password is set or unset
> >               userspace may check it again.
> >
> > -             On Dell and Lenovo systems, if Admin password is set, then all BIOS attributes
> > +             On Dell, Lenovo, and HP systems, if Admin password is set, then all BIOS attributes
>
> No comma after "Lenovo"

Will do
>
> >               require password validation.
> >               On Lenovo systems if you change the Admin password the new password is not active until
> >               the next boot.
> > @@ -296,6 +335,15 @@ Description:
> >                                               echo "signature" > authentication/Admin/signature
> >                                               echo "password" > authentication/Admin/certificate_to_password
> >
> > +             HP specific class extensions
> > +             --------------------------------
> > +
> > +             On HP systems the following additional settings are available:
> > +
> > +             role: enhanced-bios-auth:
> > +                                     This role is specific to Secure Platform Management (SPM) attribute.
> > +                                     It requires configuring an endorsement (kek) and signing certificate (sk).
> > +
> >
> >  What:                /sys/class/firmware-attributes/*/attributes/pending_reboot
> >  Date:                February 2021
> > @@ -364,3 +412,60 @@ Description:
> >               use it to enable extra debug attributes or BIOS features for testing purposes.
> >
> >               Note that any changes to this attribute requires a reboot for changes to take effect.
> > +
> > +
> > +             HP specific class extensions
> > +             --------------------------------
> > +
> > +What:                /sys/class/firmware-attributes/*/authentication/SPM/kek
> > +Date:                March 29
> > +KernelVersion:       5.18
> > +Contact:     "Jorge Lopez" <jorge.lopez2@xxxxxx>
> > +Description: 'kek' is a write-only file that can be used to configure the
> > +             RSA public key that will be used by the BIOS to verify
> > +             signatures when setting the signing key.  When written,
> > +             the bytes should correspond to the KEK certificate
> > +             (x509 .DER format containing an OU).  The size of the
> > +             certificate must be less than or equal to 4095 bytes.
> > +
> > +
> > +What:                /sys/class/firmware-attributes/*/authentication/SPM/sk
> > +Date:                March 29
> > +KernelVersion:       5.18
> > +Contact:     "Jorge Lopez" <jorge.lopez2@xxxxxx>
> > +Description: 'sk' is a write-only file that can be used to configure the RSA
> > +             public key that will be used by the BIOS to verify signatures
> > +             when configuring BIOS settings and security features.  When
> > +             written, the bytes should correspond to the modulus of the
> > +             public key.  The exponent is assumed to be 0x10001.
>
> The names of the files 'SPM', 'kek' and 'sk' are cryptic.

SPM - Secure Platform Manager
kek -  Key-Encryption-Key (KEK)
sk - Signature Key (SK)

Those abbreviations were used because they are industry standard and
reduce the  size of the commands.  Any suggestions?
>
> > +
> > +
> > +What:                /sys/class/firmware-attributes/*/authentication/SPM/status
> > +Date:                March 29
> > +KernelVersion:       5.18
> > +Contact:     "Jorge Lopez" <jorge.lopez2@xxxxxx>
> > +Description: 'status' is a read-only file that returns ASCII text reporting
> > +             the status information.
> > +
> > +               State:  Not Provisioned / Provisioned / Provisioning in progress
> > +               Version:  Major.   Minor
> > +               Feature Bit Mask: <16-bit unsigned number display in hex>
> > +               SPM Counter: <16-bit unsigned number display in base 10>
> > +               Signing Key Public Key Modulus (base64):
> > +               KEK Public Key Modulus (base64):
>
> This also violates 'one-value-per-file'.
> Can it be split into different files?

I will split the information in multiple files.

> This would also remove the need for the statusbin file.
>
Status bin is used by GUI applications where data is managed
accordingly instead of individual lines.

> For the values:
>
> Status: I think symbolic names are better for sysfs:
>         not_provisioned, provisioned, etc.
> Feature Bit Mask: Use names.
> Keys: It would be nicer if these could be shown directly in the files
>       that can be used to configure them.
>
> As before, what is really needed and what can be added later?

Status is needed when the user enables Secure Platform Manager in BIOS
and  KEK and/or SK are configured.

>
> > +
> > +
> > +What:                /sys/class/firmware-attributes/*/authentication/SPM/statusbin
> > +Date:                March 29
> > +KernelVersion:       5.18
> > +Contact:     "Jorge Lopez" <jorge.lopez2@xxxxxx>
> > +Description: 'statusbin' is a read-only file that returns identical status
> > +             information reported by 'status' file in binary format.
>
> How does this binary format work?

Yes.  Status bin is used by GUI applications where data is managed
accordingly instead of individual lines

>
> > +
> > +
> > +What:                /sys/class/firmware-attributes/*/attributes/last_error
> > +Date:                March 29
> > +KernelVersion:       5.18
> > +Contact:     "Jorge Lopez" <jorge.lopez2@xxxxxx>
> > +Description: 'last_error' is a read-only file that returns WMI error number
> > +             and message reported by last WMI command.
>
> Does this provide much value?
> Or could this error just be logged via pr_warn_ratelimited()?

It is specially needed to determine if WMI calls reported an error.
This property is similar to the one provided by both Dell and Lenovo
drivers
>
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f32538373164..663ae73fb8be 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9367,6 +9367,12 @@ S:     Obsolete
> >  W:   http://w1.fi/hostap-driver.html
> >  F:   drivers/net/wireless/intersil/hostap/
> >
> > +HP BIOSCFG DRIVER
> > +M:   Jorge Lopez <jorge.lopez2@xxxxxx>
> > +L:      platform-driver-x86@xxxxxxxxxxxxxxx
>
> Broken whitespace

I will be corrected.

>
> > +S:   Maintained
> > +F:   drivers/platform/x86/hp/hp-bioscfg/
> > +
> >  HP COMPAQ TC1100 TABLET WMI EXTRAS DRIVER
> >  L:   platform-driver-x86@xxxxxxxxxxxxxxx
> >  S:   Orphan
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/Makefile b/drivers/platform/x86/hp/hp-bioscfg/Makefile
> > new file mode 100644
> > index 000000000000..529eba6fa47f
> > --- /dev/null
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/Makefile
> > @@ -0,0 +1,13 @@
> > +obj-$(CONFIG_HP_BIOSCFG) := hp-bioscfg.o
>
> The kbuild part that defines CONFIG_HP_BIOSCFG is missing, so this is
> never built.
>

This is an oversight on my part.  The changes were made but never made
part of the review.

> drivers/platform/x86/hp/Makefile also needs to reference this Makefile.
>
> After fixing up Kbuild please build the driver with "make W=1" and clean
> up all the unused functions/variables.
> (This won't catch unused stuff from bioscfg.c, so you have to check
> these manually)
>

Thank you.  I will make sure to include it