From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.4442.1578061272477985690 for ; Fri, 03 Jan 2020 06:21:12 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Jhz25O9T; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578061271; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=I0QFKtf65zyQlp26aCVz7v7lZt8+mSJw4zHXzsEHaQk=; b=Jhz25O9TWElaA1B4wIdIYdly6cvLfxQ/neAYNZNGrruqeZTIjL07jS99wMOani8TtTydfj jE18qn5XguBWgI8SanOfWd5NHuPlC1RduUIfCGhd7ILd0eC6TfwPUAPMKhIJW7Wok8Pcza zWhLaG9TiYjI9FW4nvR8h2ZgcXL8FOo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-385-PuiT_xx7Niuwgkuvg5AtEw-1; Fri, 03 Jan 2020 09:21:10 -0500 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 56EB1107ACC5; Fri, 3 Jan 2020 14:21:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-245.ams2.redhat.com [10.36.116.245]) by smtp.corp.redhat.com (Postfix) with ESMTP id 81DDE60BF7; Fri, 3 Jan 2020 14:21:03 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time To: devel@edk2.groups.io, zhichao.gao@intel.com Cc: Jiewen Yao , Jian J Wang , Chao Zhang , Jordan Justen , Ard Biesheuvel , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , Stefan Berger References: <20200103030428.28176-1-zhichao.gao@intel.com> <20200103030428.28176-7-zhichao.gao@intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 3 Jan 2020 15:21:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200103030428.28176-7-zhichao.gao@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-MC-Unique: PuiT_xx7Niuwgkuvg5AtEw-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hello Zhichao, On 01/03/20 04:04, Gao, Zhichao wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2443 >=20 > Use the pcd PcdPhysicalPresenceUserConfirmTimeout to control the > wait time of user response. >=20 > Cc: Jiewen Yao > Cc: Jian J Wang > Cc: Chao Zhang > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Marc-Andr=C3=A9 Lureau > Cc: Stefan Berger > Signed-off-by: Zhichao Gao > --- > .../DxeTcg2PhysicalPresenceLib.c | 63 ++++++++++++++----- > .../DxeTcg2PhysicalPresenceLib.inf | 6 +- > 2 files changed, 52 insertions(+), 17 deletions(-) >=20 > diff --git a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalP= resenceLib.c b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalP= resenceLib.c > index 00d76ba2c2..13f78cbfac 100644 > --- a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresence= Lib.c > +++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresence= Lib.c > @@ -9,7 +9,7 @@ > =20 > Copyright (C) 2018, Red Hat, Inc. > Copyright (c) 2018, IBM Corporation. All rights reserved.
> -Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > =20 > **/ > @@ -32,6 +32,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include > #include > #include > +#include > +#include > =20 > #include > =20 > @@ -365,28 +367,57 @@ Tcg2ReadUserKey ( > { > EFI_STATUS Status; > EFI_INPUT_KEY Key; > - UINT16 InputKey; > + UINT16 ConfirmKey; > + UINTN Interval; > + INT64 Timeout; > =20 > - InputKey =3D 0; > + // > + // delay 100 milli-second > + // > + Interval =3D 100; > + ConfirmKey =3D (CautionKey) ? SCAN_F12 : SCAN_F10; > + Timeout =3D (INT64)PcdGet32 (PcdPhysicalPresenceUserConfirmTimeout= ); > + if (Timeout > 0) { > + Timeout =3D (INT64)MultU64x32 ((UINT64)Timeout, 1000); > + } else { > + // > + // Wait forever > + // > + Timeout =3D MAX_INT64; > + } > + > + // > + // Wait for user response within the time-out > + // > do { > + MicroSecondDelay (Interval * 1000); > + > Status =3D gBS->CheckEvent (gST->ConIn->WaitForKey); > if (!EFI_ERROR (Status)) { > Status =3D gST->ConIn->ReadKeyStroke (gST->ConIn, &Key); > - if (Key.ScanCode =3D=3D SCAN_ESC) { > - InputKey =3D Key.ScanCode; > - } > - if ((Key.ScanCode =3D=3D SCAN_F10) && !CautionKey) { > - InputKey =3D Key.ScanCode; > - } > - if ((Key.ScanCode =3D=3D SCAN_F12) && CautionKey) { > - InputKey =3D Key.ScanCode; > + if (!EFI_ERROR (Status)) { > + if (Key.ScanCode =3D=3D ConfirmKey) { > + // > + // User Confirmation > + // > + return TRUE; > + } > + > + if (Key.ScanCode =3D=3D SCAN_ESC) { > + // > + // User Rejection > + // > + return FALSE; > + } > + } else if (Status =3D=3D EFI_DEVICE_ERROR) { > + // > + // If error, assume User Rejection > + // > + return FALSE; > } > } > - } while (InputKey =3D=3D 0); > - > - if (InputKey !=3D SCAN_ESC) { > - return TRUE; > - } > + Timeout -=3D Interval; > + } while (Timeout > 0); > =20 > return FALSE; > } (1) I don't understand why the original (pre-patch) code uses CheckEvent() in a busy loop. WaitForEvent() looks like a better (more resource-conservative) option. Does the original code use CheckEvent() because WaitForEvent() is restricted to TPL_APPLICATION? I don't think that being restricted to TPL_APPLICATION should be a problem. As far as I can tell, the only call path to Tcg2ReadUserKey() is: Tcg2PhysicalPresenceLibProcessRequest() Tcg2ExecutePendingTpmRequest() Tcg2UserConfirm() Tcg2ReadUserKey() In turn, Tcg2PhysicalPresenceLibProcessRequest() is supposed to be called from PlatformBootManagerLib, which in turn is called from BdsDxe. Therefore I think we can safely assume TPL_APPLICATION. I think we should add a separate patch first, for rewriting the present logic of Tcg2ReadUserKey() with WaitForEvent() -- remove the busy loop. (2) Once we use WaitForEvent(), it is easier and more elegant to use a timer event, in addition to "gST->ConIn->WaitForKey", to limit the waiting for a keypress. For example, the BdsWaitForSingleEvent() function in "MdeModulePkg/Universal/BdsDxe/BdsEntry.c" is used for implementing a timed wait for a hotkey, if I understand correctly. (3) I think that the Tcg2ReadUserKey() function should take the timeout parameter, and the PCD should be consumed in the Tcg2UserConfirm() function instead. Tcg2UserConfirm() should pass the value of the PCD to Tcg2ReadUserKey(). The PCD is called "PcdPhysicalPresenceUserConfirmTimeout", therefore it seems more closely tied to the Tcg2UserConfirm() function, in purpose. The Tcg2ReadUserKey() function seems to be a general utility function, so it should take a parameter, rather than fetch a particular PCD. (4) The comment block on the Tcg2ReadUserKey() function should be updated. (5) Please make sure that you format the patches next time with the "--stat=3D1000 --stat-graph-width=3D20" options. The pathnames of the files that are modified by this patch set are very long, and they are truncated in the diffstats. That way, the diffstat is not helpful. The "BaseTools/Scripts/SetupGit.py" script can automate at least the "--stat-graph-width=3D20" option for you. (6) When posting several patches, or large patches, it is helpful for reviewers if you also push your topic branch to your local repo, and expose the corresponding URL / branch name in the cover letter. Some patches are easier to review locally (after a git-fetch from your repo). (7) I don't know what happened, but I can see only patch#0 and patch#6 from this series in my list folder. There are multiple lib instances being modified, and I couldn't compare the changes between those. Thanks Laszlo > diff --git a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalP= resenceLib.inf b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2Physica= lPresenceLib.inf > index 85ce0e2b29..701de1836c 100644 > --- a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresence= Lib.inf > +++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresence= Lib.inf > @@ -20,7 +20,7 @@ > # This external input must be validated carefully to avoid security iss= ue. > # > # Copyright (C) 2018, Red Hat, Inc. > -# Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.
> +# Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.
> # SPDX-License-Identifier: BSD-2-Clause-Patent > # > ## > @@ -62,10 +62,14 @@ > UefiBootServicesTableLib > UefiLib > UefiRuntimeServicesTableLib > + TimerLib > =20 > [Protocols] > gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES > =20 > +[Pcd] > + gEfiSecurityPkgTokenSpaceGuid.PcdPhysicalPresenceUserConfirmTimeout > + > [Guids] > ## SOMETIMES_CONSUMES ## HII > gEfiTcg2PhysicalPresenceGuid >=20