| Message ID | 20230403211548.6253-1-jorge.lopez2@hp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v7] Introduction-of-HP-BIOSCFG-driver-documentation | expand |
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.
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/20230309201022.9502-1-jorge.lopez2@hp.com/ 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 <jorge.lopez2@hp.com> > > --- > 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" <jorge.lopez2@hp.com> > +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" <jorge.lopez2@hp.com> > +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" <jorge.lopez2@hp.com> > +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" <jorge.lopez2@hp.com> > +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" <jorge.lopez2@hp.com> > > +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" <jorge.lopez2@hp.com> > > +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 <jorgealtxwork@gmail.com>: > 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" <jorge.lopez2@hp.com> >>> +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" <jorge.lopez2@hp.com> >>> +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 <thomas@t-8ch.de> wrote: > > Hi Jorge! > > > Apr 4, 2023 22:24:36 Jorge Lopez <jorgealtxwork@gmail.com>: > > > 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" <jorge.lopez2@hp.com> > >>> +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" <jorge.lopez2@hp.com> 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" <jorge.lopez2@hp.com> > >>> +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 <hdegoede@redhat.com> 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 > > >
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" <jorge.lopez2@hp.com> +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" <jorge.lopez2@hp.com> +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" <jorge.lopez2@hp.com> +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" <jorge.lopez2@hp.com> +Description: 'statusbin' is a read-only file that returns identical status + information reported by 'status' file in binary format.
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 <jorge.lopez2@hp.com> --- 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(-)