Message ID | 20230517181945.3725-1-mpearson-lenovo@squebb.ca |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1343397vqo; Wed, 17 May 2023 11:36:52 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7ah/uTA8FcZ+zftjUwiREF8yeCvNTDmzzmMwEmu5mgJu6xBkylHPtASDiFc4gjhnmLxPTV X-Received: by 2002:a05:6a21:9988:b0:100:3964:6cb with SMTP id ve8-20020a056a21998800b00100396406cbmr46767297pzb.40.1684348612453; Wed, 17 May 2023 11:36:52 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1684348612; cv=none; d=google.com; s=arc-20160816; b=zB25IVQnJ1ZZYvE/d0Fpt930H0kiv7o9xBnqYnE4W0OXBNEBJEZcJXkZLa1MByospB v232sb2b0B7VXGShpjTxDqe57t9LH2PDhqsQLS41rRyNKyDS78hn0MWOJBmcPhOJt9Gq sZJPPX9b3AiBc6wqLUnFOEsEs61FaptPEFPoTU4/uY2oJsWiwf4QB35AUgXibFmcfVeM oINUKVjSQ844tAPFNGfH4rz2QgFI8B4cvE4Nq8eBuLIHIr0dxsnUcZqnoJcA+hk0GEpc Vg23SsXAogU7732uekKHNA37sAIa4i+me6+35ErW8O10Lru7C3ffKxI0PYf6S2dhDxZA znvA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :feedback-id:dkim-signature:dkim-signature; bh=yEx5pnV4SMs/Lz7ucq6fHxr/ssA2T3F4PyuZxJWZ4yY=; b=DpbAGMwdBykqIWTYwX8vwPQtTno33vAqfsVhLQ/hJ+Eda730PC9lstw04Rc17MPkEv NTYEu7gFjiHAqUKxyU1HWomG61sv2/6Ob3NdVbUN8Er6TgCTnaAZ+lZpwC4PQyqfp6d3 XfAZLGkv2G1lk8NZx6Zze3OhrzXRkeMccgR/ortccgNd2ubB6TexWiznbdZi6cM1gSWz iI6gOW3hbXYIdmMNdSW0tmzwrv09fBj+W7AWCVWIxt4Ht9MxEDvq3Vz03u+cUiE4WvXo 4xxxm+r+dFiIcvtwx0gYMW6z11rvU5jgzX8C7Te11ZJ5MLeAYMuAceWKTp1JslK7qI8O Vm/Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@squebb.ca header.s=fm2 header.b=Rjp1dLxR; dkim=pass header.i=@messagingengine.com header.s=fm1 header.b=GmnytK7z; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id bv187-20020a632ec4000000b0050726756edesi20662905pgb.76.2023.05.17.11.36.35; Wed, 17 May 2023 11:36:52 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@squebb.ca header.s=fm2 header.b=Rjp1dLxR; dkim=pass header.i=@messagingengine.com header.s=fm1 header.b=GmnytK7z; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229678AbjEQSUC (ORCPT <rfc822;abdi.embedded@gmail.com> + 99 others); Wed, 17 May 2023 14:20:02 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49710 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229445AbjEQSUB (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 17 May 2023 14:20:01 -0400 Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 48D031708; Wed, 17 May 2023 11:20:00 -0700 (PDT) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id B3A7F5C00FD; Wed, 17 May 2023 14:19:59 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 17 May 2023 14:19:59 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=squebb.ca; h=cc :cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm2; t=1684347599; x= 1684433999; bh=yEx5pnV4SMs/Lz7ucq6fHxr/ssA2T3F4PyuZxJWZ4yY=; b=R jp1dLxRduXWT3nYcjKZOrxkxUsPrajWOV1BZEPxXO8MIBXBQIX03cltsF0H3h7IF C9XUvFwZMH3VPcOwtHUacQRKqfgHM1yk81atZ/E3iTGJ8lI1zykuxBQu5bAHlTo0 VcbpjkVFjIVvtxQDDj4LFtYcDPpNjEy6bIDoR42E9iBeiAtTjgjXWZSlLlvNRaHf WUXNjQHYLnC6DAeskjQiV1JZvVbEI0GdXEudg8qvmCgDl/Fp4rjzhKEoRci0QB/L l9MdIjevN+xZ6OyaB2gDg8u3UwET9/gJPUvcU1v2/WCo8Fm0hCR3faKYB72VCaBL Ef0F2ACDVN1I9QvxEBT9g== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1684347599; x= 1684433999; bh=yEx5pnV4SMs/Lz7ucq6fHxr/ssA2T3F4PyuZxJWZ4yY=; b=G mnytK7zwKLwGwRdlfhC7PYDkMvoW8rxidNiNVbtTl2Je4xXGViexK1KDO+ZFan+Z CS2yKFQxYCGSJVM9JiLahycsSWFPYfh69naTGXcC+G54WF5qQ0bBTTKlI8OTpOMe GC7ChaK/CNVUJkWzLOMkynKUDb+kFa10rJV9l550bNNfBnKfAnBwe2qK4QVrpHQW rtOoMkxCNaWv+XCAgGlEqyQ4bOfzKfSQPAvlLTMMnxjXTJTlEVkXDiACUYShepjP tP2dTsn/aZlP093SgKX2wBV25M6YaeekyHPn4xvLkEbd3f4KKZMHC1eKEDnuGbI+ ksk4mAsHjwhlrudKDMH7g== X-ME-Sender: <xms:zxplZDjcu-1Fe4YF8AoI6VzCm3h1pL9dkyVAU5_N4KkRZAFn2OdNgA> <xme:zxplZAAwbdwzBSTjHrsny4qpYeLzElyX6dFwZjj66IJxWdB0tos5My8s0heXJOLY- V8CiMz_XB8I83OlXxc> X-ME-Received: <xmr:zxplZDHI_SqAzCvzbFiEATO4BVs2ayc7H34eFSQ8O_Qyod2yGCgMaOoPfl6C7NO2glH_pHoe1dutudbV6ZP-c4_cqcXLTgGlPOKNKPVwE9ciZEuWL350OAtJNA> X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrfeeiuddguddvgecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecuogetfedtuddqtdduucdludehmdenucfjughrpe fhvfevufffkffojghfggfgsedtkeertdertddtnecuhfhrohhmpeforghrkhcurfgvrghr shhonhcuoehmphgvrghrshhonhdqlhgvnhhovhhosehsqhhuvggssgdrtggrqeenucggtf frrghtthgvrhhnpeeftddvjeefleffvefhgfejjeehudetteeigeeugfekhffhgeejudeu teehgfdvffenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhroh hmpehmphgvrghrshhonhdqlhgvnhhovhhosehsqhhuvggssgdrtggr X-ME-Proxy: <xmx:zxplZASv-Z9A2Ze747b4oMrFZyzAn7tevOyVWDBszUwdjYAX0nYCuw> <xmx:zxplZAzhgHzw-kBc1PZjgqtWheoWDC2OcOeBB8CCSHMMqybskXDjig> <xmx:zxplZG42By1zzlWKzooU_TrOfOBMbAOxXbd3dj8DWxZp60v9ouW1Dg> <xmx:zxplZM-7Sl5JKlrgAfCFor-A3_P_39F838u5JQw1Qc5J8KONo9qO3A> Feedback-ID: ibe194615:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 17 May 2023 14:19:58 -0400 (EDT) From: Mark Pearson <mpearson-lenovo@squebb.ca> To: mpearson-lenovo@squebb.ca Cc: hdegoede@redhat.com, markgross@kernel.org, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH 1/4] platform/x86: think-lmi: Enable opcode support on BIOS settings Date: Wed, 17 May 2023 14:19:42 -0400 Message-Id: <20230517181945.3725-1-mpearson-lenovo@squebb.ca> X-Mailer: git-send-email 2.40.1 In-Reply-To: <mpearson-lenovo@squebb.ca> References: <mpearson-lenovo@squebb.ca> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1766167530633866281?= X-GMAIL-MSGID: =?utf-8?q?1766167530633866281?= |
Series |
[1/4] platform/x86: think-lmi: Enable opcode support on BIOS settings
|
|
Commit Message
Mark Pearson
May 17, 2023, 6:19 p.m. UTC
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(-)
Comments
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,