The Lenovo workstations require the password opcode to be run before
the attribute value is changed (if Admin password is enabled).
Tested on some Thinkpads to confirm they are OK with this order too.
Signed-off-by: Mark Pearson <[email protected]>
---
drivers/platform/x86/think-lmi.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 3a396b763c49..ce3e08815a8e 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -1009,7 +1009,16 @@ static ssize_t current_value_store(struct kobject *kobj,
* 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.
+ * Workstation's require the opcode to be set before changing the
+ * attribute.
*/
+ 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;
+ }
+
set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
new_setting);
if (!set_str) {
@@ -1021,17 +1030,10 @@ static ssize_t current_value_store(struct kobject *kobj,
if (ret)
goto out;
- if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
+ if (tlmi_priv.save_mode == TLMI_SAVE_BULK)
tlmi_priv.save_required = true;
- } else {
- 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;
- }
+ 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]) {
auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
--
2.43.0
Thanks Ilpo
On Fri, Feb 9, 2024, at 10:34 AM, Ilpo Järvinen wrote:
> On Fri, 9 Feb 2024, Mark Pearson wrote:
>
>> The Lenovo workstations require the password opcode to be run before
>> the attribute value is changed (if Admin password is enabled).
>>
>> Tested on some Thinkpads to confirm they are OK with this order too.
>>
>> Signed-off-by: Mark Pearson <[email protected]>
>
> Would a Fixes tag be appropriate here?
Hmmm - good point, though it has been "wrong" since it was originally implemented (where we tested on Thinkpads).
I guess I could use the commit ID from when I originally implemented opcodes?
Fixes: 640a5fa ("platform/x86: think-lmi: Opcode support")
Do you want me to push a new version with this?
Thanks
Mark
On Fri, 9 Feb 2024, Mark Pearson wrote:
> The Lenovo workstations require the password opcode to be run before
> the attribute value is changed (if Admin password is enabled).
>
> Tested on some Thinkpads to confirm they are OK with this order too.
>
> Signed-off-by: Mark Pearson <[email protected]>
Would a Fixes tag be appropriate here?
--
i.
> ---
> drivers/platform/x86/think-lmi.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 3a396b763c49..ce3e08815a8e 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1009,7 +1009,16 @@ static ssize_t current_value_store(struct kobject *kobj,
> * 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.
> + * Workstation's require the opcode to be set before changing the
> + * attribute.
> */
> + 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;
> + }
> +
> set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
> new_setting);
> if (!set_str) {
> @@ -1021,17 +1030,10 @@ static ssize_t current_value_store(struct kobject *kobj,
> if (ret)
> goto out;
>
> - if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
> + if (tlmi_priv.save_mode == TLMI_SAVE_BULK)
> tlmi_priv.save_required = true;
> - } else {
> - 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;
> - }
> + 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]) {
> auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>
On Fri, 9 Feb 2024, Mark Pearson wrote:
> Thanks Ilpo
>
> On Fri, Feb 9, 2024, at 10:34 AM, Ilpo Järvinen wrote:
> > On Fri, 9 Feb 2024, Mark Pearson wrote:
> >
> >> The Lenovo workstations require the password opcode to be run before
> >> the attribute value is changed (if Admin password is enabled).
> >>
> >> Tested on some Thinkpads to confirm they are OK with this order too.
> >>
> >> Signed-off-by: Mark Pearson <[email protected]>
> >
> > Would a Fixes tag be appropriate here?
>
> Hmmm - good point, though it has been "wrong" since it was originally implemented (where we tested on Thinkpads).
>
> I guess I could use the commit ID from when I originally implemented opcodes?
> Fixes: 640a5fa ("platform/x86: think-lmi: Opcode support")
Yes, in that case the original commit is the correct one.
> Do you want me to push a new version with this?
I think this is simple enough to go through fixes branch so it depends on
Hans.
Reviewed-by: Ilpo Järvinen <[email protected]>
--
i.
Hi,
On 2/9/24 16:23, Mark Pearson wrote:
> The Lenovo workstations require the password opcode to be run before
> the attribute value is changed (if Admin password is enabled).
>
> Tested on some Thinkpads to confirm they are OK with this order too.
>
> Signed-off-by: Mark Pearson <[email protected]>
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
I've also added the Fixes tag and Ilpo's Reviewed-by.
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.
Regards,
Hans
> ---
> drivers/platform/x86/think-lmi.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 3a396b763c49..ce3e08815a8e 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1009,7 +1009,16 @@ static ssize_t current_value_store(struct kobject *kobj,
> * 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.
> + * Workstation's require the opcode to be set before changing the
> + * attribute.
> */
> + 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;
> + }
> +
> set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
> new_setting);
> if (!set_str) {
> @@ -1021,17 +1030,10 @@ static ssize_t current_value_store(struct kobject *kobj,
> if (ret)
> goto out;
>
> - if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
> + if (tlmi_priv.save_mode == TLMI_SAVE_BULK)
> tlmi_priv.save_required = true;
> - } else {
> - 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;
> - }
> + 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]) {
> auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",