[PATCH 1/4] platform/x86: think-lmi: Enable opcode support on BIOS settings

[PATCH 1/4] 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>
---
 drivers/platform/x86/think-lmi.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 1138f770149d..d9341305eba9 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -1001,7 +1001,28 @@ 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 */
+		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 2/4] 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.

Correct these mistakes.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 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 d9341305eba9..b8431d3b137f 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -171,7 +171,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_SYS_PWD (1 << 6)
 #define TLMI_CERT    (1 << 7)
 
 #define to_tlmi_pwd_setting(kobj)  container_of(kobj, struct tlmi_pwd_setting, kobj)
@@ -1504,7 +1504,7 @@ 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;
 
-- 
2.40.1

[PATCH 3/4] 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.

Also cleaned up so the index is set to a default of 1 rather than 0
as this just makes more sense (there is no device 0).

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

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index b8431d3b137f..2d95b1a199fa 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -456,9 +456,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);
 		}
@@ -1519,6 +1519,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 4/4] 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>
---
 drivers/platform/x86/think-lmi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 2d95b1a199fa..46500707f939 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -879,6 +879,12 @@ 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 1/4] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Hans de Goede]

Hi Mark,

On 5/17/23 20:19, Mark Pearson wrote:
> 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>
> ---
>  drivers/platform/x86/think-lmi.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 1138f770149d..d9341305eba9 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1001,7 +1001,28 @@ 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 */
> +		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("");

I'm a bit confused about how this works. You are calling the same
LENOVO_SET_BIOS_SETTINGS_GUID as the old non opcode based authentication method
without any auth string.

And then afterwards you are calling LENOVO_OPCODE_IF_GUID with
"WmiOpcodePasswordAdmin:<passwd>"

Won't the initial LENOVO_SET_BIOS_SETTINGS_GUID get rejected since
it does not include an auth-string and you have not authenticated
yet using the opcode mechanism either. IOW shouldn't the opcode
auth call go first ?

And how does this work timing wise, vs races with userspace doing
multiple sysfs writes at once.

If the authentication done afterwards really acks the last
LENOVO_SET_BIOS_SETTINGS_GUID call then a userspace based
attacker could try to race and overwrite the last
LENOVO_SET_BIOS_SETTINGS_GUID call before the ack happens... ?

If this code really is correct I think we need to introduce
a mutex to avoid this race.

And this also needs some comments to explain what is going on.

Regards,

Hans





> +	} 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,

Re: [PATCH 2/4] platform/x86: think-lmi: Correct System password interface

[Hans de Goede]

Hi Mark,

On 5/17/23 20:19, Mark Pearson wrote:
> 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.
> 
> Correct these mistakes.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  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 d9341305eba9..b8431d3b137f 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -171,7 +171,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_SYS_PWD (1 << 6)
>  #define TLMI_CERT    (1 << 7)
>  
>  #define to_tlmi_pwd_setting(kobj)  container_of(kobj, struct tlmi_pwd_setting, kobj)
> @@ -1504,7 +1504,7 @@ 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");

"smp" ?

Also this is not explained in the commit message.

Regards,

Hans


>  		if (!tlmi_priv.pwd_system)
>  			goto fail_clear_attr;
>  

Re: [PATCH 1/4] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Mark Pearson]

Thanks Hans,

On Tue, May 23, 2023, at 6:46 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 5/17/23 20:19, Mark Pearson wrote:
>> 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>
>> ---
>>  drivers/platform/x86/think-lmi.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index 1138f770149d..d9341305eba9 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -1001,7 +1001,28 @@ 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 */
>> +		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("");
>
> I'm a bit confused about how this works. You are calling the same
> LENOVO_SET_BIOS_SETTINGS_GUID as the old non opcode based authentication method
> without any auth string.
>
> And then afterwards you are calling LENOVO_OPCODE_IF_GUID with
> "WmiOpcodePasswordAdmin:<passwd>"
>
> Won't the initial LENOVO_SET_BIOS_SETTINGS_GUID get rejected since
> it does not include an auth-string and you have not authenticated
> yet using the opcode mechanism either. IOW shouldn't the opcode
> auth call go first ?
>
> And how does this work timing wise, vs races with userspace doing
> multiple sysfs writes at once.
>
> If the authentication done afterwards really acks the last
> LENOVO_SET_BIOS_SETTINGS_GUID call then a userspace based
> attacker could try to race and overwrite the last
> LENOVO_SET_BIOS_SETTINGS_GUID call before the ack happens... ?
>
> If this code really is correct I think we need to introduce
> a mutex to avoid this race.
>
> And this also needs some comments to explain what is going on.

Agreed - and looking at it now....I'm questioning it myself. This was tested so it works...but I wonder if that was more luck than judgement.
Let me do some checking - I think I may have messed up here.

Mark

Re: [PATCH 2/4] platform/x86: think-lmi: Correct System password interface

[Mark Pearson]

Thanks Hans

On Tue, May 23, 2023, at 6:46 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 5/17/23 20:19, Mark Pearson wrote:
>> 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.
>> 
>> Correct these mistakes.
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>>  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 d9341305eba9..b8431d3b137f 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -171,7 +171,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_SYS_PWD (1 << 6)
>>  #define TLMI_CERT    (1 << 7)
>>  
>>  #define to_tlmi_pwd_setting(kobj)  container_of(kobj, struct tlmi_pwd_setting, kobj)
>> @@ -1504,7 +1504,7 @@ 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");
>
> "smp" ?

Yes - that is what is common across all the platforms.
https://docs.lenovocdrt.com/#/bios/wmi/wmi_guide?id=change-a-bios-password

I never quite got a straight answer on the origins of the 'sys' account - I think it may have been originally intended but never used? Or maybe it's always been wrong.

I should change the define to be TLMI_SMP_PWD instead actually for clarity. I'll fix that.

>
> Also this is not explained in the commit message.
>

Ack - will update the commit message to include this.

Thanks
Mark

Re: [PATCH 2/4] platform/x86: think-lmi: Correct System password interface

[Hans de Goede]

Hi,

On 5/23/23 14:43, Mark Pearson wrote:
> Thanks Hans
> 
> On Tue, May 23, 2023, at 6:46 AM, Hans de Goede wrote:
>> Hi Mark,
>>
>> On 5/17/23 20:19, Mark Pearson wrote:
>>> 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.
>>>
>>> Correct these mistakes.
>>>
>>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>> ---
>>>  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 d9341305eba9..b8431d3b137f 100644
>>> --- a/drivers/platform/x86/think-lmi.c
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -171,7 +171,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_SYS_PWD (1 << 6)
>>>  #define TLMI_CERT    (1 << 7)
>>>  
>>>  #define to_tlmi_pwd_setting(kobj)  container_of(kobj, struct tlmi_pwd_setting, kobj)
>>> @@ -1504,7 +1504,7 @@ 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");
>>
>> "smp" ?
> 
> Yes - that is what is common across all the platforms.
> https://docs.lenovocdrt.com/#/bios/wmi/wmi_guide?id=change-a-bios-password

Ok, so smp stands for System Management Password I guess ? Might be good to spell
that out somehwere, e.g. in a comment.

Regards,

Hans



> 
> I never quite got a straight answer on the origins of the 'sys' account - I think it may have been originally intended but never used? Or maybe it's always been wrong.
> 
> I should change the define to be TLMI_SMP_PWD instead actually for clarity. I'll fix that.
> 
>>
>> Also this is not explained in the commit message.
>>
> 
> Ack - will update the commit message to include this.
> 
> Thanks
> Mark
> 

Re: [PATCH 2/4] platform/x86: think-lmi: Correct System password interface

[Mark Pearson]

On Tue, May 23, 2023, at 9:27 AM, Hans de Goede wrote:
> Hi,
>
> On 5/23/23 14:43, Mark Pearson wrote:
>> Thanks Hans
>> 
>> On Tue, May 23, 2023, at 6:46 AM, Hans de Goede wrote:
>>> Hi Mark,
>>>
>>> On 5/17/23 20:19, Mark Pearson wrote:
>>>> 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.
>>>>
>>>> Correct these mistakes.
>>>>
>>>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>> ---
>>>>  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 d9341305eba9..b8431d3b137f 100644
>>>> --- a/drivers/platform/x86/think-lmi.c
>>>> +++ b/drivers/platform/x86/think-lmi.c
>>>> @@ -171,7 +171,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_SYS_PWD (1 << 6)
>>>>  #define TLMI_CERT    (1 << 7)
>>>>  
>>>>  #define to_tlmi_pwd_setting(kobj)  container_of(kobj, struct tlmi_pwd_setting, kobj)
>>>> @@ -1504,7 +1504,7 @@ 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");
>>>
>>> "smp" ?
>> 
>> Yes - that is what is common across all the platforms.
>> https://docs.lenovocdrt.com/#/bios/wmi/wmi_guide?id=change-a-bios-password
>
> Ok, so smp stands for System Management Password I guess ? Might be 
> good to spell
> that out somehwere, e.g. in a comment.
>

Yes - and will do.

Mark

Re: [PATCH 1/4] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Mark Pearson]

Hi Hans,

On Tue, May 23, 2023, at 8:36 AM, Mark Pearson wrote:
> Thanks Hans,
>
> On Tue, May 23, 2023, at 6:46 AM, Hans de Goede wrote:
>> Hi Mark,
>>
>> On 5/17/23 20:19, Mark Pearson wrote:
>>> 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>
>>> ---
>>>  drivers/platform/x86/think-lmi.c | 23 ++++++++++++++++++++++-
>>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> index 1138f770149d..d9341305eba9 100644
>>> --- a/drivers/platform/x86/think-lmi.c
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -1001,7 +1001,28 @@ 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 */
>>> +		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("");
>>
>> I'm a bit confused about how this works. You are calling the same
>> LENOVO_SET_BIOS_SETTINGS_GUID as the old non opcode based authentication method
>> without any auth string.
>>
>> And then afterwards you are calling LENOVO_OPCODE_IF_GUID with
>> "WmiOpcodePasswordAdmin:<passwd>"
>>
>> Won't the initial LENOVO_SET_BIOS_SETTINGS_GUID get rejected since
>> it does not include an auth-string and you have not authenticated
>> yet using the opcode mechanism either. IOW shouldn't the opcode
>> auth call go first ?
>>
>> And how does this work timing wise, vs races with userspace doing
>> multiple sysfs writes at once.
>>
>> If the authentication done afterwards really acks the last
>> LENOVO_SET_BIOS_SETTINGS_GUID call then a userspace based
>> attacker could try to race and overwrite the last
>> LENOVO_SET_BIOS_SETTINGS_GUID call before the ack happens... ?
>>
>> If this code really is correct I think we need to introduce
>> a mutex to avoid this race.
>>
>> And this also needs some comments to explain what is going on.
>
> Agreed - and looking at it now....I'm questioning it myself. This was 
> tested so it works...but I wonder if that was more luck than judgement.
> Let me do some checking - I think I may have messed up here.
>

Looked at this and the code is correct - even if it is a bit weird :)
https://docs.lenovocdrt.com/#/bios/wmi/wmi_guide?id=set-and-save-a-bios-setting-on-newer-models

The save_bios_settings would fail if a password was not set (if it's required).

With regards to race conditions - that does seem somewhat unlikely in real life but I can add a mutex around this to catch that condition. I think I should probably do the same in a couple of other places (e.g. certificate_store and new_password_store) where multiple WMI calls are needed to complete an operation. 

Is it OK if I do that as a separate commit on the end of the series or would you rather it was included in this commit? As the scope is, I think, more than just this function I'm leaning towards a separate commit but let me know what best practice is.

Thanks
Mark

Re: [PATCH 1/4] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Hans de Goede]

Hi Mark,

On 5/24/23 20:20, Mark Pearson wrote:
> Hi Hans,
> 
> On Tue, May 23, 2023, at 8:36 AM, Mark Pearson wrote:
>> Thanks Hans,
>>
>> On Tue, May 23, 2023, at 6:46 AM, Hans de Goede wrote:
>>> Hi Mark,
>>>
>>> On 5/17/23 20:19, Mark Pearson wrote:
>>>> 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>
>>>> ---
>>>>  drivers/platform/x86/think-lmi.c | 23 ++++++++++++++++++++++-
>>>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>>> index 1138f770149d..d9341305eba9 100644
>>>> --- a/drivers/platform/x86/think-lmi.c
>>>> +++ b/drivers/platform/x86/think-lmi.c
>>>> @@ -1001,7 +1001,28 @@ 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 */
>>>> +		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("");
>>>
>>> I'm a bit confused about how this works. You are calling the same
>>> LENOVO_SET_BIOS_SETTINGS_GUID as the old non opcode based authentication method
>>> without any auth string.
>>>
>>> And then afterwards you are calling LENOVO_OPCODE_IF_GUID with
>>> "WmiOpcodePasswordAdmin:<passwd>"
>>>
>>> Won't the initial LENOVO_SET_BIOS_SETTINGS_GUID get rejected since
>>> it does not include an auth-string and you have not authenticated
>>> yet using the opcode mechanism either. IOW shouldn't the opcode
>>> auth call go first ?
>>>
>>> And how does this work timing wise, vs races with userspace doing
>>> multiple sysfs writes at once.
>>>
>>> If the authentication done afterwards really acks the last
>>> LENOVO_SET_BIOS_SETTINGS_GUID call then a userspace based
>>> attacker could try to race and overwrite the last
>>> LENOVO_SET_BIOS_SETTINGS_GUID call before the ack happens... ?
>>>
>>> If this code really is correct I think we need to introduce
>>> a mutex to avoid this race.
>>>
>>> And this also needs some comments to explain what is going on.
>>
>> Agreed - and looking at it now....I'm questioning it myself. This was 
>> tested so it works...but I wonder if that was more luck than judgement.
>> Let me do some checking - I think I may have messed up here.
>>
> 
> Looked at this and the code is correct - even if it is a bit weird :)
> https://docs.lenovocdrt.com/#/bios/wmi/wmi_guide?id=set-and-save-a-bios-setting-on-newer-models
> 
> The save_bios_settings would fail if a password was not set (if it's required).

Ok, can you add some comments to the next revision explaining this ?
(no need to write a novel, just some short comments)

> With regards to race conditions - that does seem somewhat unlikely in real life but I can add a mutex around this to catch that condition. I think I should probably do the same in a couple of other places (e.g. certificate_store and new_password_store) where multiple WMI calls are needed to complete an operation. 

Ack for also adding the mutex in other places where there is more
then 1 WMI call involved.

> Is it OK if I do that as a separate commit on the end of the series or would you rather it was included in this commit? As the scope is, I think, more than just this function I'm leaning towards a separate commit but let me know what best practice is.

Adding this in a separate commit is fine with me.

Regards,

Hans

Re: [PATCH 1/4] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Mark Pearson]

On Thu, May 25, 2023, at 5:52 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 5/24/23 20:20, Mark Pearson wrote:
>> Hi Hans,
>> 
>> On Tue, May 23, 2023, at 8:36 AM, Mark Pearson wrote:
>>> Thanks Hans,
>>>
>>> On Tue, May 23, 2023, at 6:46 AM, Hans de Goede wrote:
>>>> Hi Mark,
>>>>
>>>> On 5/17/23 20:19, Mark Pearson wrote:
>>>>> 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>
>>>>> ---
>>>>>  drivers/platform/x86/think-lmi.c | 23 ++++++++++++++++++++++-
>>>>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>>>> index 1138f770149d..d9341305eba9 100644
>>>>> --- a/drivers/platform/x86/think-lmi.c
>>>>> +++ b/drivers/platform/x86/think-lmi.c
>>>>> @@ -1001,7 +1001,28 @@ 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 */
>>>>> +		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("");
>>>>
>>>> I'm a bit confused about how this works. You are calling the same
>>>> LENOVO_SET_BIOS_SETTINGS_GUID as the old non opcode based authentication method
>>>> without any auth string.
>>>>
>>>> And then afterwards you are calling LENOVO_OPCODE_IF_GUID with
>>>> "WmiOpcodePasswordAdmin:<passwd>"
>>>>
>>>> Won't the initial LENOVO_SET_BIOS_SETTINGS_GUID get rejected since
>>>> it does not include an auth-string and you have not authenticated
>>>> yet using the opcode mechanism either. IOW shouldn't the opcode
>>>> auth call go first ?
>>>>
>>>> And how does this work timing wise, vs races with userspace doing
>>>> multiple sysfs writes at once.
>>>>
>>>> If the authentication done afterwards really acks the last
>>>> LENOVO_SET_BIOS_SETTINGS_GUID call then a userspace based
>>>> attacker could try to race and overwrite the last
>>>> LENOVO_SET_BIOS_SETTINGS_GUID call before the ack happens... ?
>>>>
>>>> If this code really is correct I think we need to introduce
>>>> a mutex to avoid this race.
>>>>
>>>> And this also needs some comments to explain what is going on.
>>>
>>> Agreed - and looking at it now....I'm questioning it myself. This was 
>>> tested so it works...but I wonder if that was more luck than judgement.
>>> Let me do some checking - I think I may have messed up here.
>>>
>> 
>> Looked at this and the code is correct - even if it is a bit weird :)
>> https://docs.lenovocdrt.com/#/bios/wmi/wmi_guide?id=set-and-save-a-bios-setting-on-newer-models
>> 
>> The save_bios_settings would fail if a password was not set (if it's required).
>
> Ok, can you add some comments to the next revision explaining this ?
> (no need to write a novel, just some short comments)

Of course - no problem :)

>
>> With regards to race conditions - that does seem somewhat unlikely in real life but I can add a mutex around this to catch that condition. I think I should probably do the same in a couple of other places (e.g. certificate_store and new_password_store) where multiple WMI calls are needed to complete an operation. 
>
> Ack for also adding the mutex in other places where there is more
> then 1 WMI call involved.
>
>> Is it OK if I do that as a separate commit on the end of the series or would you rather it was included in this commit? As the scope is, I think, more than just this function I'm leaning towards a separate commit but let me know what best practice is.
>
> Adding this in a separate commit is fine with me.

Thanks. I'll work on that and get a v2 series out shortly

Mark