Hi Thomas, Please see my comments below. On Sat, Apr 1, 2023 at 6:58 AM Thomas Weißschuh <thomas@xxxxxxxx> wrote: > > Hi Jorge, > > Hans asked me to do a review of your series, so this is it. > > I'll start with patch 4 because it is the one with the docs and build > system changes. > Reviews of the other patches and the code of this patch will follow. > > In my opinion the best way forward is to drop some of the non-core > and duplicated functionality. > The reduced scope will make review and rework easier and therefore speed > up the process. > > Please also Cc the general kernel mailing list > linux-kernel@xxxxxxxxxxxxxxx for future revisions. > This will make sure the patchset is picked up and tested by the bots. > Will do. > On 2023-03-09 14:10:22-0600, Jorge Lopez wrote: > > The purpose for this patch is submit HP BIOSCFG driver to be list of > > HP Linux kernel drivers. The driver include a total of 12 files > > broken in several patches. > > No need for this paragraph. I will remove it in the next submission. > > > HP BIOS Configuration driver purpose is to provide a driver supporting > > the latest sysfs class firmware attributes framework allowing the user > > to change BIOS settings and security solutions on HP Inc.’s commercial > > notebooks. > > Here it says "notebooks", below "PC's". Does it also support > non-notebook machines? The initial release of the driver will be supported for business notebooks. Although the driver is not targeted for non-notebooks machines, the driver was tested on non-notebooks in the event a decision is made to targets them > > --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes > > +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes > > @@ -22,6 +22,13 @@ Description: > > - integer: a range of numerical values > > - string > > > > + HP specific types > > + ----------------- > > + - ordered-list - a set of ordered list valid values > > + - sure-admin > > Does sure-admin still exist? I will remove that entry. Sure-admin no longer exist as part of the driver >> > + HP specific class extensions > > + ------------------------------ > > + > > + On HP systems the following additional attributes are available: > > + > > + "ordered-list"-type specific properties: > > + > > + elements: > > + A file that can be read to obtain the possible > > + list of values of the <attr>. Values are separated using > > + semi-colon (``;``). The order individual elements are listed > > + according to their priority. An Element listed first has the > > + hightest priority. Writing the list in a different order to > > + current_value alters the priority order for the particular > > + attribute. > > + > > + "sure-start"-type specific properties: > > + > > + audit_log_entries: > > + A read-only file that returns the events in the log. > > + > > + Audit log entry format > > + > > + Byte 0-15: Requested Audit Log entry (Each Audit log is 16 bytes) > > + Byte 16-127: Unused > > + > > + audit_log_entry_count: > > + A read-only file that returns the number of existing audit log events available to be read. > > + > > + [No of entries],[log entry size],[Max number of entries supported] > > sysfs is based on the idea of "one-value-per-file". > The two properties above violate this idea. > Maybe a different interface is needed. > Both properties report a single string separated by semicolon. This is not different from listing all elements in a single string separated by semicolon. > Are these properties very important for the first version of this > driver? If not I would propose to drop them for now and resubmit them > as separate patches after the main driver has been merged. > We want the initial driver to have all predefined properties available first. There are plans to add future properties and features which will be submitted as patches. > > + > > + > > What: /sys/class/firmware-attributes/*/authentication/ > > Date: February 2021 > > KernelVersion: 5.11 > > @@ -206,7 +245,7 @@ Description: > > Drivers may emit a CHANGE uevent when a password is set or unset > > userspace may check it again. > > > > - On Dell and Lenovo systems, if Admin password is set, then all BIOS attributes > > + On Dell, Lenovo, and HP systems, if Admin password is set, then all BIOS attributes > > No comma after "Lenovo" Will do > > > require password validation. > > On Lenovo systems if you change the Admin password the new password is not active until > > the next boot. > > @@ -296,6 +335,15 @@ Description: > > echo "signature" > authentication/Admin/signature > > echo "password" > authentication/Admin/certificate_to_password > > > > + HP specific class extensions > > + -------------------------------- > > + > > + On HP systems the following additional settings are available: > > + > > + role: enhanced-bios-auth: > > + This role is specific to Secure Platform Management (SPM) attribute. > > + It requires configuring an endorsement (kek) and signing certificate (sk). > > + > > > > What: /sys/class/firmware-attributes/*/attributes/pending_reboot > > Date: February 2021 > > @@ -364,3 +412,60 @@ Description: > > use it to enable extra debug attributes or BIOS features for testing purposes. > > > > Note that any changes to this attribute requires a reboot for changes to take effect. > > + > > + > > + HP specific class extensions > > + -------------------------------- > > + > > +What: /sys/class/firmware-attributes/*/authentication/SPM/kek > > +Date: March 29 > > +KernelVersion: 5.18 > > +Contact: "Jorge Lopez" <jorge.lopez2@xxxxxx> > > +Description: 'kek' is a write-only file that can be used to configure the > > + RSA public key that will be used by the BIOS to verify > > + signatures when setting the signing key. When written, > > + the bytes should correspond to the KEK certificate > > + (x509 .DER format containing an OU). The size of the > > + certificate must be less than or equal to 4095 bytes. > > + > > + > > +What: /sys/class/firmware-attributes/*/authentication/SPM/sk > > +Date: March 29 > > +KernelVersion: 5.18 > > +Contact: "Jorge Lopez" <jorge.lopez2@xxxxxx> > > +Description: 'sk' is a write-only file that can be used to configure the RSA > > + public key that will be used by the BIOS to verify signatures > > + when configuring BIOS settings and security features. When > > + written, the bytes should correspond to the modulus of the > > + public key. The exponent is assumed to be 0x10001. > > The names of the files 'SPM', 'kek' and 'sk' are cryptic. SPM - Secure Platform Manager kek - Key-Encryption-Key (KEK) sk - Signature Key (SK) Those abbreviations were used because they are industry standard and reduce the size of the commands. Any suggestions? > > > + > > + > > +What: /sys/class/firmware-attributes/*/authentication/SPM/status > > +Date: March 29 > > +KernelVersion: 5.18 > > +Contact: "Jorge Lopez" <jorge.lopez2@xxxxxx> > > +Description: 'status' is a read-only file that returns ASCII text reporting > > + the status information. > > + > > + State: Not Provisioned / Provisioned / Provisioning in progress > > + Version: Major. Minor > > + Feature Bit Mask: <16-bit unsigned number display in hex> > > + SPM Counter: <16-bit unsigned number display in base 10> > > + Signing Key Public Key Modulus (base64): > > + KEK Public Key Modulus (base64): > > This also violates 'one-value-per-file'. > Can it be split into different files? I will split the information in multiple files. > This would also remove the need for the statusbin file. > Status bin is used by GUI applications where data is managed accordingly instead of individual lines. > For the values: > > Status: I think symbolic names are better for sysfs: > not_provisioned, provisioned, etc. > Feature Bit Mask: Use names. > Keys: It would be nicer if these could be shown directly in the files > that can be used to configure them. > > As before, what is really needed and what can be added later? Status is needed when the user enables Secure Platform Manager in BIOS and KEK and/or SK are configured. > > > + > > + > > +What: /sys/class/firmware-attributes/*/authentication/SPM/statusbin > > +Date: March 29 > > +KernelVersion: 5.18 > > +Contact: "Jorge Lopez" <jorge.lopez2@xxxxxx> > > +Description: 'statusbin' is a read-only file that returns identical status > > + information reported by 'status' file in binary format. > > How does this binary format work? Yes. Status bin is used by GUI applications where data is managed accordingly instead of individual lines > > > + > > + > > +What: /sys/class/firmware-attributes/*/attributes/last_error > > +Date: March 29 > > +KernelVersion: 5.18 > > +Contact: "Jorge Lopez" <jorge.lopez2@xxxxxx> > > +Description: 'last_error' is a read-only file that returns WMI error number > > + and message reported by last WMI command. > > Does this provide much value? > Or could this error just be logged via pr_warn_ratelimited()? It is specially needed to determine if WMI calls reported an error. This property is similar to the one provided by both Dell and Lenovo drivers > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index f32538373164..663ae73fb8be 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -9367,6 +9367,12 @@ S: Obsolete > > W: http://w1.fi/hostap-driver.html > > F: drivers/net/wireless/intersil/hostap/ > > > > +HP BIOSCFG DRIVER > > +M: Jorge Lopez <jorge.lopez2@xxxxxx> > > +L: platform-driver-x86@xxxxxxxxxxxxxxx > > Broken whitespace I will be corrected. > > > +S: Maintained > > +F: drivers/platform/x86/hp/hp-bioscfg/ > > + > > HP COMPAQ TC1100 TABLET WMI EXTRAS DRIVER > > L: platform-driver-x86@xxxxxxxxxxxxxxx > > S: Orphan > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/Makefile b/drivers/platform/x86/hp/hp-bioscfg/Makefile > > new file mode 100644 > > index 000000000000..529eba6fa47f > > --- /dev/null > > +++ b/drivers/platform/x86/hp/hp-bioscfg/Makefile > > @@ -0,0 +1,13 @@ > > +obj-$(CONFIG_HP_BIOSCFG) := hp-bioscfg.o > > The kbuild part that defines CONFIG_HP_BIOSCFG is missing, so this is > never built. > This is an oversight on my part. The changes were made but never made part of the review. > drivers/platform/x86/hp/Makefile also needs to reference this Makefile. > > After fixing up Kbuild please build the driver with "make W=1" and clean > up all the unused functions/variables. > (This won't catch unused stuff from bioscfg.c, so you have to check > these manually) > Thank you. I will make sure to include it
