| Message ID | 20230517181945.3725-1-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/4] platform/x86: think-lmi: Enable opcode support on BIOS settings | expand |
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,
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
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
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
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
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,
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(-)