[PATCH 1/4] platform/x86: think-lmi: improve check if BIOS account security enabled

[PATCH 1/4] platform/x86: think-lmi: improve check if BIOS account security enabled

[Mark Pearson]

Improve determination of whether authentication account is enabled by
checking if either password or certificate is enabled.

Renamed valid to pwd_enabled for better readability.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/platform/x86/think-lmi.c | 26 +++++++++++++-------------
 drivers/platform/x86/think-lmi.h |  2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 4cfb53206cb8..727a9400d406 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -391,7 +391,7 @@ static ssize_t is_enabled_show(struct kobject *kobj, struct kobj_attribute *attr
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 
-	return sysfs_emit(buf, "%d\n", setting->valid);
+	return sysfs_emit(buf, "%d\n", setting->pwd_enabled || setting->cert_installed);
 }
 
 static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled);
@@ -469,7 +469,7 @@ static ssize_t new_password_store(struct kobject *kobj,
 		if (ret)
 			goto out;
 
-		if (tlmi_priv.pwd_admin->valid) {
+		if (tlmi_priv.pwd_admin->pwd_enabled) {
 			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
 					tlmi_priv.pwd_admin->password);
 			if (ret)
@@ -777,7 +777,7 @@ static ssize_t certificate_store(struct kobject *kobj,
 				new_cert, setting->signature);
 	} else {
 		/* This is a fresh install */
-		if (!setting->valid || !setting->password[0]) {
+		if (!setting->pwd_enabled || !setting->password[0]) {
 			kfree(new_cert);
 			return -EACCES;
 		}
@@ -1019,7 +1019,7 @@ static ssize_t current_value_store(struct kobject *kobj,
 		 * Workstation's require the opcode to be set before changing the
 		 * attribute.
 		 */
-		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
 			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
 						  tlmi_priv.pwd_admin->password);
 			if (ret)
@@ -1042,7 +1042,7 @@ static ssize_t current_value_store(struct kobject *kobj,
 		else
 			ret = tlmi_save_bios_settings("");
 	} else { /* old non-opcode based authentication method (deprecated) */
-		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+		if (tlmi_priv.pwd_admin->pwd_enabled && 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],
@@ -1215,7 +1215,7 @@ static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *
 			if (ret)
 				goto out;
 		} else if (tlmi_priv.opcode_support) {
-			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
 				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
 							  tlmi_priv.pwd_admin->password);
 				if (ret)
@@ -1223,7 +1223,7 @@ static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *
 			}
 			ret = tlmi_save_bios_settings("");
 		} else { /* old non-opcode based authentication method (deprecated) */
-			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+			if (tlmi_priv.pwd_admin->pwd_enabled && 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],
@@ -1273,7 +1273,7 @@ static ssize_t debug_cmd_store(struct kobject *kobj, struct kobj_attribute *attr
 	if (!new_setting)
 		return -ENOMEM;
 
-	if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+	if (tlmi_priv.pwd_admin->pwd_enabled && 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],
@@ -1637,14 +1637,14 @@ static int tlmi_analyze(void)
 		goto fail_clear_attr;
 
 	if (tlmi_priv.pwdcfg.core.password_state & TLMI_PAP_PWD)
-		tlmi_priv.pwd_admin->valid = true;
+		tlmi_priv.pwd_admin->pwd_enabled = true;
 
 	tlmi_priv.pwd_power = tlmi_create_auth("pop", "power-on");
 	if (!tlmi_priv.pwd_power)
 		goto fail_clear_attr;
 
 	if (tlmi_priv.pwdcfg.core.password_state & TLMI_POP_PWD)
-		tlmi_priv.pwd_power->valid = true;
+		tlmi_priv.pwd_power->pwd_enabled = true;
 
 	if (tlmi_priv.opcode_support) {
 		tlmi_priv.pwd_system = tlmi_create_auth("smp", "system");
@@ -1652,7 +1652,7 @@ static int tlmi_analyze(void)
 			goto fail_clear_attr;
 
 		if (tlmi_priv.pwdcfg.core.password_state & TLMI_SMP_PWD)
-			tlmi_priv.pwd_system->valid = true;
+			tlmi_priv.pwd_system->pwd_enabled = true;
 
 		tlmi_priv.pwd_hdd = tlmi_create_auth("hdd", "hdd");
 		if (!tlmi_priv.pwd_hdd)
@@ -1670,7 +1670,7 @@ static int tlmi_analyze(void)
 			/* Check if PWD is configured and set index to first drive found */
 			if (tlmi_priv.pwdcfg.ext.hdd_user_password ||
 					tlmi_priv.pwdcfg.ext.hdd_master_password) {
-				tlmi_priv.pwd_hdd->valid = true;
+				tlmi_priv.pwd_hdd->pwd_enabled = true;
 				if (tlmi_priv.pwdcfg.ext.hdd_master_password)
 					tlmi_priv.pwd_hdd->index =
 						ffs(tlmi_priv.pwdcfg.ext.hdd_master_password) - 1;
@@ -1680,7 +1680,7 @@ static int tlmi_analyze(void)
 			}
 			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
 					tlmi_priv.pwdcfg.ext.nvme_master_password) {
-				tlmi_priv.pwd_nvme->valid = true;
+				tlmi_priv.pwd_nvme->pwd_enabled = true;
 				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
 					tlmi_priv.pwd_nvme->index =
 						ffs(tlmi_priv.pwdcfg.ext.nvme_master_password) - 1;
diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
index e1975ffebeb4..4728f40143a3 100644
--- a/drivers/platform/x86/think-lmi.h
+++ b/drivers/platform/x86/think-lmi.h
@@ -65,7 +65,7 @@ struct tlmi_pwdcfg {
 /* password setting details */
 struct tlmi_pwd_setting {
 	struct kobject kobj;
-	bool valid;
+	bool pwd_enabled;
 	char password[TLMI_PWD_BUFSIZE];
 	const char *pwd_type;
 	const char *role;
-- 
2.47.0

[PATCH 2/4] platform/x86: think-lmi: Add certificate as mechanism

[Mark Pearson]

As both password or certificate authentication are available as mechanisms
update the documentation to add certificate as an option

Update driver to return correct mechanism appropriately.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 Documentation/ABI/testing/sysfs-class-firmware-attributes | 2 +-
 drivers/platform/x86/think-lmi.c                          | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 9c82c7b42ff8..1a8b59f5d6e3 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -193,7 +193,7 @@ Description:
 
 		mechanism:
 					The means of authentication.  This attribute is mandatory.
-					Only supported type currently is "password".
+					Supported types are "password" or "certificate".
 
 		max_password_length:
 					A file that can be read to obtain the
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 727a9400d406..46ab82fb2898 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -524,6 +524,10 @@ static struct kobj_attribute auth_max_pass_length = __ATTR_RO(max_password_lengt
 static ssize_t mechanism_show(struct kobject *kobj, struct kobj_attribute *attr,
 			 char *buf)
 {
+	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
+
+	if (setting->cert_installed)
+		return sysfs_emit(buf, "certificate\n");
 	return sysfs_emit(buf, "password\n");
 }
 static struct kobj_attribute auth_mechanism = __ATTR_RO(mechanism);
-- 
2.47.0

[PATCH 3/4] platform/x86: think-lmi: Allow empty admin password

[Mark Pearson]

SVP = BIOS Supervisor/Admin password
SMP = BIOS System password

If SMP ACL is enabled in the BIOS then the system allows you to set the
SMP without a SVP password configured. Change code to allow this.
BIOS will return permissions error if SVP is required.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/platform/x86/think-lmi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 46ab82fb2898..751e351dfc42 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -469,7 +469,12 @@ static ssize_t new_password_store(struct kobject *kobj,
 		if (ret)
 			goto out;
 
-		if (tlmi_priv.pwd_admin->pwd_enabled) {
+		/*
+		 * Note admin password not always required if SMPControl enabled in BIOS,
+		 * So only set if it's configured.
+		 * Let BIOS figure it out - we'll get an error if operation not permitted
+		 */
+		if (tlmi_priv.pwd_admin->pwd_enabled && strlen(tlmi_priv.pwd_admin->password)) {
 			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
 					tlmi_priv.pwd_admin->password);
 			if (ret)
-- 
2.47.0

Re: [PATCH 3/4] platform/x86: think-lmi: Allow empty admin password

[Ilpo Järvinen]

On Mon, 21 Oct 2024, Mark Pearson wrote:

> SVP = BIOS Supervisor/Admin password
> SMP = BIOS System password
> 
> If SMP ACL is enabled in the BIOS then the system allows you to set the
> SMP without a SVP password configured. Change code to allow this.
> BIOS will return permissions error if SVP is required.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  drivers/platform/x86/think-lmi.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 46ab82fb2898..751e351dfc42 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -469,7 +469,12 @@ static ssize_t new_password_store(struct kobject *kobj,
>  		if (ret)
>  			goto out;
>  
> -		if (tlmi_priv.pwd_admin->pwd_enabled) {
> +		/*
> +		 * Note admin password not always required if SMPControl enabled in BIOS,

is not always required

> +		 * So only set if it's configured.
> +		 * Let BIOS figure it out - we'll get an error if operation not permitted

operation is not permitted.

Please reflow the comment to fit into 80 chars (this only relates to 
comments to keep their line lengths readable without large eye movement).

> +		 */
> +		if (tlmi_priv.pwd_admin->pwd_enabled && strlen(tlmi_priv.pwd_admin->password)) {
>  			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
>  					tlmi_priv.pwd_admin->password);
>  			if (ret)
> 

-- 
 i.

Re: [PATCH 3/4] platform/x86: think-lmi: Allow empty admin password

[Mark Pearson]

Hi Ilpo

On Tue, Oct 22, 2024, at 3:59 AM, Ilpo Järvinen wrote:
> On Mon, 21 Oct 2024, Mark Pearson wrote:
>
>> SVP = BIOS Supervisor/Admin password
>> SMP = BIOS System password
>> 
>> If SMP ACL is enabled in the BIOS then the system allows you to set the
>> SMP without a SVP password configured. Change code to allow this.
>> BIOS will return permissions error if SVP is required.
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>>  drivers/platform/x86/think-lmi.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index 46ab82fb2898..751e351dfc42 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -469,7 +469,12 @@ static ssize_t new_password_store(struct kobject *kobj,
>>  		if (ret)
>>  			goto out;
>>  
>> -		if (tlmi_priv.pwd_admin->pwd_enabled) {
>> +		/*
>> +		 * Note admin password not always required if SMPControl enabled in BIOS,
>
> is not always required
>
>> +		 * So only set if it's configured.
>> +		 * Let BIOS figure it out - we'll get an error if operation not permitted
>
> operation is not permitted.
>
> Please reflow the comment to fit into 80 chars (this only relates to 
> comments to keep their line lengths readable without large eye movement).
>
>> +		 */
>> +		if (tlmi_priv.pwd_admin->pwd_enabled && strlen(tlmi_priv.pwd_admin->password)) {
>>  			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
>>  					tlmi_priv.pwd_admin->password);
>>  			if (ret)
>> 
>
> -- 
>  i.

Thanks for the review.
I'll update those. I'll give it a couple of days in case there is any other feedback
Mark

[PATCH 4/4] platform/x86: think-lmi: Multi-certificate support

[Mark Pearson]

Lenovo are adding support for both Admin and System certificates to
the certificate based authentication feature

This commit adds the support for this.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 .../testing/sysfs-class-firmware-attributes   |   1 +
 drivers/platform/x86/think-lmi.c              | 141 ++++++++++++++----
 drivers/platform/x86/think-lmi.h              |   4 +
 3 files changed, 116 insertions(+), 30 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 1a8b59f5d6e3..2713efa509b4 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -303,6 +303,7 @@ Description:
 					being configured allowing anyone to make changes.
 					After any of these operations the system must reboot for the changes to
 					take effect.
+					Admin and System certificates are supported from 2025 systems onward.
 
 		certificate_thumbprint:
 					Read only attribute used to display the MD5, SHA1 and SHA256 thumbprints
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 751e351dfc42..fca190232c24 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -169,11 +169,12 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
  */
 #define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4"
 
-#define TLMI_POP_PWD BIT(0) /* Supervisor */
-#define TLMI_PAP_PWD BIT(1) /* Power-on */
-#define TLMI_HDD_PWD BIT(2) /* HDD/NVME */
-#define TLMI_SMP_PWD BIT(6) /* System Management */
-#define TLMI_CERT    BIT(7) /* Certificate Based */
+#define TLMI_POP_PWD  BIT(0) /* Supervisor */
+#define TLMI_PAP_PWD  BIT(1) /* Power-on */
+#define TLMI_HDD_PWD  BIT(2) /* HDD/NVME */
+#define TLMI_SMP_PWD  BIT(6) /* System Management */
+#define TLMI_CERT_SVC BIT(7) /* Admin Certificate Based */
+#define TLMI_CERT_SMC BIT(8) /* System Certificate Based */
 
 static const struct tlmi_err_codes tlmi_errs[] = {
 	{"Success", 0},
@@ -678,18 +679,35 @@ static ssize_t cert_thumbprint(char *buf, const char *arg, int count)
 	return count;
 }
 
+#define NUM_THUMBTYPES 3
+static char *thumbtypes[NUM_THUMBTYPES] = {"Md5", "Sha1", "Sha256"};
+
 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;
+	char *wmistr;
+	int count = 0, i;
 
 	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);
+	for (i = 0; i < NUM_THUMBTYPES; i++) {
+		if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
+			/* Format: 'SVC | SMC, Thumbtype' */
+			wmistr = kasprintf(GFP_KERNEL, "%s,%s",
+					   setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
+					   thumbtypes[i]);
+		} else {
+			/* Format: 'Thumbtype' */
+			wmistr = kasprintf(GFP_KERNEL, "%s", thumbtypes[i]);
+		}
+		if (!wmistr)
+			return -ENOMEM;
+		count += cert_thumbprint(buf, wmistr, count);
+		kfree(wmistr);
+	}
+
 	return count;
 }
 
@@ -720,8 +738,15 @@ static ssize_t cert_to_password_store(struct kobject *kobj,
 	if (!passwd)
 		return -ENOMEM;
 
-	/* Format: 'Password,Signature' */
-	auth_str = kasprintf(GFP_KERNEL, "%s,%s", passwd, setting->signature);
+	if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
+		/* Format: 'SVC | SMC, password, signature' */
+		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
+				     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
+				     passwd, setting->signature);
+	} else {
+		/* Format: 'Password,Signature' */
+		auth_str = kasprintf(GFP_KERNEL, "%s,%s", passwd, setting->signature);
+	}
 	if (!auth_str) {
 		kfree_sensitive(passwd);
 		return -ENOMEM;
@@ -735,12 +760,19 @@ static ssize_t cert_to_password_store(struct kobject *kobj,
 
 static struct kobj_attribute auth_cert_to_password = __ATTR_WO(cert_to_password);
 
+enum cert_install_mode {
+	TLMI_CERT_INSTALL,
+	TLMI_CERT_UPDATE,
+};
+
 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);
+	enum cert_install_mode install_mode = TLMI_CERT_INSTALL;
 	char *auth_str, *new_cert;
+	char *signature;
 	char *guid;
 	int ret;
 
@@ -756,10 +788,18 @@ static ssize_t certificate_store(struct kobject *kobj,
 		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 (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
+			/* Format: 'SVC | SMC, serial#, signature' */
+			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
+					     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
+					     dmi_get_system_info(DMI_PRODUCT_SERIAL),
+					     setting->signature);
+		} else {
+			/* Format: 'serial#, signature' */
+			auth_str = kasprintf(GFP_KERNEL, "%s,%s",
+					     dmi_get_system_info(DMI_PRODUCT_SERIAL),
+					     setting->signature);
+		}
 		if (!auth_str)
 			return -ENOMEM;
 
@@ -776,24 +816,59 @@ static ssize_t certificate_store(struct kobject *kobj,
 
 	if (setting->cert_installed) {
 		/* Certificate is installed so this is an update */
-		if (!setting->signature || !setting->signature[0]) {
+		install_mode = TLMI_CERT_UPDATE;
+		/* If admin account enabled - need to use it's signature */
+		if (tlmi_priv.pwd_admin->pwd_enabled)
+			signature = tlmi_priv.pwd_admin->signature;
+		else
+			signature = setting->signature;
+	} else { /* Cert install */
+		/* Check if SMC and SVC already installed */
+		if ((setting == tlmi_priv.pwd_system) && tlmi_priv.pwd_admin->cert_installed) {
+			/* This gets treated as a cert update */
+			install_mode = TLMI_CERT_UPDATE;
+			signature = tlmi_priv.pwd_admin->signature;
+		} else { /* Regular cert install */
+			install_mode = TLMI_CERT_INSTALL;
+			signature = setting->signature;
+		}
+	}
+
+	if (install_mode == TLMI_CERT_UPDATE) {
+		/* This is a certificate update */
+		if (!signature || !signature[0]) {
 			kfree(new_cert);
 			return -EACCES;
 		}
 		guid = LENOVO_UPDATE_BIOS_CERT_GUID;
-		/* Format: 'Certificate,Signature' */
-		auth_str = kasprintf(GFP_KERNEL, "%s,%s",
-				new_cert, setting->signature);
+		if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
+			/* Format: 'SVC | SMC, certificate, signature' */
+			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
+					     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
+					     new_cert, signature);
+		} else {
+			/* Format: 'Certificate,Signature' */
+			auth_str = kasprintf(GFP_KERNEL, "%s,%s",
+					     new_cert, signature);
+		}
 	} else {
 		/* This is a fresh install */
-		if (!setting->pwd_enabled || !setting->password[0]) {
+		/* To set admin cert, a password must be enabled */
+		if ((setting == tlmi_priv.pwd_admin) &&
+		    (!setting->pwd_enabled || !setting->password[0])) {
 			kfree(new_cert);
 			return -EACCES;
 		}
 		guid = LENOVO_SET_BIOS_CERT_GUID;
-		/* Format: 'Certificate,Admin-password' */
-		auth_str = kasprintf(GFP_KERNEL, "%s,%s",
-				new_cert, setting->password);
+		if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
+			/* Format: 'SVC | SMC, Certificate, password' */
+			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
+					     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
+					     new_cert, setting->password);
+		} else {
+			/* Format: 'Certificate, password' */
+			auth_str = kasprintf(GFP_KERNEL, "%s,%s", new_cert, setting->password);
+		}
 	}
 	kfree(new_cert);
 	if (!auth_str)
@@ -873,14 +948,19 @@ static umode_t auth_attr_is_visible(struct kobject *kobj,
 		return 0;
 	}
 
-	/* We only display certificates on Admin account, if supported */
+	/* We only display certificates, if supported */
 	if (attr == &auth_certificate.attr ||
 	    attr == &auth_signature.attr ||
 	    attr == &auth_save_signature.attr ||
 	    attr == &auth_cert_thumb.attr ||
 	    attr == &auth_cert_to_password.attr) {
-		if ((setting == tlmi_priv.pwd_admin) && tlmi_priv.certificate_support)
+		if (tlmi_priv.certificate_support) {
+			if (setting == tlmi_priv.pwd_admin)
+				return attr->mode;
+		if ((tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) &&
+		    (setting == tlmi_priv.pwd_system))
 			return attr->mode;
+		}
 		return 0;
 	}
 
@@ -1700,12 +1780,13 @@ 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;
-
+	if (tlmi_priv.certificate_support) {
+		tlmi_priv.pwd_admin->cert_installed =
+			tlmi_priv.pwdcfg.core.password_state & TLMI_CERT_SVC;
+		tlmi_priv.pwd_system->cert_installed =
+			tlmi_priv.pwdcfg.core.password_state & TLMI_CERT_SMC;
+	}
 	return 0;
-
 fail_clear_attr:
 	for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) {
 		if (tlmi_priv.setting[i]) {
diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
index 4728f40143a3..f267d8b46957 100644
--- a/drivers/platform/x86/think-lmi.h
+++ b/drivers/platform/x86/think-lmi.h
@@ -41,6 +41,10 @@ enum save_mode {
 };
 
 /* password configuration details */
+#define TLMI_PWDCFG_MODE_LEGACY    0
+#define TLMI_PWDCFG_MODE_PASSWORD  1
+#define TLMI_PWDCFG_MODE_MULTICERT 3
+
 struct tlmi_pwdcfg_core {
 	uint32_t password_mode;
 	uint32_t password_state;
-- 
2.47.0

Re: [PATCH 4/4] platform/x86: think-lmi: Multi-certificate support

[Ilpo Järvinen]

On Mon, 21 Oct 2024, Mark Pearson wrote:

> Lenovo are adding support for both Admin and System certificates to
> the certificate based authentication feature
> 
> This commit adds the support for this.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  .../testing/sysfs-class-firmware-attributes   |   1 +
>  drivers/platform/x86/think-lmi.c              | 141 ++++++++++++++----
>  drivers/platform/x86/think-lmi.h              |   4 +
>  3 files changed, 116 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> index 1a8b59f5d6e3..2713efa509b4 100644
> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> @@ -303,6 +303,7 @@ Description:
>  					being configured allowing anyone to make changes.
>  					After any of these operations the system must reboot for the changes to
>  					take effect.
> +					Admin and System certificates are supported from 2025 systems onward.
>  
>  		certificate_thumbprint:
>  					Read only attribute used to display the MD5, SHA1 and SHA256 thumbprints
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 751e351dfc42..fca190232c24 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -169,11 +169,12 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
>   */
>  #define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4"
>  
> -#define TLMI_POP_PWD BIT(0) /* Supervisor */
> -#define TLMI_PAP_PWD BIT(1) /* Power-on */
> -#define TLMI_HDD_PWD BIT(2) /* HDD/NVME */
> -#define TLMI_SMP_PWD BIT(6) /* System Management */
> -#define TLMI_CERT    BIT(7) /* Certificate Based */
> +#define TLMI_POP_PWD  BIT(0) /* Supervisor */
> +#define TLMI_PAP_PWD  BIT(1) /* Power-on */
> +#define TLMI_HDD_PWD  BIT(2) /* HDD/NVME */
> +#define TLMI_SMP_PWD  BIT(6) /* System Management */
> +#define TLMI_CERT_SVC BIT(7) /* Admin Certificate Based */
> +#define TLMI_CERT_SMC BIT(8) /* System Certificate Based */
>  
>  static const struct tlmi_err_codes tlmi_errs[] = {
>  	{"Success", 0},
> @@ -678,18 +679,35 @@ static ssize_t cert_thumbprint(char *buf, const char *arg, int count)
>  	return count;
>  }
>  
> +#define NUM_THUMBTYPES 3
> +static char *thumbtypes[NUM_THUMBTYPES] = {"Md5", "Sha1", "Sha256"};
> +
>  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;
> +	char *wmistr;
> +	int count = 0, i;

Reverse xmas-tree order.

>  
>  	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);
> +	for (i = 0; i < NUM_THUMBTYPES; i++) {

Use ARRAY_SIZE() + add include for it.

These days, the usual custom is to use unsigned int for loop variables 
that are never meant to be negative.

> +		if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
> +			/* Format: 'SVC | SMC, Thumbtype' */
> +			wmistr = kasprintf(GFP_KERNEL, "%s,%s",
> +					   setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
> +					   thumbtypes[i]);
> +		} else {
> +			/* Format: 'Thumbtype' */
> +			wmistr = kasprintf(GFP_KERNEL, "%s", thumbtypes[i]);
> +		}
> +		if (!wmistr)
> +			return -ENOMEM;
> +		count += cert_thumbprint(buf, wmistr, count);
> +		kfree(wmistr);
> +	}
> +
>  	return count;
>  }
>  
> @@ -720,8 +738,15 @@ static ssize_t cert_to_password_store(struct kobject *kobj,
>  	if (!passwd)
>  		return -ENOMEM;
>  
> -	/* Format: 'Password,Signature' */
> -	auth_str = kasprintf(GFP_KERNEL, "%s,%s", passwd, setting->signature);
> +	if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
> +		/* Format: 'SVC | SMC, password, signature' */
> +		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
> +				     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
> +				     passwd, setting->signature);
> +	} else {
> +		/* Format: 'Password,Signature' */
> +		auth_str = kasprintf(GFP_KERNEL, "%s,%s", passwd, setting->signature);
> +	}
>  	if (!auth_str) {
>  		kfree_sensitive(passwd);
>  		return -ENOMEM;
> @@ -735,12 +760,19 @@ static ssize_t cert_to_password_store(struct kobject *kobj,
>  
>  static struct kobj_attribute auth_cert_to_password = __ATTR_WO(cert_to_password);
>  
> +enum cert_install_mode {
> +	TLMI_CERT_INSTALL,
> +	TLMI_CERT_UPDATE,
> +};
> +
>  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);
> +	enum cert_install_mode install_mode = TLMI_CERT_INSTALL;
>  	char *auth_str, *new_cert;
> +	char *signature;
>  	char *guid;
>  	int ret;
>  
> @@ -756,10 +788,18 @@ static ssize_t certificate_store(struct kobject *kobj,
>  		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 (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
> +			/* Format: 'SVC | SMC, serial#, signature' */
> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
> +					     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
> +					     dmi_get_system_info(DMI_PRODUCT_SERIAL),
> +					     setting->signature);
> +		} else {
> +			/* Format: 'serial#, signature' */
> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s",
> +					     dmi_get_system_info(DMI_PRODUCT_SERIAL),
> +					     setting->signature);
> +		}
>  		if (!auth_str)
>  			return -ENOMEM;
>  
> @@ -776,24 +816,59 @@ static ssize_t certificate_store(struct kobject *kobj,
>  
>  	if (setting->cert_installed) {
>  		/* Certificate is installed so this is an update */
> -		if (!setting->signature || !setting->signature[0]) {
> +		install_mode = TLMI_CERT_UPDATE;
> +		/* If admin account enabled - need to use it's signature */
> +		if (tlmi_priv.pwd_admin->pwd_enabled)
> +			signature = tlmi_priv.pwd_admin->signature;
> +		else
> +			signature = setting->signature;
> +	} else { /* Cert install */
> +		/* Check if SMC and SVC already installed */
> +		if ((setting == tlmi_priv.pwd_system) && tlmi_priv.pwd_admin->cert_installed) {
> +			/* This gets treated as a cert update */
> +			install_mode = TLMI_CERT_UPDATE;
> +			signature = tlmi_priv.pwd_admin->signature;
> +		} else { /* Regular cert install */
> +			install_mode = TLMI_CERT_INSTALL;
> +			signature = setting->signature;
> +		}
> +	}
> +
> +	if (install_mode == TLMI_CERT_UPDATE) {
> +		/* This is a certificate update */
> +		if (!signature || !signature[0]) {
>  			kfree(new_cert);
>  			return -EACCES;
>  		}
>  		guid = LENOVO_UPDATE_BIOS_CERT_GUID;
> -		/* Format: 'Certificate,Signature' */
> -		auth_str = kasprintf(GFP_KERNEL, "%s,%s",
> -				new_cert, setting->signature);
> +		if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
> +			/* Format: 'SVC | SMC, certificate, signature' */
> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
> +					     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
> +					     new_cert, signature);
> +		} else {
> +			/* Format: 'Certificate,Signature' */
> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s",
> +					     new_cert, signature);
> +		}
>  	} else {
>  		/* This is a fresh install */
> -		if (!setting->pwd_enabled || !setting->password[0]) {
> +		/* To set admin cert, a password must be enabled */
> +		if ((setting == tlmi_priv.pwd_admin) &&
> +		    (!setting->pwd_enabled || !setting->password[0])) {
>  			kfree(new_cert);
>  			return -EACCES;
>  		}
>  		guid = LENOVO_SET_BIOS_CERT_GUID;
> -		/* Format: 'Certificate,Admin-password' */
> -		auth_str = kasprintf(GFP_KERNEL, "%s,%s",
> -				new_cert, setting->password);
> +		if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
> +			/* Format: 'SVC | SMC, Certificate, password' */
> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
> +					     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
> +					     new_cert, setting->password);
> +		} else {
> +			/* Format: 'Certificate, password' */
> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s", new_cert, setting->password);
> +		}

Have you considered creating a helper for this if/else construct to create 
the string? (you've basically repeated it multiple times by now with 
limited change to only parameters AFAICT).

>  	}
>  	kfree(new_cert);
>  	if (!auth_str)
> @@ -873,14 +948,19 @@ static umode_t auth_attr_is_visible(struct kobject *kobj,
>  		return 0;
>  	}
>  
> -	/* We only display certificates on Admin account, if supported */
> +	/* We only display certificates, if supported */
>  	if (attr == &auth_certificate.attr ||
>  	    attr == &auth_signature.attr ||
>  	    attr == &auth_save_signature.attr ||
>  	    attr == &auth_cert_thumb.attr ||
>  	    attr == &auth_cert_to_password.attr) {
> -		if ((setting == tlmi_priv.pwd_admin) && tlmi_priv.certificate_support)
> +		if (tlmi_priv.certificate_support) {
> +			if (setting == tlmi_priv.pwd_admin)

These two if()s combined look the same as the old ode, why are you redoing 
it?

> +				return attr->mode;
> +		if ((tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) &&
> +		    (setting == tlmi_priv.pwd_system))
>  			return attr->mode;
> +		}
>  		return 0;
>  	}
>  
> @@ -1700,12 +1780,13 @@ 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;
> -
> +	if (tlmi_priv.certificate_support) {
> +		tlmi_priv.pwd_admin->cert_installed =
> +			tlmi_priv.pwdcfg.core.password_state & TLMI_CERT_SVC;
> +		tlmi_priv.pwd_system->cert_installed =
> +			tlmi_priv.pwdcfg.core.password_state & TLMI_CERT_SMC;
> +	}
>  	return 0;
> -

Stray change.

-- 
 i.

>  fail_clear_attr:
>  	for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) {
>  		if (tlmi_priv.setting[i]) {
> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
> index 4728f40143a3..f267d8b46957 100644
> --- a/drivers/platform/x86/think-lmi.h
> +++ b/drivers/platform/x86/think-lmi.h
> @@ -41,6 +41,10 @@ enum save_mode {
>  };
>  
>  /* password configuration details */
> +#define TLMI_PWDCFG_MODE_LEGACY    0
> +#define TLMI_PWDCFG_MODE_PASSWORD  1
> +#define TLMI_PWDCFG_MODE_MULTICERT 3
> +
>  struct tlmi_pwdcfg_core {
>  	uint32_t password_mode;
>  	uint32_t password_state;
> 

Re: [PATCH 4/4] platform/x86: think-lmi: Multi-certificate support

[Mark Pearson]

Hi Ilpo,

Thanks for the review.

On Tue, Oct 22, 2024, at 4:14 AM, Ilpo Järvinen wrote:
> On Mon, 21 Oct 2024, Mark Pearson wrote:
>
>> Lenovo are adding support for both Admin and System certificates to
>> the certificate based authentication feature
>> 
>> This commit adds the support for this.
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>>  .../testing/sysfs-class-firmware-attributes   |   1 +
>>  drivers/platform/x86/think-lmi.c              | 141 ++++++++++++++----
>>  drivers/platform/x86/think-lmi.h              |   4 +
>>  3 files changed, 116 insertions(+), 30 deletions(-)
>> 
>> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> index 1a8b59f5d6e3..2713efa509b4 100644
>> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> @@ -303,6 +303,7 @@ Description:
>>  					being configured allowing anyone to make changes.
>>  					After any of these operations the system must reboot for the changes to
>>  					take effect.
>> +					Admin and System certificates are supported from 2025 systems onward.
>>  
>>  		certificate_thumbprint:
>>  					Read only attribute used to display the MD5, SHA1 and SHA256 thumbprints
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index 751e351dfc42..fca190232c24 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -169,11 +169,12 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
>>   */
>>  #define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4"
>>  
>> -#define TLMI_POP_PWD BIT(0) /* Supervisor */
>> -#define TLMI_PAP_PWD BIT(1) /* Power-on */
>> -#define TLMI_HDD_PWD BIT(2) /* HDD/NVME */
>> -#define TLMI_SMP_PWD BIT(6) /* System Management */
>> -#define TLMI_CERT    BIT(7) /* Certificate Based */
>> +#define TLMI_POP_PWD  BIT(0) /* Supervisor */
>> +#define TLMI_PAP_PWD  BIT(1) /* Power-on */
>> +#define TLMI_HDD_PWD  BIT(2) /* HDD/NVME */
>> +#define TLMI_SMP_PWD  BIT(6) /* System Management */
>> +#define TLMI_CERT_SVC BIT(7) /* Admin Certificate Based */
>> +#define TLMI_CERT_SMC BIT(8) /* System Certificate Based */
>>  
>>  static const struct tlmi_err_codes tlmi_errs[] = {
>>  	{"Success", 0},
>> @@ -678,18 +679,35 @@ static ssize_t cert_thumbprint(char *buf, const char *arg, int count)
>>  	return count;
>>  }
>>  
>> +#define NUM_THUMBTYPES 3
>> +static char *thumbtypes[NUM_THUMBTYPES] = {"Md5", "Sha1", "Sha256"};
>> +
>>  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;
>> +	char *wmistr;
>> +	int count = 0, i;
>
> Reverse xmas-tree order.

Ack

>
>>  
>>  	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);
>> +	for (i = 0; i < NUM_THUMBTYPES; i++) {
>
> Use ARRAY_SIZE() + add include for it.
>
> These days, the usual custom is to use unsigned int for loop variables 
> that are never meant to be negative.
>
Will update.

>> +		if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
>> +			/* Format: 'SVC | SMC, Thumbtype' */
>> +			wmistr = kasprintf(GFP_KERNEL, "%s,%s",
>> +					   setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
>> +					   thumbtypes[i]);
>> +		} else {
>> +			/* Format: 'Thumbtype' */
>> +			wmistr = kasprintf(GFP_KERNEL, "%s", thumbtypes[i]);
>> +		}
>> +		if (!wmistr)
>> +			return -ENOMEM;
>> +		count += cert_thumbprint(buf, wmistr, count);
>> +		kfree(wmistr);
>> +	}
>> +
>>  	return count;
>>  }
>>  
>> @@ -720,8 +738,15 @@ static ssize_t cert_to_password_store(struct kobject *kobj,
>>  	if (!passwd)
>>  		return -ENOMEM;
>>  
>> -	/* Format: 'Password,Signature' */
>> -	auth_str = kasprintf(GFP_KERNEL, "%s,%s", passwd, setting->signature);
>> +	if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
>> +		/* Format: 'SVC | SMC, password, signature' */
>> +		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
>> +				     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
>> +				     passwd, setting->signature);
>> +	} else {
>> +		/* Format: 'Password,Signature' */
>> +		auth_str = kasprintf(GFP_KERNEL, "%s,%s", passwd, setting->signature);
>> +	}
>>  	if (!auth_str) {
>>  		kfree_sensitive(passwd);
>>  		return -ENOMEM;
>> @@ -735,12 +760,19 @@ static ssize_t cert_to_password_store(struct kobject *kobj,
>>  
>>  static struct kobj_attribute auth_cert_to_password = __ATTR_WO(cert_to_password);
>>  
>> +enum cert_install_mode {
>> +	TLMI_CERT_INSTALL,
>> +	TLMI_CERT_UPDATE,
>> +};
>> +
>>  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);
>> +	enum cert_install_mode install_mode = TLMI_CERT_INSTALL;
>>  	char *auth_str, *new_cert;
>> +	char *signature;
>>  	char *guid;
>>  	int ret;
>>  
>> @@ -756,10 +788,18 @@ static ssize_t certificate_store(struct kobject *kobj,
>>  		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 (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
>> +			/* Format: 'SVC | SMC, serial#, signature' */
>> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
>> +					     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
>> +					     dmi_get_system_info(DMI_PRODUCT_SERIAL),
>> +					     setting->signature);
>> +		} else {
>> +			/* Format: 'serial#, signature' */
>> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>> +					     dmi_get_system_info(DMI_PRODUCT_SERIAL),
>> +					     setting->signature);
>> +		}
>>  		if (!auth_str)
>>  			return -ENOMEM;
>>  
>> @@ -776,24 +816,59 @@ static ssize_t certificate_store(struct kobject *kobj,
>>  
>>  	if (setting->cert_installed) {
>>  		/* Certificate is installed so this is an update */
>> -		if (!setting->signature || !setting->signature[0]) {
>> +		install_mode = TLMI_CERT_UPDATE;
>> +		/* If admin account enabled - need to use it's signature */
>> +		if (tlmi_priv.pwd_admin->pwd_enabled)
>> +			signature = tlmi_priv.pwd_admin->signature;
>> +		else
>> +			signature = setting->signature;
>> +	} else { /* Cert install */
>> +		/* Check if SMC and SVC already installed */
>> +		if ((setting == tlmi_priv.pwd_system) && tlmi_priv.pwd_admin->cert_installed) {
>> +			/* This gets treated as a cert update */
>> +			install_mode = TLMI_CERT_UPDATE;
>> +			signature = tlmi_priv.pwd_admin->signature;
>> +		} else { /* Regular cert install */
>> +			install_mode = TLMI_CERT_INSTALL;
>> +			signature = setting->signature;
>> +		}
>> +	}
>> +
>> +	if (install_mode == TLMI_CERT_UPDATE) {
>> +		/* This is a certificate update */
>> +		if (!signature || !signature[0]) {
>>  			kfree(new_cert);
>>  			return -EACCES;
>>  		}
>>  		guid = LENOVO_UPDATE_BIOS_CERT_GUID;
>> -		/* Format: 'Certificate,Signature' */
>> -		auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>> -				new_cert, setting->signature);
>> +		if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
>> +			/* Format: 'SVC | SMC, certificate, signature' */
>> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
>> +					     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
>> +					     new_cert, signature);
>> +		} else {
>> +			/* Format: 'Certificate,Signature' */
>> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>> +					     new_cert, signature);
>> +		}
>>  	} else {
>>  		/* This is a fresh install */
>> -		if (!setting->pwd_enabled || !setting->password[0]) {
>> +		/* To set admin cert, a password must be enabled */
>> +		if ((setting == tlmi_priv.pwd_admin) &&
>> +		    (!setting->pwd_enabled || !setting->password[0])) {
>>  			kfree(new_cert);
>>  			return -EACCES;
>>  		}
>>  		guid = LENOVO_SET_BIOS_CERT_GUID;
>> -		/* Format: 'Certificate,Admin-password' */
>> -		auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>> -				new_cert, setting->password);
>> +		if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) {
>> +			/* Format: 'SVC | SMC, Certificate, password' */
>> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s",
>> +					     setting == tlmi_priv.pwd_admin ? "SVC" : "SMC",
>> +					     new_cert, setting->password);
>> +		} else {
>> +			/* Format: 'Certificate, password' */
>> +			auth_str = kasprintf(GFP_KERNEL, "%s,%s", new_cert, setting->password);
>> +		}
>
> Have you considered creating a helper for this if/else construct to create 
> the string? (you've basically repeated it multiple times by now with 
> limited change to only parameters AFAICT).
>

I hadn't considered it, but yeah, it probably makes sense.
Not completely sure how I would do it to be honest - if you've got any suggestions let me know, but I will look into it and see if I can improve it.

>>  	}
>>  	kfree(new_cert);
>>  	if (!auth_str)
>> @@ -873,14 +948,19 @@ static umode_t auth_attr_is_visible(struct kobject *kobj,
>>  		return 0;
>>  	}
>>  
>> -	/* We only display certificates on Admin account, if supported */
>> +	/* We only display certificates, if supported */
>>  	if (attr == &auth_certificate.attr ||
>>  	    attr == &auth_signature.attr ||
>>  	    attr == &auth_save_signature.attr ||
>>  	    attr == &auth_cert_thumb.attr ||
>>  	    attr == &auth_cert_to_password.attr) {
>> -		if ((setting == tlmi_priv.pwd_admin) && tlmi_priv.certificate_support)
>> +		if (tlmi_priv.certificate_support) {
>> +			if (setting == tlmi_priv.pwd_admin)
>
> These two if()s combined look the same as the old ode, why are you redoing 
> it?

My indenting here is somehow messed up...not sure how that happened (will fix).
The block below should be indented too...and then I think it makes more sense.
Let me know if you disagree.

>
>> +				return attr->mode;
>> +		if ((tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) &&
>> +		    (setting == tlmi_priv.pwd_system))
>>  			return attr->mode;
>> +		}
>>  		return 0;
>>  	}
>>  
>> @@ -1700,12 +1780,13 @@ 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;
>> -
>> +	if (tlmi_priv.certificate_support) {
>> +		tlmi_priv.pwd_admin->cert_installed =
>> +			tlmi_priv.pwdcfg.core.password_state & TLMI_CERT_SVC;
>> +		tlmi_priv.pwd_system->cert_installed =
>> +			tlmi_priv.pwdcfg.core.password_state & TLMI_CERT_SMC;
>> +	}
>>  	return 0;
>> -
>
> Stray change.

Oops. Will fix.

>
> -- 
>  i.
>
>>  fail_clear_attr:
>>  	for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) {
>>  		if (tlmi_priv.setting[i]) {
>> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
>> index 4728f40143a3..f267d8b46957 100644
>> --- a/drivers/platform/x86/think-lmi.h
>> +++ b/drivers/platform/x86/think-lmi.h
>> @@ -41,6 +41,10 @@ enum save_mode {
>>  };
>>  
>>  /* password configuration details */
>> +#define TLMI_PWDCFG_MODE_LEGACY    0
>> +#define TLMI_PWDCFG_MODE_PASSWORD  1
>> +#define TLMI_PWDCFG_MODE_MULTICERT 3
>> +
>>  struct tlmi_pwdcfg_core {
>>  	uint32_t password_mode;
>>  	uint32_t password_state;
>>

Mark