lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3635398e-f810-c42b-9c6e-f9e0bb19e298@linux.intel.com>
Date: Tue, 22 Oct 2024 11:14:39 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Mark Pearson <mpearson-lenovo@...ebb.ca>
cc: Hans de Goede <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] platform/x86: think-lmi: Multi-certificate support

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@...ebb.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;
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ