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 features 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.
Signed-off-by: Jorge Lopez <[email protected]>
---
Based on the latest platform-drivers-x86.git/for-next
History
Version 7
Includes only sysfs-class-firmware-attributes documentation
Version 6
Breaks down the changes into 4 patches
SureAdmin-attributes was removed
Version 5
Remove version 4 patch 1
Address review changes proposed in Version 4
Reorganize all patches number and file order
---
.../testing/sysfs-class-firmware-attributes | 119 ++++++++++++++++--
1 file changed, 107 insertions(+), 12 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 4cdba3477176..10afbb78baec 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -22,6 +22,12 @@ Description:
- integer: a range of numerical values
- string
+ HP specific types
+ -----------------
+ - ordered-list - a set of ordered list valid values
+ - sure-start
+
+
All attribute types support the following values:
current_value:
@@ -42,16 +48,16 @@ Description:
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>
+ A file that can be read to obtain
+ the IETF language tag corresponding to the
+ "display_name" of the <attr>
"enumeration"-type specific properties:
possible_values:
- A file that can be read to obtain the possible
- values of the <attr>. Values are separated using
- semi-colon (``;``).
+ A file that can be read to obtain the possible
+ values of the <attr>. Values are separated using
+ semi-colon (``;``).
"integer"-type specific properties:
@@ -64,8 +70,8 @@ Description:
bound value of the <attr>
scalar_increment:
- A file that can be read to obtain the scalar value used for
- increments of current_value this attribute accepts.
+ A file that can be read to obtain the scalar value used for
+ increments of current_value this attribute accepts.
"string"-type specific properties:
@@ -126,6 +132,40 @@ 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
+ highest 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.
+ Values are separated using semi-colon (``;``)
+
+ 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.
+ Values are separated using comma (``,``)
+
+ [No of entries],[log entry size],[Max number of entries supported]
+
+
What: /sys/class/firmware-attributes/*/authentication/
Date: February 2021
KernelVersion: 5.11
@@ -139,8 +179,7 @@ Description:
For example a "BIOS Admin" password and "System" Password can be set,
reset or cleared using these attributes.
- - An "Admin" password is used for preventing modification to the BIOS
- settings.
+ - An "Admin" password is used for preventing modification to the BIOS settings.
- A "System" password is required to boot a machine.
Change in any of these two authentication methods will also generate an
@@ -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
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
@@ -311,7 +359,7 @@ Description:
== =========================================
0 All BIOS attributes setting are current
1 A reboot is necessary to get pending BIOS
- attribute changes applied
+ attribute changes applied
== =========================================
Note, userspace applications need to follow below steps for efficient
@@ -364,3 +412,50 @@ 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 - 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 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):
+
+
+What: /sys/class/firmware-attributes/*/authentication/SPM/statusbin
+Date: March 29
+KernelVersion: 5.18
+Contact: "Jorge Lopez" <[email protected]>
+Description: 'statusbin' is a read-only file that returns identical status
+ information reported by 'status' file in binary format.
--
2.34.1
On Mon, Apr 03, 2023 at 04:15:48PM -0500, 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 features 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.
If this is single patch, I'd like to write the patch subject as
"Documentation: sysfs: document HP-specific firmware attributes".
And also, adjust the patch description accordingly, since as it is
written above, it looks like general documentation of HP-specific feature
(which should be in actual diff).
> Version 7
> Includes only sysfs-class-firmware-attributes documentation
Where is the rest of patches if this is a series? Had they been merged?
Thanks.
--
An old man doll... just what I always wanted! - Clara
Hi Bagas,
>
> Where is the rest of patches if this is a series? Had they been merged?
>
There is only one change as requested by Thomas Weißschuh
> Feel free to ONLY submit the patch with the documentation for the next
> revision. Then we can nail down the interface and initial functionality
> and you don't always have to adapt the code to the changing interface.
Perhaps, I misunderstood Thomas request. I will address a few other
comments and will submit all files again.
Regards,
Jorge
Hi Bagas,
On 2023-04-04 09:33:53+0700, Bagas Sanjaya wrote:
> On Mon, Apr 03, 2023 at 04:15:48PM -0500, 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 features 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.
>
> If this is single patch, I'd like to write the patch subject as
> "Documentation: sysfs: document HP-specific firmware attributes".
>
> And also, adjust the patch description accordingly, since as it is
> written above, it looks like general documentation of HP-specific feature
> (which should be in actual diff).
>
> > Version 7
> > Includes only sysfs-class-firmware-attributes documentation
>
> Where is the rest of patches if this is a series? Had they been merged?
It was my proposal to focus on the documentation first in a single
patch.
So we can nail down the scope and details of the user-facing API without
Jorge and the reviewers spending time on polishing internals that will
change anyways.
The code exists and will be submitted with future revisions again.
You can find v6 with the code here:
https://lore.kernel.org/all/[email protected]/
I should have also requested a note to that point with this revision.
Thomas
Hi Jorge!
Comments inline.
Please also Cc reviewers of past revisions to future revisions.
On 2023-04-03 16:15:48-0500, 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 features 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.
>
> Signed-off-by: Jorge Lopez <[email protected]>
>
> ---
> Based on the latest platform-drivers-x86.git/for-next
>
> History
>
> Version 7
> Includes only sysfs-class-firmware-attributes documentation
>
> Version 6
> Breaks down the changes into 4 patches
> SureAdmin-attributes was removed
>
> Version 5
> Remove version 4 patch 1
> Address review changes proposed in Version 4
> Reorganize all patches number and file order
> ---
> .../testing/sysfs-class-firmware-attributes | 119 ++++++++++++++++--
> 1 file changed, 107 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> index 4cdba3477176..10afbb78baec 100644
> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> @@ -22,6 +22,12 @@ Description:
> - integer: a range of numerical values
> - string
>
> + HP specific types
> + -----------------
> + - ordered-list - a set of ordered list valid values
> + - sure-start
Could you explain what "sure-start" does?
Is it actually an attribute type of which multiple attributes can exist?
Or are there just some global properties that need to be exposed?
If it is global it should be directly under
/sys/class/firmware-attributes/*/authentication/
without needing the type.
> +
> +
> All attribute types support the following values:
>
> current_value:
> @@ -42,16 +48,16 @@ Description:
> 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>
> + A file that can be read to obtain
> + the IETF language tag corresponding to the
> + "display_name" of the <attr>
Are these reindentations and other cleanups intentional?
If they are intentional and there are no interactions with your actual
patch you could split them into their own patch and submit them
separately.
This way we wouldn't have to worry about them here anymore.
Note:
These indentations are different from the newly introduced documentation.
>
> "enumeration"-type specific properties:
>
> possible_values:
> - A file that can be read to obtain the possible
> - values of the <attr>. Values are separated using
> - semi-colon (``;``).
> + A file that can be read to obtain the possible
> + values of the <attr>. Values are separated using
> + semi-colon (``;``).
>
> "integer"-type specific properties:
>
> @@ -64,8 +70,8 @@ Description:
> bound value of the <attr>
>
> scalar_increment:
> - A file that can be read to obtain the scalar value used for
> - increments of current_value this attribute accepts.
> + A file that can be read to obtain the scalar value used for
> + increments of current_value this attribute accepts.
>
> "string"-type specific properties:
>
> @@ -126,6 +132,40 @@ 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
> + highest 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.
> + Values are separated using semi-colon (``;``)
> +
> + Audit log entry format
> +
> + Byte 0-15: Requested Audit Log entry (Each Audit log is 16 bytes)
> + Byte 16-127: Unused
How to interpret each log entry?
If it is an opaque thing from the firmware that would also be useful to
know.
> +
> + audit_log_entry_count:
> + 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]
Will log entry size always be 16? Or can it be bigger in the future when
more bytes are used?
This should be mentioned.
Is audit_log_entry_count ever used without reading audit_log_entries
right after?
If not the count file could be dropped.
> +
> +
> What: /sys/class/firmware-attributes/*/authentication/
> Date: February 2021
> KernelVersion: 5.11
> @@ -139,8 +179,7 @@ Description:
> For example a "BIOS Admin" password and "System" Password can be set,
> reset or cleared using these attributes.
>
> - - An "Admin" password is used for preventing modification to the BIOS
> - settings.
> + - An "Admin" password is used for preventing modification to the BIOS settings.
> - A "System" password is required to boot a machine.
>
> Change in any of these two authentication methods will also generate an
> @@ -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
> 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
> @@ -311,7 +359,7 @@ Description:
> == =========================================
> 0 All BIOS attributes setting are current
> 1 A reboot is necessary to get pending BIOS
> - attribute changes applied
> + attribute changes applied
> == =========================================
>
> Note, userspace applications need to follow below steps for efficient
> @@ -364,3 +412,50 @@ 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 - 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 reporting
> + the status information.
> +
> + State: Not Provisioned / Provisioned / Provisioning in progress
> + Version: Major. Minor
> + Feature Bit Mask: <16-bit unsigned number display in hex>
How are these bits to be interpreted?
> + SPM Counter: <16-bit unsigned number display in base 10>
> + Signing Key Public Key Modulus (base64):
> + KEK Public Key Modulus (base64):
Is " (base64)" supposed to be part of the contents of the file?
> +
> +
> +What: /sys/class/firmware-attributes/*/authentication/SPM/statusbin
> +Date: March 29
> +KernelVersion: 5.18
> +Contact: "Jorge Lopez" <[email protected]>
> +Description: 'statusbin' is a read-only file that returns identical status
> + information reported by 'status' file in binary format.
This documentation should contain enough information to understand the
files contents.
I understand that one WMI call will return all the fields that are part
of the "status" and "statusbin" in one response.
Are these WMI calls especially expensive or called especially
frequently?
If not I would still argue to split them into one file per field and
remove the statusbin file.
It is much more intuitive to read for example the KEK from the file
"kek" where it is also updated from.
This interface will be used by various applications and it is much
easier to read simple files than having to parse single fields from
bespoke text or binary files.
Thomas
Hi Thomas,
BTW, I decided to submit all files individually to facilitate the
review process. Only Makefile and Kconfig files will be provided as a
single patch.
I will be out of town until April 11 and will reply back upon my return.
Please see my comments below.
>
> > + HP specific types
> > + -----------------
> > + - ordered-list - a set of ordered list valid values
> > + - sure-start
>
> Could you explain what "sure-start" does?
> Is it actually an attribute type of which multiple attributes can exist?
>
It is an attribute type of which multiple attributes can exist.
At this moment Sure-Start reports both the number of audit logs and
all logs reported by BIOS.
Sure-Start is exposed directly under
/sys/class/firmware-attributes/*/attributes/. Sure Start does not
provide any authentication.
> Or are there just some global properties that need to be exposed?
> If it is global it should be directly under
> /sys/class/firmware-attributes/*/authentication/
> without needing the type.
>
> > +
> > +
> > All attribute types support the following values:
> >
> > current_value:
> > @@ -42,16 +48,16 @@ Description:
> > 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>
> > + A file that can be read to obtain
> > + the IETF language tag corresponding to the
> > + "display_name" of the <attr>
>
> Are these reindentations and other cleanups intentional?
>
> If they are intentional and there are no interactions with your actual
> patch you could split them into their own patch and submit them
> separately.
>
> This way we wouldn't have to worry about them here anymore.
They were unintentionally. I will reset them back in the next review
>
> Note:
> These indentations are different from the newly introduced documentation.
>
> >
> > + audit_log_entries:
> > + A read-only file that returns the events in the log.
> > + Values are separated using semi-colon (``;``)
> > +
> > + Audit log entry format
> > +
> > + Byte 0-15: Requested Audit Log entry (Each Audit log is 16 bytes)
> > + Byte 16-127: Unused
>
> How to interpret each log entry?
>
Byte 0: status
1: event id
2: msg number
3: severity
4: source ID
5: system state at event
6-12 Time stamp
13-15: internal buffer data
Application needs to have knowledge of the data provided by BIOS in
order to interpret the audit log.
> If it is an opaque thing from the firmware that would also be useful to
> know.
>
> > +
> > + audit_log_entry_count:
> > + 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]
>
> Will log entry size always be 16? Or can it be bigger in the future when
> more bytes are used?
> This should be mentioned.
Log entry size is always 16 bytes in size. The reason is to report a
maximum of 256 entries. Total 4096 bytes
>
> Is audit_log_entry_count ever used without reading audit_log_entries
> right after?
Yes. The counter is necessary to determine how many logs are available
to be read.
> If not the count file could be dropped.
>
> > +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 reporting
> > + the status information.
> > +
> > + State: Not Provisioned / Provisioned / Provisioning in progress
> > + Version: Major. Minor
> > + Feature Bit Mask: <16-bit unsigned number display in hex>
>
> How are these bits to be interpreted?
This information is provided by BIOS. It is one of those obscure
values from BIOS.
>
> > + SPM Counter: <16-bit unsigned number display in base 10>
> > + Signing Key Public Key Modulus (base64):
> > + KEK Public Key Modulus (base64):
>
> Is " (base64)" supposed to be part of the contents of the file?
The information reported for Signing Key and KEK public key are
reported as base64 values. It applies only to the data and not to the
file contents.
>
> > +
> > +
> > +What: /sys/class/firmware-attributes/*/authentication/SPM/statusbin
> > +Date: March 29
> > +KernelVersion: 5.18
> > +Contact: "Jorge Lopez" <[email protected]>
> > +Description: 'statusbin' is a read-only file that returns identical status
> > + information reported by 'status' file in binary format.
>
> This documentation should contain enough information to understand the
> files contents.
>
>
> I understand that one WMI call will return all the fields that are part
> of the "status" and "statusbin" in one response.
>
> Are these WMI calls especially expensive or called especially
> frequently?
>
Unfortunately the WMI to read the Status binary data is expensive
hence the reason of only calling once.
> If not I would still argue to split them into one file per field and
> remove the statusbin file.
>
Regards,
Jorge
Hi All,
FWIW I have been reading along with both the v6 and
this v7 posting.
Thomas, Mark, thank you for your review / feedback
on this series.
On 4/4/23 15:37, Jorge Lopez wrote:
> Hi Bagas,
>
>>
>> Where is the rest of patches if this is a series? Had they been merged?
>>
>
> There is only one change as requested by Thomas Weißschuh
>
>> Feel free to ONLY submit the patch with the documentation for the next
>> revision. Then we can nail down the interface and initial functionality
>> and you don't always have to adapt the code to the changing interface.
>
> Perhaps, I misunderstood Thomas request. I will address a few other
> comments and will submit all files again.
I think that Thomas' suggestion to first focus on getting
the userspace API right and then implement the agreed
upon API is a good idea.
So for the next version just post only the documentation
patch again please.
Note it is not the intention to merge just the documentation
patch without merging the code first.
The idea is to first nail the API down and then modify
the code once to implement the agreed upon API.
Then once the API has been agreed upon post newer
versions which also include the code again.
And then once people are also happy with the code
we can merge both the code + documentation in one go.
Regards,
Hans
>
> Regards,
>
> Jorge
>
Hi Jorge!
Apr 4, 2023 22:24:36 Jorge Lopez <[email protected]>:
> Hi Thomas,
>
> BTW, I decided to submit all files individually to facilitate the
> review process. Only Makefile and Kconfig files will be provided as a
> single patch.
> I will be out of town until April 11 and will reply back upon my return.
Sorry for the slow response.
>
> Please see my comments below.
>
>>
>>> + HP specific types
>>> + -----------------
>>> + - ordered-list - a set of ordered list valid values
>>> + - sure-start
>>
>> Could you explain what "sure-start" does?
>> Is it actually an attribute type of which multiple attributes can exist?
>>
>
> It is an attribute type of which multiple attributes can exist.
> At this moment Sure-Start reports both the number of audit logs and
> all logs reported by BIOS.
> Sure-Start is exposed directly under
> /sys/class/firmware-attributes/*/attributes/. Sure Start does not
> provide any authentication.
But what does it mean?
For ordered-list there is a nice explanation.
>
>> Or are there just some global properties that need to be exposed?
>> If it is global it should be directly under
>> /sys/class/firmware-attributes/*/authentication/
>> without needing the type.
>>
>>> +
>>> +
>>> All attribute types support the following values:
>>>
>>> current_value:
>>> @@ -42,16 +48,16 @@ Description:
>>> 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>
>>> + A file that can be read to obtain
>>> + the IETF language tag corresponding to the
>>> + "display_name" of the <attr>
>>
>> Are these reindentations and other cleanups intentional?
>>
>> If they are intentional and there are no interactions with your actual
>> patch you could split them into their own patch and submit them
>> separately.
>>
>> This way we wouldn't have to worry about them here anymore.
>
> They were unintentionally. I will reset them back in the next review
>>
>> Note:
>> These indentations are different from the newly introduced documentation.
>>
>>>
>>> + audit_log_entries:
>>> + A read-only file that returns the events in the log.
>>> + Values are separated using semi-colon (``;``)
>>> +
>>> + Audit log entry format
>>> +
>>> + Byte 0-15: Requested Audit Log entry (Each Audit log is 16 bytes)
>>> + Byte 16-127: Unused
>>
>> How to interpret each log entry?
>>
>
> Byte 0: status
> 1: event id
> 2: msg number
> 3: severity
> 4: source ID
> 5: system state at event
> 6-12 Time stamp
> 13-15: internal buffer data
>
> Application needs to have knowledge of the data provided by BIOS in
> order to interpret the audit log.
>
>> If it is an opaque thing from the firmware that would also be useful to
>> know.
>>
>>> +
>>> + audit_log_entry_count:
>>> + 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]
>>
>> Will log entry size always be 16? Or can it be bigger in the future when
>> more bytes are used?
>> This should be mentioned.
>
> Log entry size is always 16 bytes in size. The reason is to report a
> maximum of 256 entries. Total 4096 bytes
Does it make sense to expose the number 16 from sysfs if it never can change anyways?
Note you can also customize the filesize reported in sysfs to expose the maximum size to be used by userspace.
>>
>> Is audit_log_entry_count ever used without reading audit_log_entries
>> right after?
> Yes. The counter is necessary to determine how many logs are available
> to be read.
I think the cleaner interface would be to have users provide a buffer to read into and then they check the return value of read().
Users can't trust the count value anyways as it is prone to TOCTOU races.
>
>> If not the count file could be dropped.
>>
>>> +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 reporting
>>> + the status information.
>>> +
>>> + State: Not Provisioned / Provisioned / Provisioning in progress
>>> + Version: Major. Minor
>>> + Feature Bit Mask: <16-bit unsigned number display in hex>
>>
>> How are these bits to be interpreted?
> This information is provided by BIOS. It is one of those obscure
> values from BIOS.
>>
>>> + SPM Counter: <16-bit unsigned number display in base 10>
>>> + Signing Key Public Key Modulus (base64):
>>> + KEK Public Key Modulus (base64):
>>
>> Is " (base64)" supposed to be part of the contents of the file?
>
> The information reported for Signing Key and KEK public key are
> reported as base64 values. It applies only to the data and not to the
> file contents.
Put is the file format:
KEK Public Key Modulus (base64): ...
KEK Public Key Modulus: ...
The docs indicate the former.
>>
>>> +
>>> +
>>> +What: /sys/class/firmware-attributes/*/authentication/SPM/statusbin
>>> +Date: March 29
>>> +KernelVersion: 5.18
>>> +Contact: "Jorge Lopez" <[email protected]>
>>> +Description: 'statusbin' is a read-only file that returns identical status
>>> + information reported by 'status' file in binary format.
>>
>> This documentation should contain enough information to understand the
>> files contents.
>>
>>
>> I understand that one WMI call will return all the fields that are part
>> of the "status" and "statusbin" in one response.
>>
>> Are these WMI calls especially expensive or called especially
>> frequently?
>>
>
> Unfortunately the WMI to read the Status binary data is expensive
> hence the reason of only calling once.
Hm, I still dislike the interface, sorry.
What about caching the values in the driver and exposing them via different files?
>> If not I would still argue to split them into one file per field and
>> remove the statusbin file.
>>
>
> Regards,
> > Jorge
Hi Thomas,
On Tue, Apr 11, 2023 at 11:16 AM Thomas Weißschuh <[email protected]> wrote:
>
> Hi Jorge!
>
>
> Apr 4, 2023 22:24:36 Jorge Lopez <[email protected]>:
>
> > Hi Thomas,
> >
> > BTW, I decided to submit all files individually to facilitate the
> > review process. Only Makefile and Kconfig files will be provided as a
> > single patch.
> > I will be out of town until April 11 and will reply back upon my return.
>
> Sorry for the slow response.
>
> >
> > Please see my comments below.
> >
> >>
> >>> + HP specific types
> >>> + -----------------
> >>> + - ordered-list - a set of ordered list valid values
> >>> + - sure-start
> But what does it mean?
> For ordered-list there is a nice explanation.
>
The latest submission includes a short explanation
HP specific types
-----------------
- ordered-list - a set of ordered list valid values
- sure-start - report audit logs read from BIOS
> >>> + audit_log_entry_count:
> >>> + 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]
> >>
> >> Will log entry size always be 16? Or can it be bigger in the future when
> >> more bytes are used?
> >> This should be mentioned.
> >
> > Log entry size is always 16 bytes in size. The reason is to report a
> > maximum of 256 entries. Total 4096 bytes
>
> Does it make sense to expose the number 16 from sysfs if it never can change anyways?
The current audit log file is 16 bytes but future BIOS can expand the
audit log to up to 128 bytes.
Additional details are now included in the audit_log_entry_count
section. WMI reports only the total number of audit logs but not the
individual audit log size hence the reason the size is hardcoded to 16
bytes in the driver. A patch will be provided when the audit log size
changes or a new WMI is provided.
>
> Note you can also customize the filesize reported in sysfs to expose the maximum size to be used by userspace.
>
This is another area with additional details in the latest review.
"sure-start"-type specific properties:
audit_log_entries:
A read-only file that returns
the events in the log.
Values are separated using
semi-colon (``;``)
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.
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 to up to 128 bytes long
in future BIOS versions.
> >>
> >> Is audit_log_entry_count ever used without reading audit_log_entries
> >> right after?
> > Yes. The counter is necessary to determine how many logs are available
> > to be read.
>
> I think the cleaner interface would be to have users provide a buffer to read into and then they check the return value of read().
> Users can't trust the count value anyways as it is prone to TOCTOU races.
Audit logs buffer information includes an audit log number associated
with each log entry. The log number information is added by the
driver. The audit log is read as a single continuous buffer and the
application does not read individual audit logs while reading sysfs.
The application copies the whole buffer, extracts and identifies each
audit log from its local buffer.
Byte 0: Audit log number
Byte 1-16: Audit log information
>
> >
> >> If not the count file could be dropped.
> >>
> >>> +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 reporting
> >>> + the status information.
> >>> +
> >>> + State: Not Provisioned / Provisioned / Provisioning in progress
> >>> + Version: Major. Minor
> >>> + Feature Bit Mask: <16-bit unsigned number display in hex>
> >>
> >> How are these bits to be interpreted?
> > This information is provided by BIOS. It is one of those obscure
> > values from BIOS.
> >>
> >>> + SPM Counter: <16-bit unsigned number display in base 10>
> >>> + Signing Key Public Key Modulus (base64):
> >>> + KEK Public Key Modulus (base64):
> >>
> >> Is " (base64)" supposed to be part of the contents of the file?
> >
> > The information reported for Signing Key and KEK public key are
> > reported as base64 values. It applies only to the data and not to the
> > file contents.
>
> Put is the file format:
> KEK Public Key Modulus (base64): ...
> KEK Public Key Modulus: ...
>
> The docs indicate the former.
Also included in the last review is additional clarification on how
the Modules are reported.
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 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): <256 bytes
base64 in hex>
KEK Public Key Modulus (base64): <256 bytes base64 in hex>
>
> >>
> >>> +
> >>> +
> >>> +What: /sys/class/firmware-attributes/*/authentication/SPM/statusbin
> >>> +Date: March 29
> >>> +KernelVersion: 5.18
> >>> +Contact: "Jorge Lopez" <[email protected]>
> >>> +Description: 'statusbin' is a read-only file that returns identical status
> >>> + information reported by 'status' file in binary format.
> >>
> >> This documentation should contain enough information to understand the
> >> files contents.
> >>
> >>
> >> I understand that one WMI call will return all the fields that are part
> >> of the "status" and "statusbin" in one response.
> >>
> >> Are these WMI calls especially expensive or called especially
> >> frequently?
> >>
> >
> > Unfortunately the WMI to read the Status binary data is expensive
> > hence the reason of only calling once.
>
> Hm, I still dislike the interface, sorry.
No worries. The implementation is a legacy from the Windows driver
and security applications define it.
> What about caching the values in the driver and exposing them via different files?
The information changes as a group during the SPM configuration
process so caching would not help much. Version field is the only
field that can be cached.
If the information is split in difference files, there are conditions
when the user would read stale data in one field or another.
> >
> > Regards,
> > > Jorge
>
Hi there,
Version 9 review for the sysfs-class-firmware-attributes document was
submitted and I look forward to getting your comments. I understand
you have other things happening, but as soon as the document is
finalized, I can move to submit the rest of the files.
While the document review is ongoing, I proceeded to address all
comments made by Thomas, Hans, Bagas, and Mark regarding both
documentation and source code changes. The code compiles with W=1
flag without any issues and the refactoring proposed by Thomas was
incorporated. Testing on multiple platforms continues to validate the
driver proper operation between reviews.
Regards,
Jorge
On Thu, Apr 6, 2023 at 9:08 AM Hans de Goede <[email protected]> wrote:
>
> Hi All,
>
> FWIW I have been reading along with both the v6 and
> this v7 posting.
>
> Thomas, Mark, thank you for your review / feedback
> on this series.
>
> On 4/4/23 15:37, Jorge Lopez wrote:
> > Hi Bagas,
> >
> >>
> >> Where is the rest of patches if this is a series? Had they been merged?
> >>
> >
> > There is only one change as requested by Thomas Weißschuh
> >
> >> Feel free to ONLY submit the patch with the documentation for the next
> >> revision. Then we can nail down the interface and initial functionality
> >> and you don't always have to adapt the code to the changing interface.
> >
> > Perhaps, I misunderstood Thomas request. I will address a few other
> > comments and will submit all files again.
>
> I think that Thomas' suggestion to first focus on getting
> the userspace API right and then implement the agreed
> upon API is a good idea.
>
> So for the next version just post only the documentation
> patch again please.
>
> Note it is not the intention to merge just the documentation
> patch without merging the code first.
>
> The idea is to first nail the API down and then modify
> the code once to implement the agreed upon API.
>
> Then once the API has been agreed upon post newer
> versions which also include the code again.
>
> And then once people are also happy with the code
> we can merge both the code + documentation in one go.
>
> Regards,
>
> Hans
>
>
>
>
>
>
>
> >
> > Regards,
> >
> > Jorge
> >
>