[PATCH v4 1/8] platform/x86: think-lmi: mutex protection around multiple WMI calls

[PATCH v4 1/8] platform/x86: think-lmi: mutex protection around multiple WMI calls

[Mark Pearson]

When an attribute is being changed if the Admin account is enabled, or if a password
is being updated then multiple WMI calls are needed.
Add mutex protection to ensure no race conditions are introduced.

Fixes: b49f72e7f96d ("platform/x86: think-lmi: Certificate authentication support")
Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v2:
 - New commit added after review of other patches in series.
Changes in v3:
 - Simplified mutex handling as recommended.
Changes in v4:
 - This was the 5th patch in the series but moved to be first.
 - Added Fixes tag.
 - Improved commit information to be clearer.

 drivers/platform/x86/think-lmi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 1138f770149d..6cf77bc26b05 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -14,6 +14,7 @@
 #include <linux/acpi.h>
 #include <linux/errno.h>
 #include <linux/fs.h>
+#include <linux/mutex.h>
 #include <linux/string.h>
 #include <linux/types.h>
 #include <linux/dmi.h>
@@ -195,6 +196,7 @@ static const char * const level_options[] = {
 };
 static struct think_lmi tlmi_priv;
 static struct class *fw_attr_class;
+static DEFINE_MUTEX(tlmi_mutex);
 
 /* ------ Utility functions ------------*/
 /* Strip out CR if one is present */
@@ -437,6 +439,9 @@ static ssize_t new_password_store(struct kobject *kobj,
 	/* Strip out CR if one is present, setting password won't work if it is present */
 	strip_cr(new_pwd);
 
+	/* Use lock in case multiple WMI operations needed */
+	mutex_lock(&tlmi_mutex);
+
 	pwdlen = strlen(new_pwd);
 	/* pwdlen == 0 is allowed to clear the password */
 	if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) {
@@ -493,6 +498,7 @@ static ssize_t new_password_store(struct kobject *kobj,
 		kfree(auth_str);
 	}
 out:
+	mutex_unlock(&tlmi_mutex);
 	kfree(new_pwd);
 	return ret ?: count;
 }
@@ -981,6 +987,9 @@ static ssize_t current_value_store(struct kobject *kobj,
 	/* Strip out CR if one is present */
 	strip_cr(new_setting);
 
+	/* Use lock in case multiple WMI operations needed */
+	mutex_lock(&tlmi_mutex);
+
 	/* Check if certificate authentication is enabled and active */
 	if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
 		if (!tlmi_priv.pwd_admin->signature || !tlmi_priv.pwd_admin->save_signature) {
@@ -1039,6 +1048,7 @@ static ssize_t current_value_store(struct kobject *kobj,
 		kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
 	}
 out:
+	mutex_unlock(&tlmi_mutex);
 	kfree(auth_str);
 	kfree(set_str);
 	kfree(new_setting);
-- 
2.40.1

[PATCH v4 2/8] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Mark Pearson]

Whilst reviewing some documentation from the FW team on using WMI on
Lenovo system I noticed that we weren't using Opcode support when
changing BIOS settings in the thinkLMI driver.

We should be doing this to ensure we're future proof as the old
non-opcode mechanism has been deprecated.

Tested on X1 Carbon G10 and G11.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v2:
 - Update comment for clearer explanation of what the driver is doing.
Changes in v3:
 - None. Version bump with rest of series.
Changes in v4:
 - Fixed code alignment as requested.
 - changed 'non opcode' to 'non-opcode'.
 - Missing space added at end of comment.
 - This patch was previously #1 and is now #2 in series.

 drivers/platform/x86/think-lmi.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 6cf77bc26b05..80a5c989db03 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -1010,7 +1010,33 @@ static ssize_t current_value_store(struct kobject *kobj,
 				tlmi_priv.pwd_admin->save_signature);
 		if (ret)
 			goto out;
-	} else { /* Non certiifcate based authentication */
+	} else if (tlmi_priv.opcode_support) {
+		/*
+		 * If opcode support is present use that interface.
+		 * Note - this sets the variable and then the password as separate
+		 * WMI calls. Function tlmi_save_bios_settings will error if the
+		 * password is incorrect.
+		 */
+		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_SETTINGS_GUID, set_str);
+		if (ret)
+			goto out;
+
+		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
+						  tlmi_priv.pwd_admin->password);
+			if (ret)
+				goto out;
+		}
+
+		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]) {
 			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
 					tlmi_priv.pwd_admin->password,
-- 
2.40.1

[PATCH v4 3/8] platform/x86: think-lmi: Correct System password interface

[Mark Pearson]

The system password identification was incorrect. This means that if
the password was enabled it wouldn't be detected correctly; and setting
it would not work.
Also updated code to use TLMI_SMP_PWD instead of TLMI_SYS_PWD to be in
sync with Lenovo documentation.

Fixes: 640a5fa50a42 ("platform/x86: think-lmi: Opcode support")
Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v2:
 - Updated define name to be SMP_PWD instead of SYS_PWD.
 - Clarified in comments what each password type is.
Changes in v3:
 - None. Version bump with rest of series.
Changes in v4:
 - This patch was previously #2 and is now #3 in series.
 - Patch split so comment updates moved into new patch (next in series).

 drivers/platform/x86/think-lmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 80a5c989db03..f6d1931540f1 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -172,7 +172,7 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
 #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_SMP_PWD (1 << 6) /* System Management */
 #define TLMI_CERT    (1 << 7)
 
 #define to_tlmi_pwd_setting(kobj)  container_of(kobj, struct tlmi_pwd_setting, kobj)
@@ -1519,11 +1519,11 @@ static int tlmi_analyze(void)
 		tlmi_priv.pwd_power->valid = true;
 
 	if (tlmi_priv.opcode_support) {
-		tlmi_priv.pwd_system = tlmi_create_auth("sys", "system");
+		tlmi_priv.pwd_system = tlmi_create_auth("smp", "system");
 		if (!tlmi_priv.pwd_system)
 			goto fail_clear_attr;
 
-		if (tlmi_priv.pwdcfg.core.password_state & TLMI_SYS_PWD)
+		if (tlmi_priv.pwdcfg.core.password_state & TLMI_SMP_PWD)
 			tlmi_priv.pwd_system->valid = true;
 
 		tlmi_priv.pwd_hdd = tlmi_create_auth("hdd", "hdd");
-- 
2.40.1

[PATCH v4 4/8] platform/x86: think-lmi: Update password attribute comments

[Mark Pearson]

Add comments to clarify what the different password attributes
are (as requested).

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v4:
 - New patch split out from previous patch #2.

 drivers/platform/x86/think-lmi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index f6d1931540f1..564e3fc33cfb 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -169,11 +169,11 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
  */
 #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_POP_PWD (1 << 0) /* Supervisor */
+#define TLMI_PAP_PWD (1 << 1) /* Power-on */
+#define TLMI_HDD_PWD (1 << 2) /* HDD/NVME */
 #define TLMI_SMP_PWD (1 << 6) /* System Management */
-#define TLMI_CERT    (1 << 7)
+#define TLMI_CERT    (1 << 7) /* Certificate Based */
 
 #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)
-- 
2.40.1

[PATCH v4 5/8] platform/x86: think-lmi: Update password fields to use BIT

[Mark Pearson]

Code clean up to use BIT macro as suggested.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v4:
 - New patch split out from previous patch #2.

 drivers/platform/x86/think-lmi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 564e3fc33cfb..e3be99b44ce0 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -169,11 +169,11 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
  */
 #define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4"
 
-#define TLMI_POP_PWD (1 << 0) /* Supervisor */
-#define TLMI_PAP_PWD (1 << 1) /* Power-on */
-#define TLMI_HDD_PWD (1 << 2) /* HDD/NVME */
-#define TLMI_SMP_PWD (1 << 6) /* System Management */
-#define TLMI_CERT    (1 << 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    BIT(7) /* Certificate Based */
 
 #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)
-- 
2.40.1

[PATCH v4 6/8] platform/x86: think-lmi: Correct NVME password handling

[Mark Pearson]

NVME passwords identifier have been standardised across the Lenovo
systems and now use udrp and adrp (user and admin level) instead of
unvp and mnvp.

This should apparently be backwards compatible.

Fixes: 640a5fa50a42 ("platform/x86: think-lmi: Opcode support")
Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v2 & v3: 
 - None. Version bumped in series.
Changes in v4:
 - This patch was previously #2 and is now #6 in series.
 - index default change split into new patch (next in series).

 drivers/platform/x86/think-lmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index e3be99b44ce0..71bbe169c77e 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -461,9 +461,9 @@ static ssize_t new_password_store(struct kobject *kobj,
 				sprintf(pwd_type, "mhdp%d", setting->index);
 		} else if (setting == tlmi_priv.pwd_nvme) {
 			if (setting->level == TLMI_LEVEL_USER)
-				sprintf(pwd_type, "unvp%d", setting->index);
+				sprintf(pwd_type, "udrp%d", setting->index);
 			else
-				sprintf(pwd_type, "mnvp%d", setting->index);
+				sprintf(pwd_type, "adrp%d", setting->index);
 		} else {
 			sprintf(pwd_type, "%s", setting->pwd_type);
 		}
-- 
2.40.1

[PATCH v4 7/8] platform/x86: think-lmi: Correct NVME index default

[Mark Pearson]

The NVME/HDD index used by WMI starts at 1 so corrected the default
appropriately.
Note, zero index is still permitted in case it is required on future
platforms.
Documentation updated correspondingly

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v4:
 - New patch. Split out changes into separate commit as requested.
 - Update documentation.
 - Details on zero index added to commit message.

 Documentation/ABI/testing/sysfs-class-firmware-attributes | 4 ++--
 drivers/platform/x86/think-lmi.c                          | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 4cdba3477176..1b3ecae80b3d 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -243,8 +243,8 @@ Description:
 
 		index:
 					Used with HDD and NVME authentication to set the drive index
-					that is being referenced (e.g hdd0, hdd1 etc)
-					This attribute defaults to device 0.
+					that is being referenced (e.g hdd1, hdd2 etc)
+					This attribute defaults to device 1.
 
 		certificate, signature, save_signature:
 					These attributes are used for certificate based authentication. This is
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 71bbe169c77e..2aaaee879488 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -1534,6 +1534,10 @@ static int tlmi_analyze(void)
 		if (!tlmi_priv.pwd_nvme)
 			goto fail_clear_attr;
 
+		/* Set default hdd/nvme index to 1 as there is no device 0 */
+		tlmi_priv.pwd_hdd->index = 1;
+		tlmi_priv.pwd_nvme->index = 1;
+
 		if (tlmi_priv.pwdcfg.core.password_state & TLMI_HDD_PWD) {
 			/* Check if PWD is configured and set index to first drive found */
 			if (tlmi_priv.pwdcfg.ext.hdd_user_password ||
-- 
2.40.1

[PATCH v4 8/8] platform/x86: think-lmi: Don't display unnecessary authentication settings

[Mark Pearson]

If Opcode support is available (which is the standard for all platforms
going forward) then there is no need to have the encoding and kbdlang
attributes visible.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v2 & v3:
 - None. Version bumped in series.
Changes in v4:
 - Fixed code alignment as requested.
 - This patch was previously #4 and is now #8 in series.

 drivers/platform/x86/think-lmi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 2aaaee879488..52d1ce8dfe44 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -885,6 +885,11 @@ static umode_t auth_attr_is_visible(struct kobject *kobj,
 		return 0;
 	}
 
+	/* Don't display un-needed settings if opcode available */
+	if ((attr == &auth_encoding.attr || attr == &auth_kbdlang.attr) &&
+	    tlmi_priv.opcode_support)
+		return 0;
+
 	return attr->mode;
 }
 
-- 
2.40.1

Re: [PATCH v4 8/8] platform/x86: think-lmi: Don't display unnecessary authentication settings

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

On Thu, 1 Jun 2023, Mark Pearson wrote:

> If Opcode support is available (which is the standard for all platforms
> going forward) then there is no need to have the encoding and kbdlang
> attributes visible.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Thanks a lot, the patches look good now. One small thing for future: next 
time, try to arrange a series such that the patches with Fixes tags are 
the first patches, in here I think it's not a big deal since 2/8 doesn't 
seem to conflict with 3/8.

For all patches 1-8:

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v4 8/8] platform/x86: think-lmi: Don't display unnecessary authentication settings

[Mark Pearson]

On Fri, Jun 2, 2023, at 7:12 AM, Ilpo Järvinen wrote:
> On Thu, 1 Jun 2023, Mark Pearson wrote:
>
>> If Opcode support is available (which is the standard for all platforms
>> going forward) then there is no need to have the encoding and kbdlang
>> attributes visible.
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>
> Thanks a lot, the patches look good now. One small thing for future: next 
> time, try to arrange a series such that the patches with Fixes tags are 
> the first patches, in here I think it's not a big deal since 2/8 doesn't 
> seem to conflict with 3/8.
>
Ah - OK, thanks for the note, I didn't know that.

> For all patches 1-8:
>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>

Many thanks
Mark

Re: [PATCH v4 1/8] platform/x86: think-lmi: mutex protection around multiple WMI calls

[Hans de Goede]

Hi,

On 6/1/23 22:05, Mark Pearson wrote:
> When an attribute is being changed if the Admin account is enabled, or if a password
> is being updated then multiple WMI calls are needed.
> Add mutex protection to ensure no race conditions are introduced.
> 
> Fixes: b49f72e7f96d ("platform/x86: think-lmi: Certificate authentication support")
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans





> ---
> Changes in v2:
>  - New commit added after review of other patches in series.
> Changes in v3:
>  - Simplified mutex handling as recommended.
> Changes in v4:
>  - This was the 5th patch in the series but moved to be first.
>  - Added Fixes tag.
>  - Improved commit information to be clearer.
> 
>  drivers/platform/x86/think-lmi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 1138f770149d..6cf77bc26b05 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -14,6 +14,7 @@
>  #include <linux/acpi.h>
>  #include <linux/errno.h>
>  #include <linux/fs.h>
> +#include <linux/mutex.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/dmi.h>
> @@ -195,6 +196,7 @@ static const char * const level_options[] = {
>  };
>  static struct think_lmi tlmi_priv;
>  static struct class *fw_attr_class;
> +static DEFINE_MUTEX(tlmi_mutex);
>  
>  /* ------ Utility functions ------------*/
>  /* Strip out CR if one is present */
> @@ -437,6 +439,9 @@ static ssize_t new_password_store(struct kobject *kobj,
>  	/* Strip out CR if one is present, setting password won't work if it is present */
>  	strip_cr(new_pwd);
>  
> +	/* Use lock in case multiple WMI operations needed */
> +	mutex_lock(&tlmi_mutex);
> +
>  	pwdlen = strlen(new_pwd);
>  	/* pwdlen == 0 is allowed to clear the password */
>  	if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) {
> @@ -493,6 +498,7 @@ static ssize_t new_password_store(struct kobject *kobj,
>  		kfree(auth_str);
>  	}
>  out:
> +	mutex_unlock(&tlmi_mutex);
>  	kfree(new_pwd);
>  	return ret ?: count;
>  }
> @@ -981,6 +987,9 @@ static ssize_t current_value_store(struct kobject *kobj,
>  	/* Strip out CR if one is present */
>  	strip_cr(new_setting);
>  
> +	/* Use lock in case multiple WMI operations needed */
> +	mutex_lock(&tlmi_mutex);
> +
>  	/* Check if certificate authentication is enabled and active */
>  	if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
>  		if (!tlmi_priv.pwd_admin->signature || !tlmi_priv.pwd_admin->save_signature) {
> @@ -1039,6 +1048,7 @@ static ssize_t current_value_store(struct kobject *kobj,
>  		kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
>  	}
>  out:
> +	mutex_unlock(&tlmi_mutex);
>  	kfree(auth_str);
>  	kfree(set_str);
>  	kfree(new_setting);