Thanks Jorge

On Tue, May 23, 2023, at 10:16 AM, Jorge Lopez wrote:
> On Fri, May 19, 2023 at 6:19 PM Mark Pearson <[email protected]> wrote:
>>
>> Thanks Jorge,
>>
>> On Fri, May 19, 2023, at 4:58 PM, Jorge Lopez wrote:
>> > On Fri, May 19, 2023 at 12:34 PM Mark Pearson <[email protected]> wrote:
>> >>
>> >> Hi Jorge,
>> >>
>> >> On Wed, May 17, 2023, at 11:50 AM, Jorge Lopez wrote:
>> >> > 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.
>> >> >
>> >> > Many features of HP Commercial notebooks 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 BIOSCFG driver provides
>> >> > a native Linux solution and the exposed features facilitates the
>> >> > migration to Linux environments.
>> >> >
>> >> > The Linux security features to be provided in hp-bioscfg driver enables
>> >> > managing the BIOS settings and security solutions via sysfs, a virtual
>> >> > filesystem that can be used by user-mode applications. The new
>> >> > documentation cover HP-specific firmware sysfs attributes such Secure
>> >> > Platform Management and Sure Start. Each section provides security
>> >> > feature description and identifies sysfs directories and files exposed
>> >> > by the driver.
>> >> >
>> >> > Many HP Commercial notebooks include a feature called Secure Platform
>> >> > Management (SPM), which replaces older password-based BIOS settings
>> >> > management with public key cryptography. PC secure product management
>> >> > begins when a target system is provisioned with cryptographic keys
>> >> > that are used to ensure the integrity of communications between system
>> >> > management utilities and the BIOS.
>> >> >
>> >> > HP Commercial notebooks have several BIOS settings that control its
>> >> > behaviour and capabilities, many of which are related to security.
>> >> > To prevent unauthorized changes to these settings, the system can
>> >> > be configured to use a cryptographic signature-based authorization
>> >> > string that the BIOS will use to verify authorization to modify the
>> >> > setting.
>> >> >
>> >> > Linux Security components are under development and not published yet.
>> >> > The only linux component is the driver (hp bioscfg) at this time.
>> >> > Other published security components are under Windows.
>> >> >
>> >> > Signed-off-by: Jorge Lopez <[email protected]>
>> >> >
>> >> > ---
>> >> > Based on the latest platform-drivers-x86.git/for-next
>> >> > ---
>> >> > .../testing/sysfs-class-firmware-attributes | 102 +++++++++++++++++-
>> >> > 1 file changed, 100 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> >> > b/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> >> > index 4cdba3477176..f8d6c089228b 100644
>> >> > --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> >> > +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> >> <snip>
>> >> > +
>> >> > +
>> >> > + HP specific class extensions - Secure Platform Manager (SPM)
>> >> > + --------------------------------
>> >> > +
>> >> > +What: /sys/class/firmware-attributes/*/authentication/SPM/kek
>> >> > +Date: March 29
>> >> > +KernelVersion: 5.18
>> >> > +Contact: "Jorge Lopez" <[email protected]>
>> >> > +Description:
>> >> > + 'kek' Key-Encryption-Key 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" <[email protected]>
>> >> > +Description:
>> >> > + 'sk' Signature Key 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.
>> >> > +
>> >> > +What: /sys/class/firmware-attributes/*/authentication/SPM/status
>> >> > +Date: March 29
>> >> > +KernelVersion: 5.18
>> >> > +Contact: "Jorge Lopez" <[email protected]>
>> >> > +Description:
>> >> > + 'status' is a read-only file that returns ASCII text in JSON format
>> >> > reporting
>> >> > + the status information.
>> >> > +
>> >> > + "State": "not provisioned | provisioned | provisioning in progress
>> >> > ",
>> >> > + "Version": " Major. Minor ",
>> >> > + "Nonce": <16-bit unsigned number display in base 10>,
>> >> > + "FeaturesInUse": <16-bit unsigned number display in base 10>,
>> >> > + "EndorsementKeyMod": "<256 bytes in base64>",
>> >> > + "SigningKeyMod": "<256 bytes in base64>"
>> >> > +
>> >> > +What: /sys/class/firmware-attributes/*/attributes/Sure_Start/audit_log_entries
>> >> > +Date: March 29
>> >> > +KernelVersion: 5.18
>> >> > +Contact: "Jorge Lopez" <[email protected]>
>> >> > +Description:
>> >> > + 'audit_log_entries' is 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
>> >> > +
>> >> > +What: /sys/class/firmware-attributes/*/attributes/Sure_Start/audit_log_entry_count
>> >> > +Date: March 29
>> >> > +KernelVersion: 5.18
>> >> > +Contact: "Jorge Lopez" <[email protected]>
>> >> > +Description:
>> >> > + 'audit_log_entry_count' is a read-only file that returns the number
>> >> > of existing
>> >> > + audit log events available to be read. Values are separated using
>> >> > comma (``,``)
>> >> > +
>> >> > + [No of entries],[log entry size],[Max number of entries supported]
>> >> > +
>> >> > + log entry size identifies audit log size for the current BIOS
>> >> > version.
>> >> > + The current size is 16 bytes but it can be up to 128 bytes long in
>> >> > future BIOS
>> >> > + versions.
>> >> > --
>> >> > 2.34.1
>> >>
>> >> Firstly apologies as I've done a poor job of following the updates to this series - so if this has already been discussed and agreed by more seasoned kernel contributors please feel free to disregard my comments :) I was catching up on my inbox and had some thoughts.
>> >
>> > No worries.
>> >
>> >>
>> >> For SPM - as this replaces password usage, is it done for all account types? It seems a bit odd having it be a replacement for the passwords but in it's own location and not in the same place as (for example) Admin/current_password.
>> >> For the Lenovo implementation I put certificate, signature and save_signature in the authentication/Admin directory and I realise your implementation is different with the keys but if the kek and sk are only used with the Admin account then shouldn't they also be in that directory? It would be nice to have some commonality across vendors in my opinion.
>> >>
>> >
>> > SPM (Security Platform Management) does not replace password usage and
>> > it is done for the Admin account (Setup Password). SPM is a security
>> > feature available to the user to configure/update BIOS settings when
>> > Sure Admin is enabled in BIOS. One of the files provided under SPM is
>> > the key _role and its value is set to ‘bios-spm.’ A ‘bios-spm’ role
>> > indicates a password is used in combination with an
>> > endorsement/signing key. It is also the reason why SPM is kept
>> > separate under the list of authentication attributes.
>>
>> Ah - I think I get it, and in that case I can see why you keep it separate and it is a unique role.
>> I withdraw my suggestion :)
>>
>> >
>> >> For the Sure_Start I would propose de-branding this so it's generic and I don't think it fits under attributes as it doesn't support any of the other required attribute fields. I think your implementation of an audit log seems neat but if another vendor was to do similar it would be better to be able to reuse the same attribute name and enable common tooling.
>> >> I propose having this as just log/audit_entries and log/audit_count and have the log folder in the top alongside authentication and attributes.
>> >> If someone wants to add other logs in the future it would be a good place to have them.
>> >
>> > I like the idea of having a log folder alongside authentication and
>> > attributes where future logs can be placed. Unfortunately, Sure_Start
>> > is part of the security attributes and the initial driver provides a
>> > minimal implementation. Other attributes associated with Sure_Start
>> > and available in BIOS are 'SureStart Production Mode', 'Sure Start
>> > Security Event Boot Notification', 'Sure Start BIOS Settings
>> > Protection', 'Sure Start Secure Boot Keys Protection', and 'Sure Start
>> > Security Event Policy'
>>
>> Aren't all of these just regular attributes that show up under the attributes folder (and have current_value and possible_value settings etc)? Do they get created in your equivalent of probing the BIOS to determine what attributes are available or are they manually created entries? (I'm assuming automatic or they would be also added to the documentation)
>
> 'SureStart Production Mode', 'Sure Start Security Event Boot
> Notification', 'Sure Start BIOS Settings Protection', 'Sure Start
> Secure Boot Keys Protection', and 'Sure Start Security Event Policy'
> are reported by BIOS therefore listed under attributes. 'Sure
> Start/audit_log_entries and Sure_Start/audit_log_count_entries are
> created manually and kept under attributes/Sure_Start; same location
> as all other 'Sure Start' entries.
>
Thanks for the clarification

>>
>> It seemed to me the audit log entries were being handled differently because you had to specifically create the entries, with (I assume) some special handling for display?
>
> Gathering the number of entries and reading each audit logs require
> making multiple WMI calls (Security related). If 'Sure Start' was not
> part of a security feature, I would agree to move it under a log
> directory; sadly it is not the case.

OK - I'll defer to the more experienced kernel folk; but to me I think the argument still stands that these two fields are not standard attributes (they won't have any of the regular attribute fields) and by being in a directory they look as if they are trying to be. My vote would still be to:

- make them non vendor trademark specific so it could be used by other vendors for similar in the future.
- move them out of the attributes folder as they seem a bad fit to me

Thanks for all the insights - appreciated :)
Mark