| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <26b3d2dc-5a3e-46dd-8321-a04ba25dbf4f@app.fastmail.com>
Date: Tue, 22 Oct 2024 15:30:02 -0400
From: "Mark Pearson" <mpearson-lenovo@...ebb.ca>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: "Hans de Goede" <hdegoede@...hat.com>,
"platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] platform/x86: think-lmi: Multi-certificate support
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@...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.
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
Powered by blists - more mailing lists