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

[Jorge Lopez]

Hi Mark,

Please see my comments below.

On Sat, Apr 1, 2023 at 7:48 PM Mark Pearson <mpearson-lenovo@xxxxxxxxx> wrote:
>
> Hi Jorge,
>
> As I implemented similar on our platforms I have a couple of suggestions which may or may not be helpful.
>
> On Sat, Apr 1, 2023, at 7:58 AM, Thomas Weißschuh wrote:
> > Hi Jorge,
> >
> <snip>
> > On 2023-03-09 14:10:22-0600, Jorge Lopez wrote:
> <snip>
> >
> >> Many features of HP Commercial PC’s can be managed using Windows
> >> Management Instrumentation (WMI). WMI is an implementation of Web-Based
> >> Enterprise Management (WBEM) that provides a standards-based interface
> >> for changing and monitoring system settings.  HP BISOCFG driver provides
> >> a native Linux solution and the exposed features facilitates the
> >> migration to Linux environments.
>
> I'd remove this paragraph personally - but as a minor note, typo in BISOCFG
>
> <snip>
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> >> index 4cdba3477176..d1ae6b77da13 100644
> >> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> >> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> <snip>
> >> @@ -126,6 +133,38 @@ Description:
> >>                                      value will not be effective through sysfs until this rule is
> >>                                      met.
> >>
> >> +            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.
>
> isn't this already covered in the 'possible_values' attribute - it's just a string of items? Curious as to when/how this would be used instead of possible_values (but I should probably read the code)
> Typo in 'hightest'.

Done.  Possible values provides a list of values in any order.
elements in Ordered-list list items in level of priority such it is
case of list of boot order values.
>
> <snip>
> >
> >> +
> >> +
> >>  What:               /sys/class/firmware-attributes/*/authentication/
> >>  Date:               February 2021
> >>  KernelVersion:      5.11
> >> @@ -206,7 +245,7 @@ Description:
> <snip>
> >> @@ -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).
> >> +
>
> Your implementation might be different on HP's; but on the Lenovo's this was still used along with the regular roles - it's just the authentication changed from password to a signature approach.
>
> Just checking that you really need a whole new role and that it isn't part of the existing role.
>
Unfortunately, we need a whole new role.

> <snip>
>
> >> +            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.
> >
>
> I wondered if these could be combined with the signature and certificate fields that I implemented for the Lenovo platforms - and those be moved out of the Lenovo specific section and then made general (and optional)
The behavior with Secure Platform Manager requires having KEK and SK separate.

> kek looks like it corresponds to certificate and sk to signature?
> KEK - Key-Encryption-Key
      SK - Signature Key
> >


> >> +
> >> +
> >> +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()?
>
> This one seemed odd to me too - doesn't the driver return the error to the use on a failed WMI access?
>
It was intended for debug purposes and to determine if the failure was
reported because of WMI error.  The WMI error is masked by the driver
and the error reported by WMI is lost.
for instance,   WMI error 6 is reported by driver as  -EINVAL.
This attribute will be removed and replaced by pr_warn().

Jorge.