* [PATCH 1/2] Documentation: syfs-class-firmware-attributes: Lenovo Certificate support
@ 2022-03-12 0:04 Mark Pearson
2022-03-12 0:04 ` [PATCH 2/2] platform/x86: think-lmi: Certificate authentication support Mark Pearson
2022-03-14 13:13 ` [PATCH 1/2] Documentation: syfs-class-firmware-attributes: Lenovo Certificate support Hans de Goede
0 siblings, 2 replies; 6+ messages in thread
From: Mark Pearson @ 2022-03-12 0:04 UTC (permalink / raw)
To: markpearson; +Cc: hdegoede, markgross, platform-driver-x86
Certificate based authentication is available as an alternative to
password based authentication.
The WMI commands are cryptographically signed using a separate
signing server and will be verified by the BIOS before being
accepted.
This commit details the fields that are needed to support that
implementation. At present the changes are intended for Lenovo
platforms, but have been designed to keep them as flexible as possible
for future implementations from other vendors.
Signed-off-by: Mark Pearson <markpearson@lenovo.com>
---
.../testing/sysfs-class-firmware-attributes | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 13e31c6a0e9c..1d9c3bb1dbcd 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -246,6 +246,43 @@ Description:
that is being referenced (e.g hdd0, hdd1 etc)
This attribute defaults to device 0.
+ certificate:
+ signature:
+ These attributes are used for certificate based authentication. This is
+ used in conjunction with a signing server as an alternative to password
+ based authentication.
+ The user writes to the attribute with a BASE64 encoded string obtained
+ from the signing server.
+ The attribute can be displayed to check the stored value.
+
+ Some usage examples:
+ Installing a certificate to enable feature:
+ echo <supervisor password > authentication/Admin/current_password
+ echo <signed certificate> > authentication/Admin/certificate
+
+ Updating the installed certificate:
+ echo <signature> > authentication/Admin/signature
+ echo <signed certificate> > authentication/Admin/certificate
+
+ Removing the installed certificate:
+ echo <signature> > authentication/Admin/signature
+ echo '' > authentication/Admin/signature
+
+ You cannot enable certificate authentication if a supervisor password
+ has not been set.
+ After any of these operations the system must reboot for the changes to
+ take effect
+
+ certificate_thumbprint
+ Read only attribute used to display the MD5, SHA1 and SHA256 thumbprints
+ for the certificate installed in the BIOS.
+
+ certificate_to_password
+ Write only attribute used to switch from certificate based authentication
+ back to password based.
+ Usage:
+ echo <signature> > authentication/Admin/signature
+ echo <password> > authentication/Admin/certificate_to_password
What: /sys/class/firmware-attributes/*/attributes/pending_reboot
@@ -315,3 +352,18 @@ 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.
+
+What: /sys/class/firmware-attributes/*/attributes/set_signature
+What: /sys/class/firmware-attributes/*/attributes/save_signature
+Date: March 2022
+KernelVersion: 5.18
+Contact: Mark Pearson <markpearson@lenovo.com>
+Description:
+ These attributes are used when certificate based authentication are enabled.
+ The set_signature and save_signature are both obtained from the signing server
+ and both need to be set when changing an attribute.
+ Usage example:
+ echo <set signature> > set_signature
+ echo <save signature> > save_signature
+ echo Enable > PasswordBeep/current_value
+ The attributes can be read to display the stored value.
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] platform/x86: think-lmi: Certificate authentication support
2022-03-12 0:04 [PATCH 1/2] Documentation: syfs-class-firmware-attributes: Lenovo Certificate support Mark Pearson
@ 2022-03-12 0:04 ` Mark Pearson
2022-03-14 15:15 ` Hans de Goede
2022-03-14 13:13 ` [PATCH 1/2] Documentation: syfs-class-firmware-attributes: Lenovo Certificate support Hans de Goede
1 sibling, 1 reply; 6+ messages in thread
From: Mark Pearson @ 2022-03-12 0:04 UTC (permalink / raw)
To: markpearson; +Cc: hdegoede, markgross, platform-driver-x86
Implementation of certificate authentication feature for Lenovo
platforms. This allows for signed updates of BIOS settings.
Functionality supported:
- Cert support available check. At initialisation check if BIOS
supports certification authentication and if a certificate is
installed. Enable the sysfs nodes appropriately
- certificate and signature authentication attributes to enable
a user to install, update and delete a certificate using signed
signatures
- certificate_thumbprint to confirm installed certificate details
- support to go from certificate to password based authentication
- set_signature and save_signature attributes needed for setting
BIOS attributes using certificate authentication
Tested on X1 Carbon 10 with special trial BIOS. This feature is not
generally available yet but will be released later this year.
Note, I also cleaned up the formating of the GUIDs when I was adding
the new defines. Hope that's OK to combine in this commit.
Signed-off-by: Mark Pearson <markpearson@lenovo.com>
---
drivers/platform/x86/think-lmi.c | 560 ++++++++++++++++++++++++++-----
drivers/platform/x86/think-lmi.h | 7 +
2 files changed, 475 insertions(+), 92 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 0b73e16cccea..54ce71f97c37 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -16,6 +16,7 @@
#include <linux/fs.h>
#include <linux/string.h>
#include <linux/types.h>
+#include <linux/dmi.h>
#include <linux/wmi.h>
#include "firmware_attributes_class.h"
#include "think-lmi.h"
@@ -25,95 +26,66 @@ module_param(debug_support, bool, 0444);
MODULE_PARM_DESC(debug_support, "Enable debug command support");
/*
- * Name:
- * Lenovo_BiosSetting
- * Description:
- * Get item name and settings for current LMI instance.
- * Type:
- * Query
- * Returns:
- * "Item,Value"
- * Example:
- * "WakeOnLAN,Enable"
+ * Name: BiosSetting
+ * Description: Get item name and settings for current LMI instance.
+ * Type: Query
+ * Returns: "Item,Value"
+ * Example: "WakeOnLAN,Enable"
*/
#define LENOVO_BIOS_SETTING_GUID "51F5230E-9677-46CD-A1CF-C0B23EE34DB7"
/*
- * Name:
- * Lenovo_SetBiosSetting
- * Description:
- * Change the BIOS setting to the desired value using the Lenovo_SetBiosSetting
- * class. To save the settings, use the Lenovo_SaveBiosSetting class.
+ * Name: SetBiosSetting
+ * Description: Change the BIOS setting to the desired value using the SetBiosSetting
+ * class. To save the settings, use the SaveBiosSetting class.
* BIOS settings and values are case sensitive.
* After making changes to the BIOS settings, you must reboot the computer
* before the changes will take effect.
- * Type:
- * Method
- * Arguments:
- * "Item,Value,Password,Encoding,KbdLang;"
- * Example:
- * "WakeOnLAN,Disable,pa55w0rd,ascii,us;"
+ * Type: Method
+ * Arguments: "Item,Value,Password,Encoding,KbdLang;"
+ * Example: "WakeOnLAN,Disable,pa55w0rd,ascii,us;"
*/
#define LENOVO_SET_BIOS_SETTINGS_GUID "98479A64-33F5-4E33-A707-8E251EBBC3A1"
/*
- * Name:
- * Lenovo_SaveBiosSettings
- * Description:
- * Save any pending changes in settings.
- * Type:
- * Method
- * Arguments:
- * "Password,Encoding,KbdLang;"
- * Example:
- * "pa55w0rd,ascii,us;"
+ * Name: SaveBiosSettings
+ * Description: Save any pending changes in settings.
+ * Type: Method
+ * Arguments: "Password,Encoding,KbdLang;"
+ * Example: "pa55w0rd,ascii,us;"
*/
#define LENOVO_SAVE_BIOS_SETTINGS_GUID "6A4B54EF-A5ED-4D33-9455-B0D9B48DF4B3"
/*
- * Name:
- * Lenovo_BiosPasswordSettings
- * Description:
- * Return BIOS Password settings
- * Type:
- * Query
- * Returns:
- * PasswordMode, PasswordState, MinLength, MaxLength,
+ * Name: BiosPasswordSettings
+ * Description: Return BIOS Password settings
+ * Type: Query
+ * Returns: PasswordMode, PasswordState, MinLength, MaxLength,
* SupportedEncoding, SupportedKeyboard
*/
#define LENOVO_BIOS_PASSWORD_SETTINGS_GUID "8ADB159E-1E32-455C-BC93-308A7ED98246"
/*
- * Name:
- * Lenovo_SetBiosPassword
- * Description:
- * Change a specific password.
+ * Name: SetBiosPassword
+ * Description: Change a specific password.
* - BIOS settings cannot be changed at the same boot as power-on
* passwords (POP) and hard disk passwords (HDP). If you want to change
* BIOS settings and POP or HDP, you must reboot the system after changing
* one of them.
* - A password cannot be set using this method when one does not already
* exist. Passwords can only be updated or cleared.
- * Type:
- * Method
- * Arguments:
- * "PasswordType,CurrentPassword,NewPassword,Encoding,KbdLang;"
- * Example:
- * "pop,pa55w0rd,newpa55w0rd,ascii,us;”
+ * Type: Method
+ * Arguments: "PasswordType,CurrentPassword,NewPassword,Encoding,KbdLang;"
+ * Example: "pop,pa55w0rd,newpa55w0rd,ascii,us;”
*/
#define LENOVO_SET_BIOS_PASSWORD_GUID "2651D9FD-911C-4B69-B94E-D0DED5963BD7"
/*
- * Name:
- * Lenovo_GetBiosSelections
- * Description:
- * Return a list of valid settings for a given item.
- * Type:
- * Method
- * Arguments:
- * "Item"
- * Returns:
- * "Value1,Value2,Value3,..."
+ * Name: GetBiosSelections
+ * Description: Return a list of valid settings for a given item.
+ * Type: Method
+ * Arguments: "Item"
+ * Returns: "Value1,Value2,Value3,..."
* Example:
* -> "FlashOverLAN"
* <- "Enabled,Disabled"
@@ -121,18 +93,14 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
#define LENOVO_GET_BIOS_SELECTIONS_GUID "7364651A-132F-4FE7-ADAA-40C6C7EE2E3B"
/*
- * Name:
- * Lenovo_DebugCmdGUID
- * Description
- * Debug entry GUID method for entering debug commands to the BIOS
+ * Name: DebugCmd
+ * Description: Debug entry method for entering debug commands to the BIOS
*/
#define LENOVO_DEBUG_CMD_GUID "7FF47003-3B6C-4E5E-A227-E979824A85D1"
/*
- * Name:
- * Lenovo_OpcodeIF
- * Description:
- * Opcode interface which provides the ability to set multiple
+ * Name: OpcodeIF
+ * Description: Opcode interface which provides the ability to set multiple
* parameters and then trigger an action with a final command.
* This is particularly useful for simplifying setting passwords.
* With this support comes the ability to set System, HDD and NVMe
@@ -141,10 +109,71 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
*/
#define LENOVO_OPCODE_IF_GUID "DFDDEF2C-57D4-48ce-B196-0FB787D90836"
+/*
+ * Name: SetBiosCert
+ * Description: Install BIOS certificate.
+ * Type: Method
+ * Arguments: "Certificate,Password"
+ * You must reboot the computer before the changes will take effect.
+ */
+#define LENOVO_SET_BIOS_CERT_GUID "26861C9F-47E9-44C4-BD8B-DFE7FA2610FE"
+
+/*
+ * Name: UpdateBiosCert
+ * Description: Update BIOS certificate.
+ * Type: Method
+ * Format: "Certificate,Signature"
+ * You must reboot the computer before the changes will take effect.
+ */
+#define LENOVO_UPDATE_BIOS_CERT_GUID "9AA3180A-9750-41F7-B9F7-D5D3B1BAC3CE"
+
+/*
+ * Name: ClearBiosCert
+ * Description: Uninstall BIOS certificate.
+ * Type: Method
+ * Format: "Serial,Signature"
+ * You must reboot the computer before the changes will take effect.
+ */
+#define LENOVO_CLEAR_BIOS_CERT_GUID "B2BC39A7-78DD-4D71-B059-A510DEC44890"
+/*
+ * Name: CertToPassword
+ * Description: Switch from certificate to password authentication.
+ * Type: Method
+ * Format: "Password,Signature"
+ * You must reboot the computer before the changes will take effect.
+ */
+#define LENOVO_CERT_TO_PASSWORD_GUID "0DE8590D-5510-4044-9621-77C227F5A70D"
+
+/*
+ * Name: SetBiosSettingCert
+ * Description: Set attribute using certificate authentication.
+ * Type: Method
+ * Format: "Item,Value,Signature"
+ */
+#define LENOVO_SET_BIOS_SETTING_CERT_GUID "34A008CC-D205-4B62-9E67-31DFA8B90003"
+
+/*
+ * Name: SaveBiosSettingCert
+ * Description: Save any pending changes in settings.
+ * Type: Method
+ * Format: "Signature"
+ */
+#define LENOVO_SAVE_BIOS_SETTING_CERT_GUID "C050FB9D-DF5F-4606-B066-9EFC401B2551"
+
+/*
+ * Name: CertThumbprint
+ * Description: Display Certificate thumbprints
+ * Type: Query
+ * Returns: MD5, SHA1 & SHA256 thumbprints
+ */
+#define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4"
+
#define TLMI_POP_PWD (1 << 0)
#define TLMI_PAP_PWD (1 << 1)
#define TLMI_HDD_PWD (1 << 2)
#define TLMI_SYS_PWD (1 << 3)
+#define TLMI_CERT (1 << 7)
+
#define to_tlmi_pwd_setting(kobj) container_of(kobj, struct tlmi_pwd_setting, kobj)
#define to_tlmi_attr_setting(kobj) container_of(kobj, struct tlmi_attr_setting, kobj)
@@ -608,18 +637,249 @@ static ssize_t level_store(struct kobject *kobj,
static struct kobj_attribute auth_level = __ATTR_RW(level);
+static ssize_t cert_thumbprint(char *buf, const char *arg, int count)
+{
+ const struct acpi_buffer input = { strlen(arg), (char *)arg };
+ struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+ const union acpi_object *obj;
+ acpi_status status;
+
+ output.length = ACPI_ALLOCATE_BUFFER;
+ output.pointer = NULL;
+
+ status = wmi_evaluate_method(LENOVO_CERT_THUMBPRINT_GUID, 0, 0, &input, &output);
+ if (ACPI_FAILURE(status)) {
+ kfree(output.pointer);
+ return -EIO;
+ }
+ obj = output.pointer;
+ if (!obj)
+ return -ENOMEM;
+ if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
+ kfree(output.pointer);
+ return -EIO;
+ }
+ count += sysfs_emit_at(buf, count, "%s : %s\n", arg, (char *)obj->string.pointer);
+ kfree(output.pointer);
+
+ return count;
+}
+
+static ssize_t certificate_thumbprint_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
+ int count = 0;
+
+ if (!tlmi_priv.certificate_support || !setting->cert_installed)
+ return -EOPNOTSUPP;
+
+ count += cert_thumbprint(buf, "Md5", count);
+ count += cert_thumbprint(buf, "Sha1", count);
+ count += cert_thumbprint(buf, "Sha256", count);
+ return count;
+}
+
+static struct kobj_attribute auth_cert_thumb = __ATTR_RO(certificate_thumbprint);
+
+static ssize_t cert_to_password_store(struct kobject *kobj,
+ struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
+ char *auth_str, *passwd, *p;
+ int ret;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (!tlmi_priv.certificate_support)
+ return -EOPNOTSUPP;
+
+ if (!setting->cert_installed)
+ return -EINVAL;
+
+ if (!setting->signature || !setting->signature[0])
+ return -EACCES;
+
+ passwd = kstrdup(buf, GFP_KERNEL);
+ if (!passwd)
+ return -ENOMEM;
+
+ /* Strip out CR if one is present */
+ p = strchrnul(passwd, '\n');
+ *p = '\0';
+
+ /* Format: 'Password,Signature' */
+ auth_str = kasprintf(GFP_KERNEL, "%s,%s", passwd, setting->signature);
+ if (!auth_str) {
+ kfree(passwd);
+ return -ENOMEM;
+ }
+ ret = tlmi_simple_call(LENOVO_CERT_TO_PASSWORD_GUID, auth_str);
+ kfree(auth_str);
+ kfree(passwd);
+
+ return ret ?: count;
+}
+
+static struct kobj_attribute auth_cert_to_password = __ATTR_WO(cert_to_password);
+
+static ssize_t certificate_store(struct kobject *kobj,
+ struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
+ char *auth_str, *new_cert, *p;
+ int ret;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (!tlmi_priv.certificate_support)
+ return -EOPNOTSUPP;
+
+ new_cert = kstrdup(buf, GFP_KERNEL);
+ if (!new_cert)
+ return -ENOMEM;
+ /* Strip out CR if one is present */
+ p = strchrnul(new_cert, '\n');
+ *p = '\0';
+
+ /* If empty then clear installed certificate */
+ if (new_cert[0] == '\0') { /* Clear installed certificate */
+ /* Check that signature is set */
+ if (!setting->signature || !setting->signature[0]) {
+ kfree(new_cert);
+ return -EACCES;
+ }
+ /* Format: 'serial#, signature' */
+ auth_str = kasprintf(GFP_KERNEL, "%s,%s",
+ dmi_get_system_info(DMI_PRODUCT_SERIAL),
+ setting->signature);
+ if (!auth_str) {
+ kfree(new_cert);
+ return -ENOMEM;
+ }
+ ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
+ kfree(auth_str);
+ return ret ?: count;
+ }
+
+ if (setting->cert_installed) {
+ /* Certificate is installed so this is an update */
+ if (!setting->signature || !setting->signature[0]) {
+ kfree(new_cert);
+ return -EACCES;
+ }
+ /* Format: 'Certificate,Signature' */
+ auth_str = kasprintf(GFP_KERNEL, "%s,%s",
+ new_cert, setting->signature);
+ if (!auth_str) {
+ kfree(new_cert);
+ return -ENOMEM;
+ }
+ ret = tlmi_simple_call(LENOVO_UPDATE_BIOS_CERT_GUID, auth_str);
+ kfree(auth_str);
+ } else {
+ /* This is a fresh install */
+ if (!setting->valid || !setting->password[0]) {
+ kfree(new_cert);
+ return -EACCES;
+ }
+ /* Format: 'Certificate,Admin-password' */
+ auth_str = kasprintf(GFP_KERNEL, "%s,%s",
+ new_cert, setting->password);
+ if (!auth_str) {
+ kfree(new_cert);
+ return -ENOMEM;
+ }
+ ret = tlmi_simple_call(LENOVO_SET_BIOS_CERT_GUID, auth_str);
+ kfree(auth_str);
+ }
+
+ /* If successful update stored certificate */
+ if (!ret) {
+ kfree(setting->certificate);
+ setting->certificate = new_cert;
+ } else
+ kfree(new_cert);
+
+ return ret ?: count;
+}
+
+static ssize_t certificate_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
+
+ return sysfs_emit(buf, "%s\n", setting->certificate);
+}
+
+static struct kobj_attribute auth_certificate = __ATTR_RW(certificate);
+
+static ssize_t signature_store(struct kobject *kobj,
+ struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
+ char *new_signature, *p;
+ int ret;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (!tlmi_priv.certificate_support)
+ return -EOPNOTSUPP;
+
+ new_signature = kstrdup(buf, GFP_KERNEL);
+ if (!new_signature)
+ return -ENOMEM;
+
+ /* Strip out CR if one is present */
+ p = strchrnul(new_signature, '\n');
+ *p = '\0';
+
+ /* Free any previous signature */
+ kfree(setting->signature);
+ setting->signature = new_signature;
+
+ return ret ?: count;
+}
+
+static ssize_t signature_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
+
+ return sysfs_emit(buf, "%s\n", setting->signature);
+}
+
+static struct kobj_attribute auth_signature = __ATTR_RW(signature);
+
static umode_t auth_attr_is_visible(struct kobject *kobj,
struct attribute *attr, int n)
{
struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
- /*We only want to display level and index settings on HDD/NVMe */
+ /* We only want to display level and index settings on HDD/NVMe */
if ((attr == (struct attribute *)&auth_index) ||
(attr == (struct attribute *)&auth_level)) {
if ((setting == tlmi_priv.pwd_hdd) || (setting == tlmi_priv.pwd_nvme))
return attr->mode;
return 0;
}
+
+ /* We only display certificates on Admin account, if supported */
+ if ((attr == (struct attribute *)&auth_certificate) ||
+ (attr == (struct attribute *)&auth_signature) ||
+ (attr == (struct attribute *)&auth_cert_thumb) ||
+ (attr == (struct attribute *)&auth_cert_to_password)) {
+ if ((setting == tlmi_priv.pwd_admin) && tlmi_priv.certificate_support)
+ return attr->mode;
+ return 0;
+ }
+
return attr->mode;
}
@@ -635,6 +895,10 @@ static struct attribute *auth_attrs[] = {
&auth_kbdlang.attr,
&auth_index.attr,
&auth_level.attr,
+ &auth_certificate.attr,
+ &auth_signature.attr,
+ &auth_cert_thumb.attr,
+ &auth_cert_to_password.attr,
NULL
};
@@ -703,37 +967,58 @@ static ssize_t current_value_store(struct kobject *kobj,
p = strchrnul(new_setting, '\n');
*p = '\0';
- if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
- auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
- tlmi_priv.pwd_admin->password,
- encoding_options[tlmi_priv.pwd_admin->encoding],
- tlmi_priv.pwd_admin->kbdlang);
- if (!auth_str) {
+ /* Check if certificate authentication is enabled and active */
+ if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
+ if (!tlmi_priv.set_signature || !tlmi_priv.save_signature) {
+ ret = -EINVAL;
+ goto out;
+ }
+ set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name,
+ new_setting, tlmi_priv.set_signature);
+ if (!set_str) {
ret = -ENOMEM;
goto out;
}
- }
- if (auth_str)
- set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name,
- new_setting, auth_str);
- else
- set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
- new_setting);
- if (!set_str) {
- ret = -ENOMEM;
- goto out;
- }
+ ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTING_CERT_GUID, set_str);
+ if (ret)
+ goto out;
+ ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
+ tlmi_priv.save_signature);
+ if (ret)
+ goto out;
+ } else { /* Non certiifcate based authentication */
+ if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+ auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
+ tlmi_priv.pwd_admin->password,
+ encoding_options[tlmi_priv.pwd_admin->encoding],
+ tlmi_priv.pwd_admin->kbdlang);
+ if (!auth_str) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ }
- ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str);
- if (ret)
- goto out;
+ if (auth_str)
+ set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name,
+ new_setting, auth_str);
+ else
+ set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
+ new_setting);
+ if (!set_str) {
+ ret = -ENOMEM;
+ goto out;
+ }
- if (auth_str)
- ret = tlmi_save_bios_settings(auth_str);
- else
- ret = tlmi_save_bios_settings("");
+ ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str);
+ if (ret)
+ goto out;
+ if (auth_str)
+ ret = tlmi_save_bios_settings(auth_str);
+ else
+ ret = tlmi_save_bios_settings("");
+ }
if (!ret && !tlmi_priv.pending_changes) {
tlmi_priv.pending_changes = true;
/* let userland know it may need to check reboot pending again */
@@ -823,6 +1108,63 @@ static ssize_t pending_reboot_show(struct kobject *kobj, struct kobj_attribute *
static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot);
+static ssize_t attr_signature_store(bool set_sig, const char *buf)
+{
+ char *new_signature, *p;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (!tlmi_priv.certificate_support)
+ return -EOPNOTSUPP;
+
+ new_signature = kstrdup(buf, GFP_KERNEL);
+ if (!new_signature)
+ return -ENOMEM;
+
+ /* Strip out CR if one is present */
+ p = strchrnul(new_signature, '\n');
+ *p = '\0';
+
+ /* Free any previous signature */
+ kfree(set_sig ? tlmi_priv.set_signature : tlmi_priv.save_signature);
+ if (set_sig)
+ tlmi_priv.set_signature = new_signature;
+ else
+ tlmi_priv.save_signature = new_signature;
+
+ return 0;
+}
+
+static ssize_t set_signature_store(struct kobject *kobj,
+ struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ return attr_signature_store(true /*set_sig*/, buf) ?: count;
+}
+
+static ssize_t save_signature_store(struct kobject *kobj,
+ struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ return attr_signature_store(false /*set_sig*/, buf) ?: count;
+}
+
+static ssize_t set_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ return sysfs_emit(buf, "%s\n", tlmi_priv.set_signature);
+}
+
+static ssize_t save_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ return sysfs_emit(buf, "%s\n", tlmi_priv.save_signature);
+}
+
+static struct kobj_attribute attr_set_signature = __ATTR_RW(set_signature);
+static struct kobj_attribute attr_save_signature = __ATTR_RW(save_signature);
+
/* ---- Debug interface--------------------------------------------------------- */
static ssize_t debug_cmd_store(struct kobject *kobj, struct kobj_attribute *attr,
const char *buf, size_t count)
@@ -896,8 +1238,21 @@ static void tlmi_release_attr(void)
sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
if (tlmi_priv.can_debug_cmd && debug_support)
sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
+ if (tlmi_priv.certificate_support) {
+ sysfs_remove_file(&tlmi_priv.attribute_kset->kobj,
+ &attr_set_signature.attr);
+ sysfs_remove_file(&tlmi_priv.attribute_kset->kobj,
+ &attr_save_signature.attr);
+ }
kset_unregister(tlmi_priv.attribute_kset);
+ if (tlmi_priv.certificate_support) {
+ kfree(tlmi_priv.pwd_admin->certificate);
+ kfree(tlmi_priv.pwd_admin->signature);
+ kfree(tlmi_priv.set_signature);
+ kfree(tlmi_priv.save_signature);
+ }
+
/* Authentication structures */
sysfs_remove_group(&tlmi_priv.pwd_admin->kobj, &auth_attr_group);
kobject_put(&tlmi_priv.pwd_admin->kobj);
@@ -975,6 +1330,17 @@ static int tlmi_sysfs_init(void)
if (ret)
goto fail_create_attr;
}
+ if (tlmi_priv.certificate_support) {
+ ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj,
+ &attr_set_signature.attr);
+ if (ret)
+ goto fail_create_attr;
+ ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj,
+ &attr_save_signature.attr);
+ if (ret)
+ goto fail_create_attr;
+ }
+
/* Create authentication entries */
tlmi_priv.authentication_kset = kset_create_and_add("authentication", NULL,
&tlmi_priv.class_dev->kobj);
@@ -1087,6 +1453,11 @@ static int tlmi_analyze(void)
if (wmi_has_guid(LENOVO_OPCODE_IF_GUID))
tlmi_priv.opcode_support = true;
+ if (wmi_has_guid(LENOVO_SET_BIOS_CERT_GUID) &&
+ wmi_has_guid(LENOVO_SET_BIOS_SETTING_CERT_GUID) &&
+ wmi_has_guid(LENOVO_SAVE_BIOS_SETTING_CERT_GUID))
+ tlmi_priv.certificate_support = true;
+
/*
* Try to find the number of valid settings of this machine
* and use it to create sysfs attributes.
@@ -1198,6 +1569,11 @@ static int tlmi_analyze(void)
}
}
}
+
+ if (tlmi_priv.certificate_support &&
+ (tlmi_priv.pwdcfg.core.password_state & TLMI_CERT))
+ tlmi_priv.pwd_admin->cert_installed = true;
+
return 0;
fail_clear_attr:
diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
index e46c7f383353..47c11b006f6c 100644
--- a/drivers/platform/x86/think-lmi.h
+++ b/drivers/platform/x86/think-lmi.h
@@ -62,6 +62,9 @@ struct tlmi_pwd_setting {
char kbdlang[TLMI_LANG_MAXLEN];
int index; /*Used for HDD and NVME auth */
enum level_option level;
+ bool cert_installed;
+ char *certificate;
+ char *signature;
};
/* Attribute setting details */
@@ -82,6 +85,7 @@ struct think_lmi {
bool pending_changes;
bool can_debug_cmd;
bool opcode_support;
+ bool certificate_support;
struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT];
struct device *class_dev;
@@ -94,6 +98,9 @@ struct think_lmi {
struct tlmi_pwd_setting *pwd_system;
struct tlmi_pwd_setting *pwd_hdd;
struct tlmi_pwd_setting *pwd_nvme;
+
+ char *set_signature;
+ char *save_signature;
};
#endif /* !_THINK_LMI_H_ */
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Documentation: syfs-class-firmware-attributes: Lenovo Certificate support
2022-03-12 0:04 [PATCH 1/2] Documentation: syfs-class-firmware-attributes: Lenovo Certificate support Mark Pearson
2022-03-12 0:04 ` [PATCH 2/2] platform/x86: think-lmi: Certificate authentication support Mark Pearson
@ 2022-03-14 13:13 ` Hans de Goede
1 sibling, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2022-03-14 13:13 UTC (permalink / raw)
To: Mark Pearson; +Cc: markgross, platform-driver-x86
Hi,
On 3/12/22 01:04, Mark Pearson wrote:
> Certificate based authentication is available as an alternative to
> password based authentication.
>
> The WMI commands are cryptographically signed using a separate
> signing server and will be verified by the BIOS before being
> accepted.
>
> This commit details the fields that are needed to support that
> implementation. At present the changes are intended for Lenovo
> platforms, but have been designed to keep them as flexible as possible
> for future implementations from other vendors.
>
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> .../testing/sysfs-class-firmware-attributes | 52 +++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> index 13e31c6a0e9c..1d9c3bb1dbcd 100644
> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> @@ -246,6 +246,43 @@ Description:
> that is being referenced (e.g hdd0, hdd1 etc)
> This attribute defaults to device 0.
>
> + certificate:
> + signature:
> + These attributes are used for certificate based authentication. This is
> + used in conjunction with a signing server as an alternative to password
> + based authentication.
> + The user writes to the attribute with a BASE64 encoded string obtained
> + from the signing server.
> + The attribute can be displayed to check the stored value.
> +
> + Some usage examples:
> + Installing a certificate to enable feature:
> + echo <supervisor password > authentication/Admin/current_password
> + echo <signed certificate> > authentication/Admin/certificate
> +
> + Updating the installed certificate:
> + echo <signature> > authentication/Admin/signature
> + echo <signed certificate> > authentication/Admin/certificate
> +
> + Removing the installed certificate:
> + echo <signature> > authentication/Admin/signature
> + echo '' > authentication/Admin/signature
> +
> + You cannot enable certificate authentication if a supervisor password
> + has not been set.
> + After any of these operations the system must reboot for the changes to
> + take effect
> +
> + certificate_thumbprint
> + Read only attribute used to display the MD5, SHA1 and SHA256 thumbprints
> + for the certificate installed in the BIOS.
> +
> + certificate_to_password
> + Write only attribute used to switch from certificate based authentication
> + back to password based.
> + Usage:
> + echo <signature> > authentication/Admin/signature
> + echo <password> > authentication/Admin/certificate_to_password
>
>
> What: /sys/class/firmware-attributes/*/attributes/pending_reboot
> @@ -315,3 +352,18 @@ 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.
> +
> +What: /sys/class/firmware-attributes/*/attributes/set_signature
> +What: /sys/class/firmware-attributes/*/attributes/save_signature
> +Date: March 2022
> +KernelVersion: 5.18
> +Contact: Mark Pearson <markpearson@lenovo.com>
> +Description:
> + These attributes are used when certificate based authentication are enabled.
> + The set_signature and save_signature are both obtained from the signing server
> + and both need to be set when changing an attribute.
> + Usage example:
> + echo <set signature> > set_signature
> + echo <save signature> > save_signature
> + echo Enable > PasswordBeep/current_value
> + The attributes can be read to display the stored value.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] platform/x86: think-lmi: Certificate authentication support
2022-03-12 0:04 ` [PATCH 2/2] platform/x86: think-lmi: Certificate authentication support Mark Pearson
@ 2022-03-14 15:15 ` Hans de Goede
2022-03-14 15:46 ` [External] " Mark Pearson
0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2022-03-14 15:15 UTC (permalink / raw)
To: Mark Pearson; +Cc: markgross, platform-driver-x86
Hi,
On 3/12/22 01:04, Mark Pearson wrote:
> Implementation of certificate authentication feature for Lenovo
> platforms. This allows for signed updates of BIOS settings.
>
> Functionality supported:
> - Cert support available check. At initialisation check if BIOS
> supports certification authentication and if a certificate is
> installed. Enable the sysfs nodes appropriately
> - certificate and signature authentication attributes to enable
> a user to install, update and delete a certificate using signed
> signatures
> - certificate_thumbprint to confirm installed certificate details
> - support to go from certificate to password based authentication
> - set_signature and save_signature attributes needed for setting
> BIOS attributes using certificate authentication
>
> Tested on X1 Carbon 10 with special trial BIOS. This feature is not
> generally available yet but will be released later this year.
>
> Note, I also cleaned up the formating of the GUIDs when I was adding
> the new defines. Hope that's OK to combine in this commit.
>
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> ---
> drivers/platform/x86/think-lmi.c | 560 ++++++++++++++++++++++++++-----
> drivers/platform/x86/think-lmi.h | 7 +
> 2 files changed, 475 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 0b73e16cccea..54ce71f97c37 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -16,6 +16,7 @@
> #include <linux/fs.h>
> #include <linux/string.h>
> #include <linux/types.h>
> +#include <linux/dmi.h>
> #include <linux/wmi.h>
> #include "firmware_attributes_class.h"
> #include "think-lmi.h"
> @@ -25,95 +26,66 @@ module_param(debug_support, bool, 0444);
> MODULE_PARM_DESC(debug_support, "Enable debug command support");
>
> /*
> - * Name:
> - * Lenovo_BiosSetting
> - * Description:
> - * Get item name and settings for current LMI instance.
> - * Type:
> - * Query
> - * Returns:
> - * "Item,Value"
> - * Example:
> - * "WakeOnLAN,Enable"
> + * Name: BiosSetting
> + * Description: Get item name and settings for current LMI instance.
> + * Type: Query
> + * Returns: "Item,Value"
> + * Example: "WakeOnLAN,Enable"
> */
> #define LENOVO_BIOS_SETTING_GUID "51F5230E-9677-46CD-A1CF-C0B23EE34DB7"
>
> /*
> - * Name:
> - * Lenovo_SetBiosSetting
> - * Description:
> - * Change the BIOS setting to the desired value using the Lenovo_SetBiosSetting
> - * class. To save the settings, use the Lenovo_SaveBiosSetting class.
> + * Name: SetBiosSetting
> + * Description: Change the BIOS setting to the desired value using the SetBiosSetting
> + * class. To save the settings, use the SaveBiosSetting class.
> * BIOS settings and values are case sensitive.
> * After making changes to the BIOS settings, you must reboot the computer
> * before the changes will take effect.
> - * Type:
> - * Method
> - * Arguments:
> - * "Item,Value,Password,Encoding,KbdLang;"
> - * Example:
> - * "WakeOnLAN,Disable,pa55w0rd,ascii,us;"
> + * Type: Method
> + * Arguments: "Item,Value,Password,Encoding,KbdLang;"
> + * Example: "WakeOnLAN,Disable,pa55w0rd,ascii,us;"
> */
> #define LENOVO_SET_BIOS_SETTINGS_GUID "98479A64-33F5-4E33-A707-8E251EBBC3A1"
>
> /*
> - * Name:
> - * Lenovo_SaveBiosSettings
> - * Description:
> - * Save any pending changes in settings.
> - * Type:
> - * Method
> - * Arguments:
> - * "Password,Encoding,KbdLang;"
> - * Example:
> - * "pa55w0rd,ascii,us;"
> + * Name: SaveBiosSettings
> + * Description: Save any pending changes in settings.
> + * Type: Method
> + * Arguments: "Password,Encoding,KbdLang;"
> + * Example: "pa55w0rd,ascii,us;"
> */
> #define LENOVO_SAVE_BIOS_SETTINGS_GUID "6A4B54EF-A5ED-4D33-9455-B0D9B48DF4B3"
>
> /*
> - * Name:
> - * Lenovo_BiosPasswordSettings
> - * Description:
> - * Return BIOS Password settings
> - * Type:
> - * Query
> - * Returns:
> - * PasswordMode, PasswordState, MinLength, MaxLength,
> + * Name: BiosPasswordSettings
> + * Description: Return BIOS Password settings
> + * Type: Query
> + * Returns: PasswordMode, PasswordState, MinLength, MaxLength,
> * SupportedEncoding, SupportedKeyboard
> */
> #define LENOVO_BIOS_PASSWORD_SETTINGS_GUID "8ADB159E-1E32-455C-BC93-308A7ED98246"
>
> /*
> - * Name:
> - * Lenovo_SetBiosPassword
> - * Description:
> - * Change a specific password.
> + * Name: SetBiosPassword
> + * Description: Change a specific password.
> * - BIOS settings cannot be changed at the same boot as power-on
> * passwords (POP) and hard disk passwords (HDP). If you want to change
> * BIOS settings and POP or HDP, you must reboot the system after changing
> * one of them.
> * - A password cannot be set using this method when one does not already
> * exist. Passwords can only be updated or cleared.
> - * Type:
> - * Method
> - * Arguments:
> - * "PasswordType,CurrentPassword,NewPassword,Encoding,KbdLang;"
> - * Example:
> - * "pop,pa55w0rd,newpa55w0rd,ascii,us;”
> + * Type: Method
> + * Arguments: "PasswordType,CurrentPassword,NewPassword,Encoding,KbdLang;"
> + * Example: "pop,pa55w0rd,newpa55w0rd,ascii,us;”
> */
> #define LENOVO_SET_BIOS_PASSWORD_GUID "2651D9FD-911C-4B69-B94E-D0DED5963BD7"
>
> /*
> - * Name:
> - * Lenovo_GetBiosSelections
> - * Description:
> - * Return a list of valid settings for a given item.
> - * Type:
> - * Method
> - * Arguments:
> - * "Item"
> - * Returns:
> - * "Value1,Value2,Value3,..."
> + * Name: GetBiosSelections
> + * Description: Return a list of valid settings for a given item.
> + * Type: Method
> + * Arguments: "Item"
> + * Returns: "Value1,Value2,Value3,..."
> * Example:
> * -> "FlashOverLAN"
> * <- "Enabled,Disabled"
> @@ -121,18 +93,14 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
> #define LENOVO_GET_BIOS_SELECTIONS_GUID "7364651A-132F-4FE7-ADAA-40C6C7EE2E3B"
>
> /*
> - * Name:
> - * Lenovo_DebugCmdGUID
> - * Description
> - * Debug entry GUID method for entering debug commands to the BIOS
> + * Name: DebugCmd
> + * Description: Debug entry method for entering debug commands to the BIOS
> */
> #define LENOVO_DEBUG_CMD_GUID "7FF47003-3B6C-4E5E-A227-E979824A85D1"
>
> /*
> - * Name:
> - * Lenovo_OpcodeIF
> - * Description:
> - * Opcode interface which provides the ability to set multiple
> + * Name: OpcodeIF
> + * Description: Opcode interface which provides the ability to set multiple
> * parameters and then trigger an action with a final command.
> * This is particularly useful for simplifying setting passwords.
> * With this support comes the ability to set System, HDD and NVMe
> @@ -141,10 +109,71 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
> */
> #define LENOVO_OPCODE_IF_GUID "DFDDEF2C-57D4-48ce-B196-0FB787D90836"
>
> +/*
> + * Name: SetBiosCert
> + * Description: Install BIOS certificate.
> + * Type: Method
> + * Arguments: "Certificate,Password"
> + * You must reboot the computer before the changes will take effect.
> + */
> +#define LENOVO_SET_BIOS_CERT_GUID "26861C9F-47E9-44C4-BD8B-DFE7FA2610FE"
> +
> +/*
> + * Name: UpdateBiosCert
> + * Description: Update BIOS certificate.
> + * Type: Method
> + * Format: "Certificate,Signature"
> + * You must reboot the computer before the changes will take effect.
> + */
> +#define LENOVO_UPDATE_BIOS_CERT_GUID "9AA3180A-9750-41F7-B9F7-D5D3B1BAC3CE"
> +
> +/*
> + * Name: ClearBiosCert
> + * Description: Uninstall BIOS certificate.
> + * Type: Method
> + * Format: "Serial,Signature"
> + * You must reboot the computer before the changes will take effect.
> + */
> +#define LENOVO_CLEAR_BIOS_CERT_GUID "B2BC39A7-78DD-4D71-B059-A510DEC44890"
> +/*
> + * Name: CertToPassword
> + * Description: Switch from certificate to password authentication.
> + * Type: Method
> + * Format: "Password,Signature"
> + * You must reboot the computer before the changes will take effect.
> + */
> +#define LENOVO_CERT_TO_PASSWORD_GUID "0DE8590D-5510-4044-9621-77C227F5A70D"
> +
> +/*
> + * Name: SetBiosSettingCert
> + * Description: Set attribute using certificate authentication.
> + * Type: Method
> + * Format: "Item,Value,Signature"
> + */
> +#define LENOVO_SET_BIOS_SETTING_CERT_GUID "34A008CC-D205-4B62-9E67-31DFA8B90003"
> +
> +/*
> + * Name: SaveBiosSettingCert
> + * Description: Save any pending changes in settings.
> + * Type: Method
> + * Format: "Signature"
> + */
> +#define LENOVO_SAVE_BIOS_SETTING_CERT_GUID "C050FB9D-DF5F-4606-B066-9EFC401B2551"
> +
> +/*
> + * Name: CertThumbprint
> + * Description: Display Certificate thumbprints
> + * Type: Query
> + * Returns: MD5, SHA1 & SHA256 thumbprints
> + */
> +#define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4"
> +
> #define TLMI_POP_PWD (1 << 0)
> #define TLMI_PAP_PWD (1 << 1)
> #define TLMI_HDD_PWD (1 << 2)
> #define TLMI_SYS_PWD (1 << 3)
> +#define TLMI_CERT (1 << 7)
> +
> #define to_tlmi_pwd_setting(kobj) container_of(kobj, struct tlmi_pwd_setting, kobj)
> #define to_tlmi_attr_setting(kobj) container_of(kobj, struct tlmi_attr_setting, kobj)
>
> @@ -608,18 +637,249 @@ static ssize_t level_store(struct kobject *kobj,
>
> static struct kobj_attribute auth_level = __ATTR_RW(level);
>
> +static ssize_t cert_thumbprint(char *buf, const char *arg, int count)
> +{
> + const struct acpi_buffer input = { strlen(arg), (char *)arg };
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + const union acpi_object *obj;
> + acpi_status status;
> +
> + output.length = ACPI_ALLOCATE_BUFFER;
> + output.pointer = NULL;
This initialization of output is redundant, it is already initialized
when it is declared.
> +
> + status = wmi_evaluate_method(LENOVO_CERT_THUMBPRINT_GUID, 0, 0, &input, &output);
> + if (ACPI_FAILURE(status)) {
> + kfree(output.pointer);
> + return -EIO;
> + }
> + obj = output.pointer;
> + if (!obj)
> + return -ENOMEM;
> + if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
> + kfree(output.pointer);
> + return -EIO;
> + }
> + count += sysfs_emit_at(buf, count, "%s : %s\n", arg, (char *)obj->string.pointer);
> + kfree(output.pointer);
> +
> + return count;
> +}
> +
> +static ssize_t certificate_thumbprint_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
> + int count = 0;
> +
> + if (!tlmi_priv.certificate_support || !setting->cert_installed)
> + return -EOPNOTSUPP;
> +
> + count += cert_thumbprint(buf, "Md5", count);
> + count += cert_thumbprint(buf, "Sha1", count);
> + count += cert_thumbprint(buf, "Sha256", count);
> + return count;
> +}
> +
> +static struct kobj_attribute auth_cert_thumb = __ATTR_RO(certificate_thumbprint);
> +
> +static ssize_t cert_to_password_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
> + char *auth_str, *passwd, *p;
> + int ret;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (!tlmi_priv.certificate_support)
> + return -EOPNOTSUPP;
> +
> + if (!setting->cert_installed)
> + return -EINVAL;
> +
> + if (!setting->signature || !setting->signature[0])
> + return -EACCES;
> +
> + passwd = kstrdup(buf, GFP_KERNEL);
> + if (!passwd)
> + return -ENOMEM;
> +
> + /* Strip out CR if one is present */
> + p = strchrnul(passwd, '\n');
> + *p = '\0';
> +
> + /* Format: 'Password,Signature' */
> + auth_str = kasprintf(GFP_KERNEL, "%s,%s", passwd, setting->signature);
> + if (!auth_str) {
> + kfree(passwd);
> + return -ENOMEM;
> + }
> + ret = tlmi_simple_call(LENOVO_CERT_TO_PASSWORD_GUID, auth_str);
> + kfree(auth_str);
> + kfree(passwd);
> +
> + return ret ?: count;
> +}
> +
> +static struct kobj_attribute auth_cert_to_password = __ATTR_WO(cert_to_password);
> +
> +static ssize_t certificate_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
> + char *auth_str, *new_cert, *p;
> + int ret;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (!tlmi_priv.certificate_support)
> + return -EOPNOTSUPP;
> +
> + new_cert = kstrdup(buf, GFP_KERNEL);
> + if (!new_cert)
> + return -ENOMEM;
> + /* Strip out CR if one is present */
> + p = strchrnul(new_cert, '\n');
> + *p = '\0';
> +
> + /* If empty then clear installed certificate */
> + if (new_cert[0] == '\0') { /* Clear installed certificate */
You don't need new_cert anymore here, so do:
kfree(new_cert);
here.
> + /* Check that signature is set */
> + if (!setting->signature || !setting->signature[0]) {
> + kfree(new_cert);
and drop it here,
> + return -EACCES;
> + }
> + /* Format: 'serial#, signature' */
> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
> + dmi_get_system_info(DMI_PRODUCT_SERIAL),
> + setting->signature);
> + if (!auth_str) {
> + kfree(new_cert);
and here.
> + return -ENOMEM;
> + }
> + ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
> + kfree(auth_str);
Because you were missing a kfree(new_cert) here. Also you should free + clear
setting->certificate here. So this if block should end up like this:
/* If empty then clear installed certificate */
if (new_cert[0] == '\0') { /* Clear installed certificate */
kfree(new_cert);
/* Check that signature is set */
if (!setting->signature || !setting->signature[0])
return -EACCES;
/* Format: 'serial#, signature' */
auth_str = kasprintf(GFP_KERNEL, "%s,%s",
dmi_get_system_info(DMI_PRODUCT_SERIAL),
setting->signature);
if (!auth_str)
return -ENOMEM;
ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
kfree(auth_str);
if (ret)
return ret;
kfree(setting->certificate);
setting->certificate = NULL;
return count;
}
> + }
> +
> + if (setting->cert_installed) {
> + /* Certificate is installed so this is an update */
> + if (!setting->signature || !setting->signature[0]) {
> + kfree(new_cert);
> + return -EACCES;
> + }
> + /* Format: 'Certificate,Signature' */
> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
> + new_cert, setting->signature);> + if (!auth_str) {
> + kfree(new_cert);
> + return -ENOMEM;
> + }
> + ret = tlmi_simple_call(LENOVO_UPDATE_BIOS_CERT_GUID, auth_str);
> + kfree(auth_str);
> + } else {
> + /* This is a fresh install */
> + if (!setting->valid || !setting->password[0]) {
> + kfree(new_cert);
> + return -EACCES;
> + }
> + /* Format: 'Certificate,Admin-password' */
> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
> + new_cert, setting->password);
> + if (!auth_str) {
> + kfree(new_cert);
> + return -ENOMEM;
> + }
> + ret = tlmi_simple_call(LENOVO_SET_BIOS_CERT_GUID, auth_str);
> + kfree(auth_str);
> + }
> +
> + /* If successful update stored certificate */
> + if (!ret) {
> + kfree(setting->certificate);
> + setting->certificate = new_cert;
> + } else
> + kfree(new_cert);
> +
> + return ret ?: count;
You have 2 "if (ret)" statements here (1 hidden in the return), please change this to:
if (ret) {
kfree(new_cert);
return ret;
}
kfree(setting->certificate);
setting->certificate = new_cert;
return count;
> +}
> +
> +static ssize_t certificate_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
> +
setting->certificate may be NULL here, you need to check for that and in
that case only emit a "\n" I guess.
> + return sysfs_emit(buf, "%s\n", setting->certificate);
> +}
> +
> +static struct kobj_attribute auth_certificate = __ATTR_RW(certificate);
> +
> +static ssize_t signature_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
> + char *new_signature, *p;
> + int ret;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (!tlmi_priv.certificate_support)
> + return -EOPNOTSUPP;
> +
> + new_signature = kstrdup(buf, GFP_KERNEL);
> + if (!new_signature)
> + return -ENOMEM;
> +
> + /* Strip out CR if one is present */
> + p = strchrnul(new_signature, '\n');
> + *p = '\0';
Idea for a follow-up clean-up patch: this pattern of kstrdup user-argument
(buf) + strip '\n' is repeated all over the driver, maybe add a little helper
for this?
> +
> + /* Free any previous signature */
> + kfree(setting->signature);
> + setting->signature = new_signature;
> +
> + return ret ?: count;
> +}
> +
> +static ssize_t signature_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
> +
setting->signature can be NULL here.
> + return sysfs_emit(buf, "%s\n", setting->signature);
> +}
> +
> +static struct kobj_attribute auth_signature = __ATTR_RW(signature);
> +
> static umode_t auth_attr_is_visible(struct kobject *kobj,
> struct attribute *attr, int n)
> {
> struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
> - /*We only want to display level and index settings on HDD/NVMe */
> + /* We only want to display level and index settings on HDD/NVMe */
> if ((attr == (struct attribute *)&auth_index) ||
> (attr == (struct attribute *)&auth_level)) {
> if ((setting == tlmi_priv.pwd_hdd) || (setting == tlmi_priv.pwd_nvme))
> return attr->mode;
> return 0;
> }
> +
> + /* We only display certificates on Admin account, if supported */
> + if ((attr == (struct attribute *)&auth_certificate) ||
> + (attr == (struct attribute *)&auth_signature) ||
> + (attr == (struct attribute *)&auth_cert_thumb) ||
> + (attr == (struct attribute *)&auth_cert_to_password)) {
> + if ((setting == tlmi_priv.pwd_admin) && tlmi_priv.certificate_support)
> + return attr->mode;
> + return 0;
> + }
> +
> return attr->mode;
> }
>
> @@ -635,6 +895,10 @@ static struct attribute *auth_attrs[] = {
> &auth_kbdlang.attr,
> &auth_index.attr,
> &auth_level.attr,
> + &auth_certificate.attr,
> + &auth_signature.attr,
> + &auth_cert_thumb.attr,
> + &auth_cert_to_password.attr,
> NULL
> };
>
> @@ -703,37 +967,58 @@ static ssize_t current_value_store(struct kobject *kobj,
> p = strchrnul(new_setting, '\n');
> *p = '\0';
>
> - if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> - auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> - tlmi_priv.pwd_admin->password,
> - encoding_options[tlmi_priv.pwd_admin->encoding],
> - tlmi_priv.pwd_admin->kbdlang);
> - if (!auth_str) {
> + /* Check if certificate authentication is enabled and active */
> + if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
> + if (!tlmi_priv.set_signature || !tlmi_priv.save_signature) {
> + ret = -EINVAL;
> + goto out;
> + }
> + set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name,
> + new_setting, tlmi_priv.set_signature);
> + if (!set_str) {
> ret = -ENOMEM;
> goto out;
> }
> - }
>
> - if (auth_str)
> - set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name,
> - new_setting, auth_str);
> - else
> - set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
> - new_setting);
> - if (!set_str) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTING_CERT_GUID, set_str);
> + if (ret)
> + goto out;
> + ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
> + tlmi_priv.save_signature);
> + if (ret)
> + goto out;
> + } else { /* Non certiifcate based authentication */
> + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> + auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> + tlmi_priv.pwd_admin->password,
> + encoding_options[tlmi_priv.pwd_admin->encoding],
> + tlmi_priv.pwd_admin->kbdlang);
> + if (!auth_str) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + }
>
> - ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str);
> - if (ret)
> - goto out;
> + if (auth_str)
> + set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name,
> + new_setting, auth_str);
> + else
> + set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
> + new_setting);
> + if (!set_str) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> - if (auth_str)
> - ret = tlmi_save_bios_settings(auth_str);
> - else
> - ret = tlmi_save_bios_settings("");
> + ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str);
> + if (ret)
> + goto out;
>
> + if (auth_str)
> + ret = tlmi_save_bios_settings(auth_str);
> + else
> + ret = tlmi_save_bios_settings("");
> + }
> if (!ret && !tlmi_priv.pending_changes) {
> tlmi_priv.pending_changes = true;
> /* let userland know it may need to check reboot pending again */
> @@ -823,6 +1108,63 @@ static ssize_t pending_reboot_show(struct kobject *kobj, struct kobj_attribute *
>
> static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot);
>
> +static ssize_t attr_signature_store(bool set_sig, const char *buf)
> +{
> + char *new_signature, *p;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (!tlmi_priv.certificate_support)
> + return -EOPNOTSUPP;
> +
> + new_signature = kstrdup(buf, GFP_KERNEL);
> + if (!new_signature)
> + return -ENOMEM;
> +
> + /* Strip out CR if one is present */
> + p = strchrnul(new_signature, '\n');
> + *p = '\0';
> +
> + /* Free any previous signature */
> + kfree(set_sig ? tlmi_priv.set_signature : tlmi_priv.save_signature);
> + if (set_sig)
> + tlmi_priv.set_signature = new_signature;
> + else
> + tlmi_priv.save_signature = new_signature;
> +
> + return 0;
> +}
> +
> +static ssize_t set_signature_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + return attr_signature_store(true /*set_sig*/, buf) ?: count;
> +}
> +
> +static ssize_t save_signature_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + return attr_signature_store(false /*set_sig*/, buf) ?: count;
> +}
> +
> +static ssize_t set_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "%s\n", tlmi_priv.set_signature);
> +}
> +
> +static ssize_t save_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "%s\n", tlmi_priv.save_signature);
> +}
> +
> +static struct kobj_attribute attr_set_signature = __ATTR_RW(set_signature);
> +static struct kobj_attribute attr_save_signature = __ATTR_RW(save_signature);
<note I missed this while reviewing the documentation patch...>
Why not just use the /sys/class/firmware-attributes/thinklmi/authentication/Admin/signature
value here ?
/sys/class/firmware-attributes/thinklmi/authentication/Admin/current_password is
what is used for password based setting of fw-attributes as well as for changing
the password; and
/sys/class/firmware-attributes/thinklmi/authentication/Admin/signature is set
for changing the certificate, so it would be much more consitent to also use
that for setting attributes when using certificate based auth?
Can / will the set and save signature ever be different from one another ? If yes
then I guess we may need 2 attributes for this, I guess maybe just signature +
save_signature ? Either way IMHO these 2 attributes / the 1 extra attribute
for a separate save-signature really belongs under
/sys/class/firmware-attributes/thinklmi/authentication/Admin/ IMHO and
not under /sys/class/firmware-attributes/thinklmi/attributes/
What do you think about moving these there ?
Regards,
Hans
> +
> /* ---- Debug interface--------------------------------------------------------- */
> static ssize_t debug_cmd_store(struct kobject *kobj, struct kobj_attribute *attr,
> const char *buf, size_t count)
> @@ -896,8 +1238,21 @@ static void tlmi_release_attr(void)
> sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
> if (tlmi_priv.can_debug_cmd && debug_support)
> sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
> + if (tlmi_priv.certificate_support) {
> + sysfs_remove_file(&tlmi_priv.attribute_kset->kobj,
> + &attr_set_signature.attr);
> + sysfs_remove_file(&tlmi_priv.attribute_kset->kobj,
> + &attr_save_signature.attr);
> + }
> kset_unregister(tlmi_priv.attribute_kset);
>
> + if (tlmi_priv.certificate_support) {
> + kfree(tlmi_priv.pwd_admin->certificate);
> + kfree(tlmi_priv.pwd_admin->signature);
> + kfree(tlmi_priv.set_signature);
> + kfree(tlmi_priv.save_signature);
> + }
> +
> /* Authentication structures */
> sysfs_remove_group(&tlmi_priv.pwd_admin->kobj, &auth_attr_group);
> kobject_put(&tlmi_priv.pwd_admin->kobj);
> @@ -975,6 +1330,17 @@ static int tlmi_sysfs_init(void)
> if (ret)
> goto fail_create_attr;
> }
> + if (tlmi_priv.certificate_support) {
> + ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj,
> + &attr_set_signature.attr);
> + if (ret)
> + goto fail_create_attr;
> + ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj,
> + &attr_save_signature.attr);
> + if (ret)
> + goto fail_create_attr;
> + }
> +
> /* Create authentication entries */
> tlmi_priv.authentication_kset = kset_create_and_add("authentication", NULL,
> &tlmi_priv.class_dev->kobj);
> @@ -1087,6 +1453,11 @@ static int tlmi_analyze(void)
> if (wmi_has_guid(LENOVO_OPCODE_IF_GUID))
> tlmi_priv.opcode_support = true;
>
> + if (wmi_has_guid(LENOVO_SET_BIOS_CERT_GUID) &&
> + wmi_has_guid(LENOVO_SET_BIOS_SETTING_CERT_GUID) &&
> + wmi_has_guid(LENOVO_SAVE_BIOS_SETTING_CERT_GUID))
> + tlmi_priv.certificate_support = true;
> +
> /*
> * Try to find the number of valid settings of this machine
> * and use it to create sysfs attributes.
> @@ -1198,6 +1569,11 @@ static int tlmi_analyze(void)
> }
> }
> }
> +
> + if (tlmi_priv.certificate_support &&
> + (tlmi_priv.pwdcfg.core.password_state & TLMI_CERT))
> + tlmi_priv.pwd_admin->cert_installed = true;
> +
> return 0;
>
> fail_clear_attr:
> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
> index e46c7f383353..47c11b006f6c 100644
> --- a/drivers/platform/x86/think-lmi.h
> +++ b/drivers/platform/x86/think-lmi.h
> @@ -62,6 +62,9 @@ struct tlmi_pwd_setting {
> char kbdlang[TLMI_LANG_MAXLEN];
> int index; /*Used for HDD and NVME auth */
> enum level_option level;
> + bool cert_installed;
> + char *certificate;
> + char *signature;
> };
>
> /* Attribute setting details */
> @@ -82,6 +85,7 @@ struct think_lmi {
> bool pending_changes;
> bool can_debug_cmd;
> bool opcode_support;
> + bool certificate_support;
>
> struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT];
> struct device *class_dev;
> @@ -94,6 +98,9 @@ struct think_lmi {
> struct tlmi_pwd_setting *pwd_system;
> struct tlmi_pwd_setting *pwd_hdd;
> struct tlmi_pwd_setting *pwd_nvme;
> +
> + char *set_signature;
> + char *save_signature;
> };
>
> #endif /* !_THINK_LMI_H_ */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [External] Re: [PATCH 2/2] platform/x86: think-lmi: Certificate authentication support
2022-03-14 15:15 ` Hans de Goede
@ 2022-03-14 15:46 ` Mark Pearson
2022-03-14 16:52 ` Hans de Goede
0 siblings, 1 reply; 6+ messages in thread
From: Mark Pearson @ 2022-03-14 15:46 UTC (permalink / raw)
To: Hans de Goede; +Cc: markgross, platform-driver-x86
Thanks for the review Hans
On 2022-03-14 11:15, Hans de Goede wrote:
> Hi,
>
> On 3/12/22 01:04, Mark Pearson wrote:
>> Implementation of certificate authentication feature for Lenovo
>> platforms. This allows for signed updates of BIOS settings.
>>
>> Functionality supported:
>> - Cert support available check. At initialisation check if BIOS
>> supports certification authentication and if a certificate is
>> installed. Enable the sysfs nodes appropriately
>> - certificate and signature authentication attributes to enable
>> a user to install, update and delete a certificate using signed
>> signatures
>> - certificate_thumbprint to confirm installed certificate details
>> - support to go from certificate to password based authentication
>> - set_signature and save_signature attributes needed for setting
>> BIOS attributes using certificate authentication
>>
>> Tested on X1 Carbon 10 with special trial BIOS. This feature is not
>> generally available yet but will be released later this year.
>>
>> Note, I also cleaned up the formating of the GUIDs when I was adding
>> the new defines. Hope that's OK to combine in this commit.
>>
>> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
>> ---
>> drivers/platform/x86/think-lmi.c | 560 ++++++++++++++++++++++++++-----
>> drivers/platform/x86/think-lmi.h | 7 +
>> 2 files changed, 475 insertions(+), 92 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index 0b73e16cccea..54ce71f97c37 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -16,6 +16,7 @@
<snip>
>>
>> +static ssize_t cert_thumbprint(char *buf, const char *arg, int count)
>> +{
>> + const struct acpi_buffer input = { strlen(arg), (char *)arg };
>> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>> + const union acpi_object *obj;
>> + acpi_status status;
>> +
>> + output.length = ACPI_ALLOCATE_BUFFER;
>> + output.pointer = NULL;
>
> This initialization of output is redundant, it is already initialized
> when it is declared.
Ack
<snip>
>> +
>> +static ssize_t certificate_store(struct kobject *kobj,
>> + struct kobj_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>> + char *auth_str, *new_cert, *p;
>> + int ret;
>> +
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>> + if (!tlmi_priv.certificate_support)
>> + return -EOPNOTSUPP;
>> +
>> + new_cert = kstrdup(buf, GFP_KERNEL);
>> + if (!new_cert)
>> + return -ENOMEM;
>> + /* Strip out CR if one is present */
>> + p = strchrnul(new_cert, '\n');
>> + *p = '\0';
>> +
>> + /* If empty then clear installed certificate */
>> + if (new_cert[0] == '\0') { /* Clear installed certificate */
>
> You don't need new_cert anymore here, so do:
>
> kfree(new_cert);
>
> here.
>
>> + /* Check that signature is set */
>> + if (!setting->signature || !setting->signature[0]) {
>> + kfree(new_cert);
>
> and drop it here,
>
>> + return -EACCES;
>> + }
>> + /* Format: 'serial#, signature' */
>> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>> + dmi_get_system_info(DMI_PRODUCT_SERIAL),
>> + setting->signature);
>> + if (!auth_str) {
>> + kfree(new_cert);
>
> and here.
>
>> + return -ENOMEM;
>> + }
>> + ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
>> + kfree(auth_str);
>
> Because you were missing a kfree(new_cert) here. Also you should free + clear
> setting->certificate here. So this if block should end up like this:
>
> /* If empty then clear installed certificate */
> if (new_cert[0] == '\0') { /* Clear installed certificate */
> kfree(new_cert);
>
> /* Check that signature is set */
> if (!setting->signature || !setting->signature[0])
> return -EACCES;
>
> /* Format: 'serial#, signature' */
> auth_str = kasprintf(GFP_KERNEL, "%s,%s",
> dmi_get_system_info(DMI_PRODUCT_SERIAL),
> setting->signature);
> if (!auth_str)
> return -ENOMEM;
>
> ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
> kfree(auth_str);
> if (ret)
> return ret;
>
> kfree(setting->certificate);
> setting->certificate = NULL;
> return count;
> }
>
>
Agreed - that all makes sense. Will update.
>> + }
>> +
>> + if (setting->cert_installed) {
>> + /* Certificate is installed so this is an update */
>> + if (!setting->signature || !setting->signature[0]) {
>> + kfree(new_cert);
>> + return -EACCES;
>> + }
>> + /* Format: 'Certificate,Signature' */
>> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>> + new_cert, setting->signature);> + if (!auth_str) {
>> + kfree(new_cert);
>> + return -ENOMEM;
>> + }
>> + ret = tlmi_simple_call(LENOVO_UPDATE_BIOS_CERT_GUID, auth_str);
>> + kfree(auth_str);
>> + } else {
>> + /* This is a fresh install */
>> + if (!setting->valid || !setting->password[0]) {
>> + kfree(new_cert);
>> + return -EACCES;
>> + }
>> + /* Format: 'Certificate,Admin-password' */
>> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>> + new_cert, setting->password);
>> + if (!auth_str) {
>> + kfree(new_cert);
>> + return -ENOMEM;
>> + }
>> + ret = tlmi_simple_call(LENOVO_SET_BIOS_CERT_GUID, auth_str);
>> + kfree(auth_str);
>> + }
>> +
>> + /* If successful update stored certificate */
>> + if (!ret) {
>> + kfree(setting->certificate);
>> + setting->certificate = new_cert;
>> + } else
>> + kfree(new_cert);
>> +
>> + return ret ?: count;
>
> You have 2 "if (ret)" statements here (1 hidden in the return), please change this to:
>
> if (ret) {
> kfree(new_cert);
> return ret;
> }
>
> kfree(setting->certificate);
> setting->certificate = new_cert;
> return count;
>
Will do
>
>> +}
>> +
>> +static ssize_t certificate_show(struct kobject *kobj, struct kobj_attribute *attr,
>> + char *buf)
>> +{
>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>> +
>
> setting->certificate may be NULL here, you need to check for that and in
> that case only emit a "\n" I guess.
Ack.
>
>> + return sysfs_emit(buf, "%s\n", setting->certificate);
>> +}
>> +
>> +static struct kobj_attribute auth_certificate = __ATTR_RW(certificate);
>> +
>> +static ssize_t signature_store(struct kobject *kobj,
>> + struct kobj_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>> + char *new_signature, *p;
>> + int ret;
>> +
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>> + if (!tlmi_priv.certificate_support)
>> + return -EOPNOTSUPP;
>> +
>> + new_signature = kstrdup(buf, GFP_KERNEL);
>> + if (!new_signature)
>> + return -ENOMEM;
>> +
>> + /* Strip out CR if one is present */
>> + p = strchrnul(new_signature, '\n');
>> + *p = '\0';
>
> Idea for a follow-up clean-up patch: this pattern of kstrdup user-argument
> (buf) + strip '\n' is repeated all over the driver, maybe add a little helper
> for this?
>
Yes - that would make sense. Will do.
>> +
>> + /* Free any previous signature */
>> + kfree(setting->signature);
>> + setting->signature = new_signature;
>> +
>> + return ret ?: count;
>> +}
>> +
>> +static ssize_t signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>> + char *buf)
>> +{
>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>> +
>
> setting->signature can be NULL here.
Ack
<snip>
>> +
>> +static ssize_t set_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>> + char *buf)
>> +{
>> + return sysfs_emit(buf, "%s\n", tlmi_priv.set_signature);
>> +}
>> +
>> +static ssize_t save_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>> + char *buf)
>> +{
>> + return sysfs_emit(buf, "%s\n", tlmi_priv.save_signature);
>> +}
>> +
>> +static struct kobj_attribute attr_set_signature = __ATTR_RW(set_signature);
>> +static struct kobj_attribute attr_save_signature = __ATTR_RW(save_signature);
>
> <note I missed this while reviewing the documentation patch...>
>
> Why not just use the /sys/class/firmware-attributes/thinklmi/authentication/Admin/signature
> value here ?
>
> /sys/class/firmware-attributes/thinklmi/authentication/Admin/current_password is
> what is used for password based setting of fw-attributes as well as for changing
> the password; and
>
> /sys/class/firmware-attributes/thinklmi/authentication/Admin/signature is set
> for changing the certificate, so it would be much more consitent to also use
> that for setting attributes when using certificate based auth?
>
> Can / will the set and save signature ever be different from one another ? If yes
> then I guess we may need 2 attributes for this, I guess maybe just signature +
> save_signature ? Either way IMHO these 2 attributes / the 1 extra attribute
> for a separate save-signature really belongs under
> /sys/class/firmware-attributes/thinklmi/authentication/Admin/ IMHO and
> not under /sys/class/firmware-attributes/thinklmi/attributes/
>
> What do you think about moving these there ?
>
I have no issues with moving them. I had originally intended to have
them in auth but as I needed two signatures (the set and save are
different) I decided to make it explicit and to keep them in the same
place as the attribute being modified. But I can see it making sense to
just keep those under Authentication instead.
I'll update and get rid of set_signature and move save_signature.
Many thanks for the review
Mark
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [External] Re: [PATCH 2/2] platform/x86: think-lmi: Certificate authentication support
2022-03-14 15:46 ` [External] " Mark Pearson
@ 2022-03-14 16:52 ` Hans de Goede
0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2022-03-14 16:52 UTC (permalink / raw)
To: Mark Pearson; +Cc: markgross, platform-driver-x86
Hi,
On 3/14/22 16:46, Mark Pearson wrote:
> Thanks for the review Hans
>
> On 2022-03-14 11:15, Hans de Goede wrote:
>> Hi,
>>
>> On 3/12/22 01:04, Mark Pearson wrote:
>>> Implementation of certificate authentication feature for Lenovo
>>> platforms. This allows for signed updates of BIOS settings.
>>>
>>> Functionality supported:
>>> - Cert support available check. At initialisation check if BIOS
>>> supports certification authentication and if a certificate is
>>> installed. Enable the sysfs nodes appropriately
>>> - certificate and signature authentication attributes to enable
>>> a user to install, update and delete a certificate using signed
>>> signatures
>>> - certificate_thumbprint to confirm installed certificate details
>>> - support to go from certificate to password based authentication
>>> - set_signature and save_signature attributes needed for setting
>>> BIOS attributes using certificate authentication
>>>
>>> Tested on X1 Carbon 10 with special trial BIOS. This feature is not
>>> generally available yet but will be released later this year.
>>>
>>> Note, I also cleaned up the formating of the GUIDs when I was adding
>>> the new defines. Hope that's OK to combine in this commit.
>>>
>>> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
>>> ---
>>> drivers/platform/x86/think-lmi.c | 560 ++++++++++++++++++++++++++-----
>>> drivers/platform/x86/think-lmi.h | 7 +
>>> 2 files changed, 475 insertions(+), 92 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> index 0b73e16cccea..54ce71f97c37 100644
>>> --- a/drivers/platform/x86/think-lmi.c
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -16,6 +16,7 @@
>
> <snip>
>>>
>>> +static ssize_t cert_thumbprint(char *buf, const char *arg, int count)
>>> +{
>>> + const struct acpi_buffer input = { strlen(arg), (char *)arg };
>>> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>>> + const union acpi_object *obj;
>>> + acpi_status status;
>>> +
>>> + output.length = ACPI_ALLOCATE_BUFFER;
>>> + output.pointer = NULL;
>>
>> This initialization of output is redundant, it is already initialized
>> when it is declared.
> Ack
>
> <snip>
>>> +
>>> +static ssize_t certificate_store(struct kobject *kobj,
>>> + struct kobj_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>> + char *auth_str, *new_cert, *p;
>>> + int ret;
>>> +
>>> + if (!capable(CAP_SYS_ADMIN))
>>> + return -EPERM;
>>> +
>>> + if (!tlmi_priv.certificate_support)
>>> + return -EOPNOTSUPP;
>>> +
>>> + new_cert = kstrdup(buf, GFP_KERNEL);
>>> + if (!new_cert)
>>> + return -ENOMEM;
>>> + /* Strip out CR if one is present */
>>> + p = strchrnul(new_cert, '\n');
>>> + *p = '\0';
>>> +
>>> + /* If empty then clear installed certificate */
>>> + if (new_cert[0] == '\0') { /* Clear installed certificate */
>>
>> You don't need new_cert anymore here, so do:
>>
>> kfree(new_cert);
>>
>> here.
>>
>>> + /* Check that signature is set */
>>> + if (!setting->signature || !setting->signature[0]) {
>>> + kfree(new_cert);
>>
>> and drop it here,
>>
>>> + return -EACCES;
>>> + }
>>> + /* Format: 'serial#, signature' */
>>> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>>> + dmi_get_system_info(DMI_PRODUCT_SERIAL),
>>> + setting->signature);
>>> + if (!auth_str) {
>>> + kfree(new_cert);
>>
>> and here.
>>
>>> + return -ENOMEM;
>>> + }
>>> + ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
>>> + kfree(auth_str);
>>
>> Because you were missing a kfree(new_cert) here. Also you should free + clear
>> setting->certificate here. So this if block should end up like this:
>>
>> /* If empty then clear installed certificate */
>> if (new_cert[0] == '\0') { /* Clear installed certificate */
>> kfree(new_cert);
>>
>> /* Check that signature is set */
>> if (!setting->signature || !setting->signature[0])
>> return -EACCES;
>>
>> /* Format: 'serial#, signature' */
>> auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>> dmi_get_system_info(DMI_PRODUCT_SERIAL),
>> setting->signature);
>> if (!auth_str)
>> return -ENOMEM;
>>
>> ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
>> kfree(auth_str);
>> if (ret)
>> return ret;
>>
>> kfree(setting->certificate);
>> setting->certificate = NULL;
>> return count;
>> }
>>
>>
> Agreed - that all makes sense. Will update.
>
>>> + }
>>> +
>>> + if (setting->cert_installed) {
>>> + /* Certificate is installed so this is an update */
>>> + if (!setting->signature || !setting->signature[0]) {
>>> + kfree(new_cert);
>>> + return -EACCES;
>>> + }
>>> + /* Format: 'Certificate,Signature' */
>>> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>>> + new_cert, setting->signature);> + if (!auth_str) {
>>> + kfree(new_cert);
>>> + return -ENOMEM;
>>> + }
>>> + ret = tlmi_simple_call(LENOVO_UPDATE_BIOS_CERT_GUID, auth_str);
>>> + kfree(auth_str);
>>> + } else {
>>> + /* This is a fresh install */
>>> + if (!setting->valid || !setting->password[0]) {
>>> + kfree(new_cert);
>>> + return -EACCES;
>>> + }
>>> + /* Format: 'Certificate,Admin-password' */
>>> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>>> + new_cert, setting->password);
>>> + if (!auth_str) {
>>> + kfree(new_cert);
>>> + return -ENOMEM;
>>> + }
>>> + ret = tlmi_simple_call(LENOVO_SET_BIOS_CERT_GUID, auth_str);
>>> + kfree(auth_str);
>>> + }
>>> +
>>> + /* If successful update stored certificate */
>>> + if (!ret) {
>>> + kfree(setting->certificate);
>>> + setting->certificate = new_cert;
>>> + } else
>>> + kfree(new_cert);
>>> +
>>> + return ret ?: count;
>>
>> You have 2 "if (ret)" statements here (1 hidden in the return), please change this to:
>>
>> if (ret) {
>> kfree(new_cert);
>> return ret;
>> }
>>
>> kfree(setting->certificate);
>> setting->certificate = new_cert;
>> return count;
>>
> Will do
>
>>
>>> +}
>>> +
>>> +static ssize_t certificate_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>> +
>>
>> setting->certificate may be NULL here, you need to check for that and in
>> that case only emit a "\n" I guess.
> Ack.
>
>>
>>> + return sysfs_emit(buf, "%s\n", setting->certificate);
>>> +}
>>> +
>>> +static struct kobj_attribute auth_certificate = __ATTR_RW(certificate);
>>> +
>>> +static ssize_t signature_store(struct kobject *kobj,
>>> + struct kobj_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>> + char *new_signature, *p;
>>> + int ret;
>>> +
>>> + if (!capable(CAP_SYS_ADMIN))
>>> + return -EPERM;
>>> +
>>> + if (!tlmi_priv.certificate_support)
>>> + return -EOPNOTSUPP;
>>> +
>>> + new_signature = kstrdup(buf, GFP_KERNEL);
>>> + if (!new_signature)
>>> + return -ENOMEM;
>>> +
>>> + /* Strip out CR if one is present */
>>> + p = strchrnul(new_signature, '\n');
>>> + *p = '\0';
>>
>> Idea for a follow-up clean-up patch: this pattern of kstrdup user-argument
>> (buf) + strip '\n' is repeated all over the driver, maybe add a little helper
>> for this?
>>
> Yes - that would make sense. Will do.
>
>>> +
>>> + /* Free any previous signature */
>>> + kfree(setting->signature);
>>> + setting->signature = new_signature;
>>> +
>>> + return ret ?: count;
>>> +}
>>> +
>>> +static ssize_t signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>> +
>>
>> setting->signature can be NULL here.
>
> Ack
>
> <snip>
>
>>> +
>>> +static ssize_t set_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + return sysfs_emit(buf, "%s\n", tlmi_priv.set_signature);
>>> +}
>>> +
>>> +static ssize_t save_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + return sysfs_emit(buf, "%s\n", tlmi_priv.save_signature);
>>> +}
>>> +
>>> +static struct kobj_attribute attr_set_signature = __ATTR_RW(set_signature);
>>> +static struct kobj_attribute attr_save_signature = __ATTR_RW(save_signature);
>>
>> <note I missed this while reviewing the documentation patch...>
>>
>> Why not just use the /sys/class/firmware-attributes/thinklmi/authentication/Admin/signature
>> value here ?
>>
>> /sys/class/firmware-attributes/thinklmi/authentication/Admin/current_password is
>> what is used for password based setting of fw-attributes as well as for changing
>> the password; and
>>
>> /sys/class/firmware-attributes/thinklmi/authentication/Admin/signature is set
>> for changing the certificate, so it would be much more consitent to also use
>> that for setting attributes when using certificate based auth?
>>
>> Can / will the set and save signature ever be different from one another ? If yes
>> then I guess we may need 2 attributes for this, I guess maybe just signature +
>> save_signature ? Either way IMHO these 2 attributes / the 1 extra attribute
>> for a separate save-signature really belongs under
>> /sys/class/firmware-attributes/thinklmi/authentication/Admin/ IMHO and
>> not under /sys/class/firmware-attributes/thinklmi/attributes/
>>
>> What do you think about moving these there ?
>>
> I have no issues with moving them. I had originally intended to have
> them in auth but as I needed two signatures (the set and save are
> different) I decided to make it explicit and to keep them in the same
> place as the attribute being modified. But I can see it making sense to
> just keep those under Authentication instead.
>
> I'll update and get rid of set_signature and move save_signature.
Sounds good, thank you.
> Many thanks for the review
You're welcome.
Regards,
Hans
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-14 16:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-12 0:04 [PATCH 1/2] Documentation: syfs-class-firmware-attributes: Lenovo Certificate support Mark Pearson
2022-03-12 0:04 ` [PATCH 2/2] platform/x86: think-lmi: Certificate authentication support Mark Pearson
2022-03-14 15:15 ` Hans de Goede
2022-03-14 15:46 ` [External] " Mark Pearson
2022-03-14 16:52 ` Hans de Goede
2022-03-14 13:13 ` [PATCH 1/2] Documentation: syfs-class-firmware-attributes: Lenovo Certificate support Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.