Hi Vegard,
On Wed, Oct 2, 2024, at 11:12 AM, Vegard Nossum wrote:
> From: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>
> [ Upstream commit 6f7d0f5fd8e440c3446560100ac4ff9a55eec340 ]
>
> 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 <mpearson-lenovo@xxxxxxxxx>
> Fixes: 640a5fa50a42 ("platform/x86: think-lmi: Opcode support")
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Link:
> https://lore.kernel.org/r/20240209152359.528919-1-mpearson-lenovo@xxxxxxxxx
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> (cherry picked from commit 6f7d0f5fd8e440c3446560100ac4ff9a55eec340)
> [Harshit: CVE-2024-26836; Resolve conflicts due to missing commit:
> 318d97849fc2 ("platform/x86: think-lmi: Add bulk save feature") which
> is
> not in 6.6.y]
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@xxxxxxxxxx>
> Signed-off-by: Vegard Nossum <vegard.nossum@xxxxxxxxxx>
> ---
> drivers/platform/x86/think-lmi.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index aee869769843f..2396decdb3cb3 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1021,7 +1021,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) {
> @@ -1033,13 +1042,6 @@ static ssize_t current_value_store(struct kobject *kobj,
> 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]) {
> --
> 2.34.1
Changes look good. Thank you.
However, not sure why this got caught up in a thread about CVE's? It's fixing functionality that wasn't working on some Lenovo Thinkstation platforms, but isn't a security issue in my opinion. Before the fix you just couldn't use WMI to set BIOS attributes on a few platforms - updates would be rejected.
I can see it might be nice to have in older kernels for some users though. If you need anything else let me know.
Reviewed-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
Mark
