| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOOmCE_kzVnUr9WoPAEu-e+E5=-RfHUCjj6U7kL_yhqKHsP84g@mail.gmail.com>
Date: Mon, 3 Apr 2023 11:33:20 -0500
From: Jorge Lopez <jorgealtxwork@...il.com>
To: Thomas Weißschuh <thomas@...ch.de>
Cc: hdegoede@...hat.com, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 4/4] Introduction of HP-BIOSCFG driver [4]
Hi Thomas,
Please see my comments below.
On Sat, Apr 1, 2023 at 6:58 AM Thomas Weißschuh <thomas@...ch.de> 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@...r.kernel.org 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@...com>
> > +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@...com>
> > +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@...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):
>
> 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@...com>
> > +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@...com>
> > +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@...com>
> > +L: platform-driver-x86@...r.kernel.org
>
> 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@...r.kernel.org
> > 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
Powered by blists - more mailing lists