Date   

回复: [PATCH v1 1/2] MdePkg:Update IndustryStandard/Nvme.h with Nvme amdin controller data

gaoliming
 

Cheng Zhou:
Please update the commit message to highlight this change based on NVME1.3
spec.

With this update, Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

Thanks
Liming

-----邮件原件-----
发件人: zhoucheng <zhoucheng@phytium.com.cn>
发送时间: 2021年4月22日 10:11
收件人: devel@edk2.groups.io
抄送: Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao
<gaoliming@byosoft.com.cn>
主题: [PATCH v1 1/2] MdePkg:Update IndustryStandard/Nvme.h with Nvme
amdin controller data

Add definition that describes the structure of Nvme admin controller data.

Signed-off-by: Cheng Zhou <zhoucheng@phytium.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Reviewed-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdePkg/Include/IndustryStandard/Nvme.h | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/IndustryStandard/Nvme.h
b/MdePkg/Include/IndustryStandard/Nvme.h
index 9b19a2074b0d..61bf0fadc05c 100644
--- a/MdePkg/Include/IndustryStandard/Nvme.h
+++ b/MdePkg/Include/IndustryStandard/Nvme.h
@@ -353,7 +353,13 @@ typedef struct {
UINT8 Npss; /* Number of Power States Support */
UINT8 Avscc; /* Admin Vendor Specific Command
Configuration */
UINT8 Apsta; /* Autonomous Power State Transition
Attributes */
- UINT8 Rsvd2[246]; /* Reserved as of Nvm Express 1.1 Spec
*/
+ UINT16 Wctemp; /* Warning Composite Temperature
Threshold */
+ UINT16 Cctemp; /* Critical Composite Temperature
Threshold */
+ UINT16 Mtfa; /* Maximum Time for Firmware
Activation */
+ UINT8 Hmpre[4]; /* Host Memory Buffer Preferred Size
*/
+ UINT8 Hmmin[4]; /* Host Memory Buffer Minimum Size
*/
+ UINT8 Tnvmcap[16]; /* Total NVM Capacity */
+ UINT8 Rsvd2[216]; /* Reserved as of Nvm Express 1.3 Spec
*/
//
// NVM Command Set Attributes
//
--
1.9.1


回复: [PATCH] * MdePkg/SmBios.h: Updated newly added socket info from smbios 3.4.

gaoliming
 

Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

-----邮件原件-----
发件人: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
发送时间: 2021年4月21日 14:53
收件人: Bhargava, Avinash <avinash.bhargava@intel.com>;
devel@edk2.groups.io
抄送: Ni, Ray <ray.ni@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
Kumar, Chandana C <chandana.c.kumar@intel.com>
主题: RE: [PATCH] * MdePkg/SmBios.h: Updated newly added socket info
from smbios 3.4.

Please also update SmbiosView library in ShellPkg with the new socket
types.
With that:
Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>

-----Original Message-----
From: Bhargava, Avinash <avinash.bhargava@intel.com>
Sent: Tuesday, April 20, 2021 7:39 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty; Chaganty, Rangasai V
<rangasai.v.chaganty@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
Kumar; Kumar, Chandana C <chandana.c.kumar@intel.com>
Subject: [PATCH] * MdePkg/SmBios.h: Updated newly added socket info from
smbios 3.4.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3140
Add theses socket type to MdePkg\Include\IndustryStandard\SmBios.h
-LGA1200
-LGA4189

Signed-off-by: Avinash Bhargava <avinash.bhargava@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Kumar, Chandana C <chandana.c.kumar@intel.com>
---
MdePkg/Include/IndustryStandard/SmBios.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/IndustryStandard/SmBios.h
b/MdePkg/Include/IndustryStandard/SmBios.h
index cc023b7369..39dfe4e137 100644
--- a/MdePkg/Include/IndustryStandard/SmBios.h
+++ b/MdePkg/Include/IndustryStandard/SmBios.h
@@ -1,7 +1,7 @@
/** @file

Industry Standard Definitions of SMBIOS Table Specification v3.3.0.



-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>

+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>

(C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP<BR>

(C) Copyright 2015 - 2019 Hewlett Packard Enterprise Development LP<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -810,7 +810,9 @@ typedef enum {
ProcessorUpgradeSocketLGA2066 = 0x39,

ProcessorUpgradeSocketBGA1392 = 0x3A,

ProcessorUpgradeSocketBGA1510 = 0x3B,

- ProcessorUpgradeSocketBGA1528 = 0x3C

+ ProcessorUpgradeSocketBGA1528 = 0x3C,

+ ProcessorUpgradeSocketLGA4189 = 0x3D,

+ ProcessorUpgradeSocketLGA1200 = 0x3E

} PROCESSOR_UPGRADE;



///

--
2.28.0.windows.1


Re: [PATCH] Maintainers.txt: Add 'Erdem Aktas' to Confidential Computing reviewers

Min Xu
 

Acked-by: min.m.xu@intel.com

Thanks!
Xu, Min

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Erdem
Aktas via groups.io
Sent: Thursday, April 22, 2021 11:05 PM
To: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>
Cc: Yao@google.com; Yao, Jiewen <jiewen.yao@intel.com>; Paolo Bonzini
<pbonzini@redhat.com>; jejb @ linux . ibm . com <jejb@linux.ibm.com>;
dgilbert @ redhat . com <dgilbert@redhat.com>; Xu@google.com; Xu, Min M
<min.m.xu@intel.com>; thomas . lendacky @ amd . com
<thomas.lendacky@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
Justen@google.com; Justen, Jordan L <jordan.l.justen@intel.com>; Ard
Biesheuvel <ardb+tianocore@kernel.org>; Nathaniel McCallum
<npmccallum@redhat.com>; Ning Yang <ningyang@google.com>; Erdem
Aktas <erdemaktas@google.com>
Subject: [edk2-devel] [PATCH] Maintainers.txt: Add 'Erdem Aktas' to
Confidential Computing reviewers

Add 'Erdem Aktas' as a reviewer for OvmfPkg/Confidential Computing.

Signed-off-by: Erdem Aktas <erdemaktas@google.com>
---
Maintainers.txt | 1 +
1 file changed, 1 insertion(+)

diff --git a/Maintainers.txt b/Maintainers.txt index fda3df5de2..cafe6b1ab8
100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -458,6 +458,7 @@ F: OvmfPkg/PlatformPei/AmdSev.c
F: OvmfPkg/ResetVector/
F: OvmfPkg/Sec/
R: Brijesh Singh <brijesh.singh@amd.com>
+R: Erdem Aktas <erdemaktas@google.com>
R: James Bottomley <jejb@linux.ibm.com>
R: Jiewen Yao <jiewen.yao@intel.com>
R: Min Xu <min.m.xu@intel.com>
--
2.31.1.498.g6c1eba8ee3d-goog





Re: [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV

Lendacky, Thomas
 

On 4/22/21 3:39 AM, Laszlo Ersek wrote:
On 04/22/21 09:34, Laszlo Ersek wrote:

The new InternalTpmDecryptAddressRange() function should be called
from Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().
Unfortunately, this method doesn't work. The OVMF Tcg2ConfigPei.inf file
uses the SecurityPkg Tpm2DeviceLib library. The SecurityPkg Tpm2DeviceLib
library's constructor is called before the OVMF Tcg2ConfigPei constructor.
The Tpm2DeviceLib constructor performs MMIO to the TPM base address and
fails because the pages haven't been marked unencrypted yet by OVMF
Tcg2ConfigPei. Some debug output:

Loading PEIM at 0x0007F793000 EntryPoint=0x0007F794E4F Tcg2ConfigPei.efi
*** DEBUG: InternalTpm2DeviceLibDTpmCommonConstructor:55
*** DEBUG: Tpm2GetPtpInterface:425
*** DEBUG: Tpm2IsPtpPresence:51
MMIO using encrypted memory: FED40000
!!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - 00000000 !!!!

Thanks,
Tom

Sorry, another point:

(6) where we determine that no TPM is available:

//
// If no TPM2 was detected, we still need to install
// TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon seeing
// the default (all-bits-zero) contents of PcdTpmInstanceGuid, thus we have
// to install the PPI in its place, in order to unblock any dependent
// PEIMs.
//
Status = PeiServicesInstallPpi (&mTpmInitializationDonePpiList);

we should re-encrypt the address range, as if nothing had happened.

For this, we'll likely need a similarly polymorphic function called
InternalTpmEncryptAddressRange().

(

For some background on this particular branch of the code, please refer
to commit 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone",
2018-03-09):

- Check the QEMU hardware for TPM2 availability only

- If found, set the dynamic PCD "PcdTpmInstanceGuid" to
&gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
the firmware about the TPM type.

- Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
and the consumption of the "TPM type" PCD.

- If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
(Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)

)

Thanks
Laszlo


Re: [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV

Lendacky, Thomas
 

On 4/22/21 9:51 AM, Tom Lendacky wrote:
On 4/22/21 2:34 AM, Laszlo Ersek wrote:
On 04/21/21 01:13, Lendacky, Thomas wrote:
On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C6b8da1f9a3bf4fb5f01e08d905613998%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546737416495415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=5vPlHPzGlS2%2Bqu3U4RPMITpyY%2F2ZxKJlaVYfFZItONQ%3D&amp;reserved=0

The TPM support in OVMF performs MMIO accesses during the PEI phase. At
this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
guest will fail attempting to perform MMIO to an encrypted address.
(1) The subject says SEV, not SEV-ES, and the code in the patch too
suggests SEV, not SEV-ES. If that's correct, can you please update the
commit message?
Yes, I'll update the commit message. The action is correct for all SEV
guests in general, but it is only with SEV-ES, where the tighter MMIO
checks can be performed, that an actual issue shows up.



Read the PcdTpmBaseAddress and mark the specification defined range
(0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
the MMIO requests.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 6ef77ba7bb21..de60332e9390 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -113,6 +113,7 @@ [Pcd]

[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda4b..d524929f9e10 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -141,6 +141,7 @@ AmdSevInitialize (
)
{
UINT64 EncryptionMask;
+ UINT64 TpmBaseAddress;
RETURN_STATUS PcdStatus;

//
@@ -206,6 +207,24 @@ AmdSevInitialize (
}
}

+ //
+ // PEI TPM support will perform MMIO accesses, be sure this range is not
+ // marked encrypted.
+ //
+ TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
+ if (TpmBaseAddress != 0) {
+ RETURN_STATUS DecryptStatus;
+
+ DecryptStatus = MemEncryptSevClearPageEncMask (
+ 0,
+ TpmBaseAddress,
+ EFI_SIZE_TO_PAGES (0x5000),
+ FALSE
+ );
+
+ ASSERT_RETURN_ERROR (DecryptStatus);
+ }
+
Laszlo, I'm not sure if this is the best way to approach this. It is
simple and straight forward and the TCG/TPM support is one of the few
(only?) pieces of code that does actual MMIO during PEI that is bitten
by not having the address marked as shared/unencrypted.
In SEC, I think we have MMIO access too (LAPIC --
InitializeApicTimer()); why does that work?

Hmm... Is that because we're immediately in x2apic mode, and that means
CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
support", 2015-11-30).) And, we have #VC handling in SEC too.
Missed this question in my earlier reply... LAPIC access has a dedicated
check in ValidateMmioMemory() to allow access in this case.

Thanks,
Tom


Anyway: I think the TPM (MMIO) access you see comes from this PEIM:

OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf

The driver uses the following library instance:

SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf

This library instance is what depends on "PcdTpmBaseAddress".

And it's not just that decrypting the TPM MMIO range in PlatformPei
"looks awkward", but I don't even see it immediately why PlatformPei is
guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
build report file. If Tcg2ConfigPei runs first, whatever we do in
PlatformPei is too late.

I also don't like that, with this patch, we'd decrypt the TPM range even
if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
"PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
too.)


(2) So, can you please try the following, in the
"OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:
I'll take the input from each of your emails on this and see how that all
works. Thanks for the insight and knowledge!

Tom


diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index 6776ec931ce0..0d0572b83599 100644
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -20,13 +20,16 @@ [Defines]
ENTRY_POINT = Tcg2ConfigPeimEntryPoint

[Sources]
+ MemEncrypt.h
Tcg2ConfigPeim.c
Tpm12Support.h

[Sources.IA32, Sources.X64]
+ MemEncryptSev.c
Tpm12Support.c

[Sources.ARM, Sources.AARCH64]
+ MemEncryptNull.c
Tpm12SupportNull.c

[Packages]
@@ -43,6 +46,7 @@ [LibraryClasses]

[LibraryClasses.IA32, LibraryClasses.X64]
BaseLib
+ MemEncryptSevLib
Tpm12DeviceLib

[Guids]
@@ -56,6 +60,9 @@ [Ppis]
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES

+[Pcd.IA32, Pcd.X64]
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
+
[Depex.IA32, Depex.X64]
TRUE
In the "MemEncrypt.h" file, declare a function called
InternalTpmDecryptAddressRange(). The function definition in
"MemEncryptNull.c" should do nothing, while the one in "MemEncryptSev.c"
should check MemEncryptSevIsEnabled(), and then make the above-seen
MemEncryptSevClearPageEncMask() call.

The new InternalTpmDecryptAddressRange() function should be called from
Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().

(An alternative approach would be to call MemEncryptSevIsEnabled() and
MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
on ARM / AARCH64. In addition to that, we'd have to implement a Null
instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
instance in the ArmVirtPkg DSC files. But I don't like that: the library
*class* carries SEV in the name, which is inherently X64-specific, thus
I wouldn't even like the lib *class* to leak into ArmVirtPkg.)


(3) If the approach in (2) works, then please don't forget to update the
patch subject (it currently refers to PlatformPei).


(4) The argument of the EFI_SIZE_TO_PAGES() function-like macro should
have type UINTN. The constant 0x5000 has type "int" (INT32); please cast
it to UINTN.

(In fact I would prefer a new macro for 0x5000, somewhere in the
"MdePkg/Include/IndustryStandard/Tpm*.h" files; but I can see that
SecurityPkg already open-codes the 0x5000 constant in
"Tcg/Tcg2Acpi/Tpm.asl" and "Tcg/TcgSmm/Tpm.asl", so meh.)

Thanks
Laszlo


Re: [PATCH] Maintainers.txt: Add 'Erdem Aktas' to Confidential Computing reviewers

Yao, Jiewen
 

Acked-by: jiewen.yao@intel.com

thank you!
Yao, Jiewen

在 2021年4月22日,下午11:05,Erdem Aktas via groups.io <erdemaktas=google.com@groups.io> 写道:

Add 'Erdem Aktas' as a reviewer for OvmfPkg/Confidential Computing.

Signed-off-by: Erdem Aktas <erdemaktas@google.com>
---
Maintainers.txt | 1 +
1 file changed, 1 insertion(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index fda3df5de2..cafe6b1ab8 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -458,6 +458,7 @@ F: OvmfPkg/PlatformPei/AmdSev.c
F: OvmfPkg/ResetVector/
F: OvmfPkg/Sec/
R: Brijesh Singh <brijesh.singh@amd.com>
+R: Erdem Aktas <erdemaktas@google.com>
R: James Bottomley <jejb@linux.ibm.com>
R: Jiewen Yao <jiewen.yao@intel.com>
R: Min Xu <min.m.xu@intel.com>
--
2.31.1.498.g6c1eba8ee3d-goog






Re: [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

Lendacky, Thomas
 

On 4/22/21 9:15 AM, Tom Lendacky wrote:
On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
On 04/21/21 00:54, Lendacky, Thomas wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C19a7d97e2a7b461830ed08d905528472%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546674232278910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&amp;reserved=0

Enabling TPM support results in guest termination of an SEV-ES guest
because it uses MMIO opcodes that are not currently supported.

Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
use a memory offset directly encoded in the instruction. Also, add a DEBUG
statement to identify an unsupported MMIO opcode being used.

Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 99 +++++++++++++++++++
1 file changed, 99 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 273f36499988..f9660b757d8e 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -678,6 +678,7 @@ MmioExit (
UINTN Bytes;
UINT64 *Register;
UINT8 OpCode, SignByte;
+ UINTN Address;

Bytes = 0;

@@ -727,6 +728,51 @@ MmioExit (
}
break;

+ //
+ // MMIO write (MOV moffsetX, aX)
+ //
+ case 0xA2:
+ Bytes = 1;
+ //
+ // fall through
+ //
+ case 0xA3:
+ Bytes = ((Bytes != 0) ? Bytes :
+ (InstructionData->DataSize == Size16Bits) ? 2 :
+ (InstructionData->DataSize == Size32Bits) ? 4 :
+ (InstructionData->DataSize == Size64Bits) ? 8 :
+ 0);
+
+ InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+ InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
+
+ if (InstructionData->AddrSize == Size8Bits) {
+ Address = *(UINT8 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size16Bits) {
+ Address = *(UINT16 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size32Bits) {
+ Address = *(UINT32 *) InstructionData->Immediate;
+ } else {
+ Address = *(UINTN *) InstructionData->Immediate;
+ }
(1) Can we simplify this as follows?

InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
InstructionData->End += InstructionData->ImmediateSize;
Address = 0;
CopyMem (&Address, InstructionData->Immediate,
InstructionData->ImmediateSize);
Yup, that can be done.
"Address" is a type UINTN, but since this is X64 only code, an 8-byte copy
isn't an issue. Should I add a comment about that above the setting of
"Address"? Or should I convert "Address" to a UINT64 - although
ValidateMmioMemory expects a UINTN... Thoughts?

Thanks,
Tom



+
+ Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+ if (Status != 0) {
+ return Status;
+ }
+
+ ExitInfo1 = Address;
+ ExitInfo2 = Bytes;
+ CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
+ if (Status != 0) {
+ return Status;
+ }
+ break;
+
//
// MMIO write (MOV reg/memX, immX)
//
@@ -809,6 +855,58 @@ MmioExit (
CopyMem (Register, Ghcb->SharedBuffer, Bytes);
break;

+ //
+ // MMIO read (MOV aX, moffsetX)
+ //
+ case 0xA0:
+ Bytes = 1;
+ //
+ // fall through
+ //
+ case 0xA1:
+ Bytes = ((Bytes != 0) ? Bytes :
+ (InstructionData->DataSize == Size16Bits) ? 2 :
+ (InstructionData->DataSize == Size32Bits) ? 4 :
+ (InstructionData->DataSize == Size64Bits) ? 8 :
+ 0);
+
+ InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+ InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
+
+ if (InstructionData->AddrSize == Size8Bits) {
+ Address = *(UINT8 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size16Bits) {
+ Address = *(UINT16 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size32Bits) {
+ Address = *(UINT32 *) InstructionData->Immediate;
+ } else {
+ Address = *(UINTN *) InstructionData->Immediate;
+ }
(2) Similar question as (1).
Will do.


+
+ Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+ if (Status != 0) {
+ return Status;
+ }
+
+ ExitInfo1 = Address;
+ ExitInfo2 = Bytes;
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
+ if (Status != 0) {
+ return Status;
+ }
+
+ if (Bytes == 4) {
+ //
+ // Zero-extend for 32-bit operation
+ //
+ Regs->Rax = 0;
+ }
(3) This is also seen with opcode 0x8B, but can you remind me please why
we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
32-bit operand size the 32-bit register results are zero-extended to 64-bits.


+ CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
+ break;
+
//
// MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
//
@@ -886,6 +984,7 @@ MmioExit (
break;

default:
+ DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
Status = GP_EXCEPTION;
ASSERT (FALSE);
}
(4) We should use the DEBUG_ERROR log mask here.
Will change.

Thanks,
Tom


Thanks
Laszlo






Re: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS verify support

Yao, Jiewen
 

I think we have some EDKII tool will use the Signing capability, but it is not needed during BIOS boot. That is why Signing function is in Ext.c, while verify function in in Basic.c

Please also add crypto unit test for both API - https://github.com/tianocore/edk2/tree/master/CryptoPkg/Test

Thank you
Yao Jiewen

-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Thursday, April 22, 2021 10:16 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS verify
support

Hi Jiewen,

Thanks for sharing these references.

We are currently using Salt Length of digest length.
I will add the test for new API in the unit test framework in the next version of
the patch.

In reference to adding support for RsaPssSign() API : This maybe due to my
ignorance, but I am unaware of usages where BIOS is involved in doing
asymmetric signing during run time. I do see that CryptoPkg also contains TLS
interface and that would involve asymmetric signing, but that will directly use
the OpenSSL's TLS interface for signing. And, therefore I was skeptical about
adding RsaPssSign interface.

Thanks
Sachin

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Tuesday, April 20, 2021 6:29 PM
To: Agrawal, Sachin <sachin.agrawal@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS verify
support

HI Sachin
Sorry, I forget to add link for the reference.

1) TPM2 Library Specification, part 2 structure
(https://trustedcomputinggroup.org/wp-
content/uploads/TCG_TPM2_r1p64_Part2_Structures_15may2021.pdf)
describes the PSS salt length.

For the TPM_ALG_RSAPSS signing scheme, ...
.... The salt size is
always the largest salt value that will fit into the available space.


2) NIST FIPS 186-5 draft
(https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5-draft.pdf) and NIST
FIPS 186-4 (https://doi.org/10.6028/NIST.FIPS.186-4) says:

For RSASSA-PSS,
the length (in bytes) of the salt (sLen) shall satisfy 0 ≤ sLen ≤ hLen

3) TCG FIPS 140-2 Guidance for TPM2
(https://trustedcomputinggroup.org/resource/tcg-fips-140-2-guidance-for-
tpm-2-0/) mentions:

Language in [1] Part 1 Appendix B.7 RSASSA_PSS indicates:
"For both restricted and unrestricted signing keys, the random salt length
will be the largest
size allowed by the key size and message digest size.
NOTE If the TPM implementation is required to be compliant with FIPS 186-4,
then the random
salt length will be the largest size allowed by that specification."

4) TLS1.3 - RFC8446 (https://datatracker.ietf.org/doc/rfc8446/) has below.

RSASSA-PSS PSS algorithms:
The length of the Salt MUST be equal to the length of the digest
algorithm.


My view is that, TLS 1.3 and TPM FIPS mode require salt length == hash length,
explicitly.

May I know that in your production, which salt length you choose in signing?
If you also choose salt length == hash length, then I would recommend make
the default behavior to be HASH_LEN instead of AUTO.

Also, may I recommend we add RsaPssSign API as well?

Please also add the new API to the crypto test unit test.


I notice that crypto implementation (such as openssl, mbedtls) has API to let
caller indicate what is the expected salt length. The caller may want AUTO or
MAX in their special environment. I am OK to add another API later (such as
RsaPssVerifyEx) to satisfy that need, if there is real use case.




-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Tuesday, April 20, 2021 11:20 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Hi Jiewen,

I reviewed RFC 8017 and I could not find any specific
'recommendations' on salt length to be used during signing with PSS
encoding scheme.
However, in Section D.5.2.2.1(Notes 2) of IEEE 1363a-2004, it is
recommended to use salt length atleast equal to the hash digest length.

We can modify the current API to take a additional parameter as salt
length and ONLY pursue verification operation if Salt length is
atleast equal to digest length.
This will act as a hardening mechanism for Edk2 as it will accept
signatures only with 'appropriate' salt lengths.

Let me know if this is fine and I will push a corresponding patch.

Thx
Sachin


-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Tuesday, April 20, 2021 2:12 AM
To: Agrawal, Sachin <sachin.agrawal@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Right. That has PROs and CONs.

On one hand, that allows maximum compatibility, salt could be
HASH_SIZE or MAX, or even 0 ?

On the other hand, what if the consumer only wants to accept a
specific length? E.g. TPM in FIPS mode and TLS requires
SaltLength==HashLength.

Thank you
Yao Jiewen


-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Tuesday, April 20, 2021 3:19 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Hi Jiewen,

From Section 9.1 in RFC 8017:
" Note that the verification operation follows reverse steps to recover
salt and then forward steps to recompute and compare H."

Therefore, salt length can be inferred from the PSS block structure
during verification operation.

I opted for 'RSA_PSS_SALTLEN_AUTO' as it will allow Edk2 to verify
PSS signatures of any salt lengths.

Thanks
Sachin

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Monday, April 19, 2021 7:30 PM
To: Agrawal, Sachin <sachin.agrawal@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Hi Sachin
May I know why you hardcode PSS salt length to be
RSA_PSS_SALTLEN_AUTO ?

Thank you
Yao Jiewen


-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Tuesday, April 20, 2021 10:02 AM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>;
Jiang, Guomin <guomin.jiang@intel.com>; Agrawal, Sachin
<sachin.agrawal@intel.com>
Subject: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3314

This patch uses Openssl's EVP API's to perform RSASSA-PSS
verification of a binary blob.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>

Signed-off-by: Sachin Agrawal <sachin.agrawal@intel.com>
---
CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c | 139
++++++++++++++++++++
CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c | 43 ++++++
CryptoPkg/Include/Library/BaseCryptLib.h | 27 ++++
CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 1 +
7 files changed, 213 insertions(+)

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
new file mode 100644
index 000000000000..acf5eb689cd8
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
@@ -0,0 +1,139 @@
+/** @file
+ RSA Asymmetric Cipher Wrapper Implementation over OpenSSL.
+
+ This file implements following APIs which provide basic
+ capabilities for
RSA:
+ 1) RsaPssVerify
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+#include <openssl/bn.h>
+#include <openssl/rsa.h>
+#include <openssl/objects.h>
+#include <openssl/evp.h>
+
+
+/**
+ Retrieve a pointer to EVP message digest object.
+
+ @param[in] DigestLen Length of the message digest.
+
+**/
+static
+EVP_MD*
+GetEvpMD (
+ IN UINT16 DigestLen
+ )
+{
+ switch (DigestLen){
+ case SHA256_DIGEST_SIZE:
+ return EVP_sha256();
+ break;
+ case SHA384_DIGEST_SIZE:
+ return EVP_sha384();
+ break;
+ case SHA512_DIGEST_SIZE:
+ return EVP_sha512();
+ break;
+ default:
+ return NULL;
+ }
+}
+
+
+/**
+ Verifies the RSA signature with RSASSA-PSS signature scheme
+defined in RFC
8017.
+ Implementation determines salt length automatically from the
+ signature
encoding.
+ Mask generation function is the same as the message digest algorithm.
+
+ @param[in] RsaContext Pointer to RSA context for signature
verification.
+ @param[in] Message Pointer to octet message to be verified.
+ @param[in] MsgSize Size of the message in bytes.
+ @param[in] Signature Pointer to RSASSA-PSS signature to be
verified.
+ @param[in] SigSize Size of signature in bytes.
+ @param[in] DigestLen Length of digest for RSA operation.
+
+ @retval TRUE Valid signature encoded in RSASSA-PSS.
+ @retval FALSE Invalid signature or invalid RSA context.
+
+**/
+BOOLEAN
+EFIAPI
+RsaPssVerify (
+ IN VOID *RsaContext,
+ IN CONST UINT8 *Message,
+ IN UINTN MsgSize,
+ IN CONST UINT8 *Signature,
+ IN UINTN SigSize,
+ IN UINT16 DigestLen
+ )
+{
+ BOOLEAN Result;
+ EVP_PKEY *pEvpRsaKey = NULL;
+ EVP_MD_CTX *pEvpVerifyCtx = NULL;
+ EVP_PKEY_CTX *pKeyCtx = NULL;
+ CONST EVP_MD *HashAlg = NULL;
+
+ if (RsaContext == NULL) {
+ return FALSE;
+ }
+ if (Message == NULL || MsgSize == 0 || MsgSize > INT_MAX) {
+ return FALSE;
+ }
+ if (Signature == NULL || SigSize == 0 || SigSize > INT_MAX) {
+ return FALSE;
+ }
+
+ HashAlg = GetEvpMD(DigestLen);
+
+ if (HashAlg == NULL) {
+ return FALSE;
+ }
+
+ pEvpRsaKey = EVP_PKEY_new();
+ if (pEvpRsaKey == NULL) {
+ goto _Exit;
+ }
+
+ EVP_PKEY_set1_RSA(pEvpRsaKey, RsaContext);
+
+ pEvpVerifyCtx = EVP_MD_CTX_create(); if (pEvpVerifyCtx == NULL) {
+ goto _Exit;
+ }
+
+ Result = EVP_DigestVerifyInit(pEvpVerifyCtx, &pKeyCtx, HashAlg,
+ NULL,
pEvpRsaKey) > 0;
+ if (pKeyCtx == NULL) {
+ goto _Exit;
+ }
+
+ if (Result) {
+ Result = EVP_PKEY_CTX_set_rsa_padding(pKeyCtx,
RSA_PKCS1_PSS_PADDING) > 0;
+ }
+ if (Result) {
+ Result = EVP_PKEY_CTX_set_rsa_pss_saltlen(pKeyCtx,
RSA_PSS_SALTLEN_AUTO) > 0;
+ }
+ if (Result) {
+ Result = EVP_PKEY_CTX_set_rsa_mgf1_md(pKeyCtx, HashAlg) > 0;
+ } if (Result) {
+ Result = EVP_DigestVerifyUpdate(pEvpVerifyCtx, Message,
(UINT32)MsgSize) > 0;
+ }
+ if (Result) {
+ Result = EVP_DigestVerifyFinal(pEvpVerifyCtx, Signature,
+ (UINT32)SigSize) > 0; }
+
+_Exit :
+ if (pEvpRsaKey) {
+ EVP_PKEY_free(pEvpRsaKey);
+ }
+ if (pEvpVerifyCtx) {
+ EVP_MD_CTX_destroy(pEvpVerifyCtx);
+ }
+
+ return Result;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c
new file mode 100644
index 000000000000..8d84b4c1426c
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c
@@ -0,0 +1,43 @@
+/** @file
+ RSA-PSS Asymmetric Cipher Wrapper Implementation over OpenSSL.
+
+ This file does not provide real capabilities for following APIs
+ in RSA
handling:
+ 1) RsaPssVerify
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Verifies the RSA signature with RSASSA-PSS signature scheme
+defined in RFC
8017.
+ Implementation determines salt length automatically from the
+ signature
encoding.
+ Mask generation function is the same as the message digest algorithm.
+
+ @param[in] RsaContext Pointer to RSA context for signature
verification.
+ @param[in] Message Pointer to octet message to be verified.
+ @param[in] MsgSize Size of the message in bytes.
+ @param[in] Signature Pointer to RSASSA-PSS signature to be
verified.
+ @param[in] SigSize Size of signature in bytes.
+ @param[in] DigestLen Length of digest for RSA operation.
+
+ @retval TRUE Valid signature encoded in RSASSA-PSS.
+ @retval FALSE Invalid signature or invalid RSA context.
+
+**/
+BOOLEAN
+EFIAPI
+RsaPssVerify (
+ IN VOID *RsaContext,
+ IN CONST UINT8 *Message,
+ IN UINTN MsgSize,
+ IN CONST UINT8 *Signature,
+ IN UINTN SigSize,
+ IN UINT16 DigestLen
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
b/CryptoPkg/Include/Library/BaseCryptLib.h
index 496121e6a4ed..36d560b8d691 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1363,6 +1363,33 @@ RsaPkcs1Verify (
IN UINTN SigSize
);

+/**
+ Verifies the RSA signature with RSASSA-PSS signature scheme
+defined in RFC
8017.
+ Implementation determines salt length automatically from the
+ signature
encoding.
+ Mask generation function is the same as the message digest algorithm.
+
+ @param[in] RsaContext Pointer to RSA context for signature
verification.
+ @param[in] Message Pointer to octet message to be verified.
+ @param[in] MsgSize Size of the message in bytes.
+ @param[in] Signature Pointer to RSASSA-PSS signature to be
verified.
+ @param[in] SigSize Size of signature in bytes.
+ @param[in] DigestLen Length of digest for RSA operation.
+
+ @retval TRUE Valid signature encoded in RSASSA-PSS.
+ @retval FALSE Invalid signature or invalid RSA context.
+
+**/
+BOOLEAN
+EFIAPI
+RsaPssVerify (
+ IN VOID *RsaContext,
+ IN CONST UINT8 *Message,
+ IN UINTN MsgSize,
+ IN CONST UINT8 *Signature,
+ IN UINTN SigSize,
+ IN UINT16 DigestLen
+ );
+
/**
Retrieve the RSA Private Key from the password-protected PEM key
data.


[PATCH] Maintainers.txt: Add 'Erdem Aktas' to Confidential Computing reviewers

Erdem Aktas
 

Add 'Erdem Aktas' as a reviewer for OvmfPkg/Confidential Computing.

Signed-off-by: Erdem Aktas <erdemaktas@google.com>
---
Maintainers.txt | 1 +
1 file changed, 1 insertion(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index fda3df5de2..cafe6b1ab8 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -458,6 +458,7 @@ F: OvmfPkg/PlatformPei/AmdSev.c
F: OvmfPkg/ResetVector/
F: OvmfPkg/Sec/
R: Brijesh Singh <brijesh.singh@amd.com>
+R: Erdem Aktas <erdemaktas@google.com>
R: James Bottomley <jejb@linux.ibm.com>
R: Jiewen Yao <jiewen.yao@intel.com>
R: Min Xu <min.m.xu@intel.com>
--
2.31.1.498.g6c1eba8ee3d-goog


Re: [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV

Lendacky, Thomas
 

On 4/22/21 2:34 AM, Laszlo Ersek wrote:
On 04/21/21 01:13, Lendacky, Thomas wrote:
On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C6b8da1f9a3bf4fb5f01e08d905613998%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546737416495415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=5vPlHPzGlS2%2Bqu3U4RPMITpyY%2F2ZxKJlaVYfFZItONQ%3D&amp;reserved=0

The TPM support in OVMF performs MMIO accesses during the PEI phase. At
this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
guest will fail attempting to perform MMIO to an encrypted address.
(1) The subject says SEV, not SEV-ES, and the code in the patch too
suggests SEV, not SEV-ES. If that's correct, can you please update the
commit message?
Yes, I'll update the commit message. The action is correct for all SEV
guests in general, but it is only with SEV-ES, where the tighter MMIO
checks can be performed, that an actual issue shows up.



Read the PcdTpmBaseAddress and mark the specification defined range
(0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
the MMIO requests.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 6ef77ba7bb21..de60332e9390 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -113,6 +113,7 @@ [Pcd]

[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda4b..d524929f9e10 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -141,6 +141,7 @@ AmdSevInitialize (
)
{
UINT64 EncryptionMask;
+ UINT64 TpmBaseAddress;
RETURN_STATUS PcdStatus;

//
@@ -206,6 +207,24 @@ AmdSevInitialize (
}
}

+ //
+ // PEI TPM support will perform MMIO accesses, be sure this range is not
+ // marked encrypted.
+ //
+ TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
+ if (TpmBaseAddress != 0) {
+ RETURN_STATUS DecryptStatus;
+
+ DecryptStatus = MemEncryptSevClearPageEncMask (
+ 0,
+ TpmBaseAddress,
+ EFI_SIZE_TO_PAGES (0x5000),
+ FALSE
+ );
+
+ ASSERT_RETURN_ERROR (DecryptStatus);
+ }
+
Laszlo, I'm not sure if this is the best way to approach this. It is
simple and straight forward and the TCG/TPM support is one of the few
(only?) pieces of code that does actual MMIO during PEI that is bitten
by not having the address marked as shared/unencrypted.
In SEC, I think we have MMIO access too (LAPIC --
InitializeApicTimer()); why does that work?

Hmm... Is that because we're immediately in x2apic mode, and that means
CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
support", 2015-11-30).) And, we have #VC handling in SEC too.

Anyway: I think the TPM (MMIO) access you see comes from this PEIM:

OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf

The driver uses the following library instance:

SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf

This library instance is what depends on "PcdTpmBaseAddress".

And it's not just that decrypting the TPM MMIO range in PlatformPei
"looks awkward", but I don't even see it immediately why PlatformPei is
guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
build report file. If Tcg2ConfigPei runs first, whatever we do in
PlatformPei is too late.

I also don't like that, with this patch, we'd decrypt the TPM range even
if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
"PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
too.)


(2) So, can you please try the following, in the
"OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:
I'll take the input from each of your emails on this and see how that all
works. Thanks for the insight and knowledge!

Tom


diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index 6776ec931ce0..0d0572b83599 100644
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -20,13 +20,16 @@ [Defines]
ENTRY_POINT = Tcg2ConfigPeimEntryPoint

[Sources]
+ MemEncrypt.h
Tcg2ConfigPeim.c
Tpm12Support.h

[Sources.IA32, Sources.X64]
+ MemEncryptSev.c
Tpm12Support.c

[Sources.ARM, Sources.AARCH64]
+ MemEncryptNull.c
Tpm12SupportNull.c

[Packages]
@@ -43,6 +46,7 @@ [LibraryClasses]

[LibraryClasses.IA32, LibraryClasses.X64]
BaseLib
+ MemEncryptSevLib
Tpm12DeviceLib

[Guids]
@@ -56,6 +60,9 @@ [Ppis]
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES

+[Pcd.IA32, Pcd.X64]
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
+
[Depex.IA32, Depex.X64]
TRUE
In the "MemEncrypt.h" file, declare a function called
InternalTpmDecryptAddressRange(). The function definition in
"MemEncryptNull.c" should do nothing, while the one in "MemEncryptSev.c"
should check MemEncryptSevIsEnabled(), and then make the above-seen
MemEncryptSevClearPageEncMask() call.

The new InternalTpmDecryptAddressRange() function should be called from
Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().

(An alternative approach would be to call MemEncryptSevIsEnabled() and
MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
on ARM / AARCH64. In addition to that, we'd have to implement a Null
instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
instance in the ArmVirtPkg DSC files. But I don't like that: the library
*class* carries SEV in the name, which is inherently X64-specific, thus
I wouldn't even like the lib *class* to leak into ArmVirtPkg.)


(3) If the approach in (2) works, then please don't forget to update the
patch subject (it currently refers to PlatformPei).


(4) The argument of the EFI_SIZE_TO_PAGES() function-like macro should
have type UINTN. The constant 0x5000 has type "int" (INT32); please cast
it to UINTN.

(In fact I would prefer a new macro for 0x5000, somewhere in the
"MdePkg/Include/IndustryStandard/Tpm*.h" files; but I can see that
SecurityPkg already open-codes the 0x5000 constant in
"Tcg/Tcg2Acpi/Tpm.asl" and "Tcg/TcgSmm/Tpm.asl", so meh.)

Thanks
Laszlo


Re: separate OVMF binary for TDX? [was: OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest]

Laszlo Ersek
 

On 04/21/21 19:07, Erdem Aktas wrote:
Hi Laszlo,

I am sorry to hear that it sounded like we are dictating a certain
approach. Although I can see why it sounded that way, it certainly was not
my intention.
We want to work with the EDK2 community to have a solution that is
beneficial for everyone and we appreciate the inputs that we got from you
and Paolo. Code quality is always a high priority for us. Therefore, if,
at some point, things get too hacky to impact the
quality/maintainability of the upstream code, we will consider making
adjustments on our side.

With the current discussion, I was just trying to describe our use case and
the importance of having a single binary where there is no absolute need
for architectural differences. As far as I know, the only problematic area
is modifying the reset vector to be compatible with TDX and it seems like
Intel has a solution for it without impacting the overall quality of the
upstream code. I just want to reiterate that we are open for discussion and
what we ask should not be considered "at all cost" and please let us know
that if edk2 community/maintainers are still thinking that what Intel is
proposing is not feasible.
OK.

It's not lost on me that we're talking about ~3 instructions.

Let's keep a close eye on further "polymorphisms" that would require hacks.


Can Google at least propose a designated reviewer ("R") for the
"OvmfPkg: Confidential Computing" section of "Maintainers.txt", in a
patch?
Sure I would be happy too.
Yes, please do that. It can be included in the TDX patch set from Min Xu
that modifies the beginning of reset vector as discussed above.

Thanks!
Laszlo


-Erdem

On Wed, Apr 21, 2021 at 3:44 AM Laszlo Ersek <lersek@redhat.com> wrote:

On 04/21/21 02:38, Yao, Jiewen wrote:
Hello
Do we have some conclusion on this topic?

Do we agree the one-binary solution in OVMF or we need more discussion?
Well it's not technically impossible to do, just very ugly and brittle.
And I'm doubtful that this is a unique problem ("just fix the reset
vector") the likes of which will supposedly never return during the
integration of SEV and TDX. Once we make this promise ("one firmware
binary at all costs"), the hacks we accept for its sake will only
accumulate over time, and we'll have more and more precedent to justify
the next hack. Technical debt is not exactly what we don't have enough
of, in edk2.

I won't make a secret out of the fact that I'm slightly annoyed that
this approach is being dictated by Google (as far as I understand, at
this point, anyway). I don't see or recall a lot of Google contributions
in the edk2 history or the bug tracker. I'm not enthusiastic about
complexity without explicit commitment / investment on the beneficiary's
side.

I won't nack the approach personally, but I'm quite unhappy about it.
Can Google at least propose a designated reviewer ("R") for the
"OvmfPkg: Confidential Computing" section of "Maintainers.txt", in a patch?

Thanks
Laszlo



Thank you
Yao Jiewen

-----Original Message-----
From: Erdem Aktas <erdemaktas@google.com>
Sent: Friday, April 16, 2021 3:43 AM
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: devel@edk2.groups.io; jejb@linux.ibm.com; Yao, Jiewen
<jiewen.yao@intel.com>; dgilbert@redhat.com; Laszlo Ersek
<lersek@redhat.com>; Xu, Min M <min.m.xu@intel.com>;
thomas.lendacky@amd.com; Brijesh Singh <brijesh.singh@amd.com>; Justen,
Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Nathaniel McCallum
<npmccallum@redhat.com>; Ning Yang <ningyang@google.com>
Subject: Re: [edk2-devel] separate OVMF binary for TDX? [was: OvmfPkg:
Reserve the Secrets and Cpuid page for the SEV-SNP guest]

Thanks Paolo.

On Thu, Apr 15, 2021 at 12:59 AM Paolo Bonzini <pbonzini@redhat.com>
wrote:

On 15/04/21 01:34, Erdem Aktas wrote:
We do not want to generate different binaries for AMD, Intel, Intel
with TDX, AMD with SEV/SNP etc
My question is why the user would want a single binary for VMs with and
without TDX/SNP. I know there is attestation, but why would you even
want the _possibility_ that your guest starts running without TDX or
SNP
protection, and only find out later via attestation?
There might be multiple reasons why customers want it but we need this
requirement for a couple of other reasons too.

We do not only have hardware based confidential VMs. We might have
some other solutions which measure the initial image before boot.
Ultimately we might want to use a common attestation interface where
customers might be running different kinds of VMs. Using a single
binary will make it easier to manage/verify measurements for both of
us and the customers. I am not a PM so I cannot give more context on
customer use cases.

Another reason is how we deploy and manage guest firmware. We have a
lot of optimization and customization to speed up firmware loading
time and also reduce the time to deploy new builds on the whole fleet
uniformly. Adding a new firmware binary is a big challenge for us to
enable these features. On the top of integration challenges, it will
create maintainability issues in the long run for us when we provide
tools to verify/reproduce the hashes in the attestation report.

want the _possibility_ that your guest starts running without TDX or
SNP
protection, and only find out later via attestation?
I am missing the point here. Customers should rely on only the
attestation report to establish the trust.
-If firmware does not support TDX and TDX is enabled, that firmware
will crash at some point.
-If firmware is generic firmware that supports TDX and SNP and others,
and TDX is enabled or not, still the customer needs to verify the TDX
enablement through attestation.
-If firmware is a customized binary compiled to support TDX,
irrelevant of TDX being enabled or not, still the customer needs to
verify the TDX enablement through attestation.


For a similar reason, OVMF already supports shipping a binary that
fails
to boot if SMM is not available to the firmware, because then secure
boot would be trivially circumvented.

I can understand having a single binary for both TDX or SNP. That's
not
a problem since you can set up the SEV startup VMSA to 32-bit protected
mode just like TDX wants.
I agree that this is doable but I am not sure if we need to also
modify the reset vector for AMD SNP in that case. Also it will not
solve our problem. If we start to generate a new firmware for every
feature , it will not end well for us, I think. Both TDX and SNP are
still new features in the same architecture, and it seems to me that
they are sharing a lot of common/similar code. AMD has already made
some of their patches in (SEV and SEV-ES) which works very nicely for
our use case and integration. Looks like Intel just has an issue on
how to fix their reset vector problem. Once they solve it and upstream
accepts the changes, I do not see any other big blocker. OVMF was
doing a great job on abstracting differences and providing a common
interface without creating multiple binaries. I do not see why it
should not do the same thing here.

therefore we were expecting the TDX
changes to be part of the upstream code.
Having 1 or more binaries should be unrelated to the changes being
upstream (or more likely, I am misunderstanding you).
You are right, it is my bad for not clarifying it. What I mean is we
want it to be part of the upstream so it can be easier for us to pull
the changes and they are compatible with the changes that SNP is doing
but we also do not want to use different configuration files to
generate different binaries for each use case.


Thanks,

Paolo






Re: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS verify support

Agrawal, Sachin
 

Hi Jiewen,

Thanks for sharing these references.

We are currently using Salt Length of digest length.
I will add the test for new API in the unit test framework in the next version of the patch.

In reference to adding support for RsaPssSign() API : This maybe due to my ignorance, but I am unaware of usages where BIOS is involved in doing asymmetric signing during run time. I do see that CryptoPkg also contains TLS interface and that would involve asymmetric signing, but that will directly use the OpenSSL's TLS interface for signing. And, therefore I was skeptical about adding RsaPssSign interface.

Thanks
Sachin

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Tuesday, April 20, 2021 6:29 PM
To: Agrawal, Sachin <sachin.agrawal@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS verify support

HI Sachin
Sorry, I forget to add link for the reference.

1) TPM2 Library Specification, part 2 structure (https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p64_Part2_Structures_15may2021.pdf) describes the PSS salt length.

For the TPM_ALG_RSAPSS signing scheme, ...
.... The salt size is
always the largest salt value that will fit into the available space.


2) NIST FIPS 186-5 draft (https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5-draft.pdf) and NIST FIPS 186-4 (https://doi.org/10.6028/NIST.FIPS.186-4) says:

For RSASSA-PSS,
the length (in bytes) of the salt (sLen) shall satisfy 0 ≤ sLen ≤ hLen

3) TCG FIPS 140-2 Guidance for TPM2 (https://trustedcomputinggroup.org/resource/tcg-fips-140-2-guidance-for-tpm-2-0/) mentions:

Language in [1] Part 1 Appendix B.7 RSASSA_PSS indicates:
"For both restricted and unrestricted signing keys, the random salt length will be the largest
size allowed by the key size and message digest size.
NOTE If the TPM implementation is required to be compliant with FIPS 186-4, then the random
salt length will be the largest size allowed by that specification."

4) TLS1.3 - RFC8446 (https://datatracker.ietf.org/doc/rfc8446/) has below.

RSASSA-PSS PSS algorithms:
The length of the Salt MUST be equal to the length of the digest
algorithm.


My view is that, TLS 1.3 and TPM FIPS mode require salt length == hash length, explicitly.

May I know that in your production, which salt length you choose in signing?
If you also choose salt length == hash length, then I would recommend make the default behavior to be HASH_LEN instead of AUTO.

Also, may I recommend we add RsaPssSign API as well?

Please also add the new API to the crypto test unit test.


I notice that crypto implementation (such as openssl, mbedtls) has API to let caller indicate what is the expected salt length. The caller may want AUTO or MAX in their special environment. I am OK to add another API later (such as RsaPssVerifyEx) to satisfy that need, if there is real use case.




-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Tuesday, April 20, 2021 11:20 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Hi Jiewen,

I reviewed RFC 8017 and I could not find any specific
'recommendations' on salt length to be used during signing with PSS encoding scheme.
However, in Section D.5.2.2.1(Notes 2) of IEEE 1363a-2004, it is
recommended to use salt length atleast equal to the hash digest length.

We can modify the current API to take a additional parameter as salt
length and ONLY pursue verification operation if Salt length is
atleast equal to digest length.
This will act as a hardening mechanism for Edk2 as it will accept
signatures only with 'appropriate' salt lengths.

Let me know if this is fine and I will push a corresponding patch.

Thx
Sachin


-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Tuesday, April 20, 2021 2:12 AM
To: Agrawal, Sachin <sachin.agrawal@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Right. That has PROs and CONs.

On one hand, that allows maximum compatibility, salt could be
HASH_SIZE or MAX, or even 0 ?

On the other hand, what if the consumer only wants to accept a
specific length? E.g. TPM in FIPS mode and TLS requires SaltLength==HashLength.

Thank you
Yao Jiewen


-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Tuesday, April 20, 2021 3:19 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Hi Jiewen,

From Section 9.1 in RFC 8017:
" Note that the verification operation follows reverse steps to recover
salt and then forward steps to recompute and compare H."

Therefore, salt length can be inferred from the PSS block structure
during verification operation.

I opted for 'RSA_PSS_SALTLEN_AUTO' as it will allow Edk2 to verify
PSS signatures of any salt lengths.

Thanks
Sachin

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Monday, April 19, 2021 7:30 PM
To: Agrawal, Sachin <sachin.agrawal@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
Subject: RE: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

Hi Sachin
May I know why you hardcode PSS salt length to be
RSA_PSS_SALTLEN_AUTO ?

Thank you
Yao Jiewen


-----Original Message-----
From: Agrawal, Sachin <sachin.agrawal@intel.com>
Sent: Tuesday, April 20, 2021 10:02 AM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>;
Jiang, Guomin <guomin.jiang@intel.com>; Agrawal, Sachin
<sachin.agrawal@intel.com>
Subject: [PATCH v1 1/1] CryptoPkg: BaseCryptLib: Add RSA PSS
verify support

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3314

This patch uses Openssl's EVP API's to perform RSASSA-PSS
verification of a binary blob.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>

Signed-off-by: Sachin Agrawal <sachin.agrawal@intel.com>
---
CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c | 139
++++++++++++++++++++
CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c | 43 ++++++
CryptoPkg/Include/Library/BaseCryptLib.h | 27 ++++
CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 1 +
7 files changed, 213 insertions(+)

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
new file mode 100644
index 000000000000..acf5eb689cd8
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
@@ -0,0 +1,139 @@
+/** @file
+ RSA Asymmetric Cipher Wrapper Implementation over OpenSSL.
+
+ This file implements following APIs which provide basic
+ capabilities for
RSA:
+ 1) RsaPssVerify
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+#include <openssl/bn.h>
+#include <openssl/rsa.h>
+#include <openssl/objects.h>
+#include <openssl/evp.h>
+
+
+/**
+ Retrieve a pointer to EVP message digest object.
+
+ @param[in] DigestLen Length of the message digest.
+
+**/
+static
+EVP_MD*
+GetEvpMD (
+ IN UINT16 DigestLen
+ )
+{
+ switch (DigestLen){
+ case SHA256_DIGEST_SIZE:
+ return EVP_sha256();
+ break;
+ case SHA384_DIGEST_SIZE:
+ return EVP_sha384();
+ break;
+ case SHA512_DIGEST_SIZE:
+ return EVP_sha512();
+ break;
+ default:
+ return NULL;
+ }
+}
+
+
+/**
+ Verifies the RSA signature with RSASSA-PSS signature scheme
+defined in RFC
8017.
+ Implementation determines salt length automatically from the
+ signature
encoding.
+ Mask generation function is the same as the message digest algorithm.
+
+ @param[in] RsaContext Pointer to RSA context for signature
verification.
+ @param[in] Message Pointer to octet message to be verified.
+ @param[in] MsgSize Size of the message in bytes.
+ @param[in] Signature Pointer to RSASSA-PSS signature to be verified.
+ @param[in] SigSize Size of signature in bytes.
+ @param[in] DigestLen Length of digest for RSA operation.
+
+ @retval TRUE Valid signature encoded in RSASSA-PSS.
+ @retval FALSE Invalid signature or invalid RSA context.
+
+**/
+BOOLEAN
+EFIAPI
+RsaPssVerify (
+ IN VOID *RsaContext,
+ IN CONST UINT8 *Message,
+ IN UINTN MsgSize,
+ IN CONST UINT8 *Signature,
+ IN UINTN SigSize,
+ IN UINT16 DigestLen
+ )
+{
+ BOOLEAN Result;
+ EVP_PKEY *pEvpRsaKey = NULL;
+ EVP_MD_CTX *pEvpVerifyCtx = NULL;
+ EVP_PKEY_CTX *pKeyCtx = NULL;
+ CONST EVP_MD *HashAlg = NULL;
+
+ if (RsaContext == NULL) {
+ return FALSE;
+ }
+ if (Message == NULL || MsgSize == 0 || MsgSize > INT_MAX) {
+ return FALSE;
+ }
+ if (Signature == NULL || SigSize == 0 || SigSize > INT_MAX) {
+ return FALSE;
+ }
+
+ HashAlg = GetEvpMD(DigestLen);
+
+ if (HashAlg == NULL) {
+ return FALSE;
+ }
+
+ pEvpRsaKey = EVP_PKEY_new();
+ if (pEvpRsaKey == NULL) {
+ goto _Exit;
+ }
+
+ EVP_PKEY_set1_RSA(pEvpRsaKey, RsaContext);
+
+ pEvpVerifyCtx = EVP_MD_CTX_create(); if (pEvpVerifyCtx == NULL) {
+ goto _Exit;
+ }
+
+ Result = EVP_DigestVerifyInit(pEvpVerifyCtx, &pKeyCtx, HashAlg,
+ NULL,
pEvpRsaKey) > 0;
+ if (pKeyCtx == NULL) {
+ goto _Exit;
+ }
+
+ if (Result) {
+ Result = EVP_PKEY_CTX_set_rsa_padding(pKeyCtx,
RSA_PKCS1_PSS_PADDING) > 0;
+ }
+ if (Result) {
+ Result = EVP_PKEY_CTX_set_rsa_pss_saltlen(pKeyCtx,
RSA_PSS_SALTLEN_AUTO) > 0;
+ }
+ if (Result) {
+ Result = EVP_PKEY_CTX_set_rsa_mgf1_md(pKeyCtx, HashAlg) > 0;
+ } if (Result) {
+ Result = EVP_DigestVerifyUpdate(pEvpVerifyCtx, Message,
(UINT32)MsgSize) > 0;
+ }
+ if (Result) {
+ Result = EVP_DigestVerifyFinal(pEvpVerifyCtx, Signature,
+ (UINT32)SigSize) > 0; }
+
+_Exit :
+ if (pEvpRsaKey) {
+ EVP_PKEY_free(pEvpRsaKey);
+ }
+ if (pEvpVerifyCtx) {
+ EVP_MD_CTX_destroy(pEvpVerifyCtx);
+ }
+
+ return Result;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c
new file mode 100644
index 000000000000..8d84b4c1426c
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssNull.c
@@ -0,0 +1,43 @@
+/** @file
+ RSA-PSS Asymmetric Cipher Wrapper Implementation over OpenSSL.
+
+ This file does not provide real capabilities for following APIs
+ in RSA
handling:
+ 1) RsaPssVerify
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Verifies the RSA signature with RSASSA-PSS signature scheme
+defined in RFC
8017.
+ Implementation determines salt length automatically from the
+ signature
encoding.
+ Mask generation function is the same as the message digest algorithm.
+
+ @param[in] RsaContext Pointer to RSA context for signature
verification.
+ @param[in] Message Pointer to octet message to be verified.
+ @param[in] MsgSize Size of the message in bytes.
+ @param[in] Signature Pointer to RSASSA-PSS signature to be verified.
+ @param[in] SigSize Size of signature in bytes.
+ @param[in] DigestLen Length of digest for RSA operation.
+
+ @retval TRUE Valid signature encoded in RSASSA-PSS.
+ @retval FALSE Invalid signature or invalid RSA context.
+
+**/
+BOOLEAN
+EFIAPI
+RsaPssVerify (
+ IN VOID *RsaContext,
+ IN CONST UINT8 *Message,
+ IN UINTN MsgSize,
+ IN CONST UINT8 *Signature,
+ IN UINTN SigSize,
+ IN UINT16 DigestLen
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
b/CryptoPkg/Include/Library/BaseCryptLib.h
index 496121e6a4ed..36d560b8d691 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1363,6 +1363,33 @@ RsaPkcs1Verify (
IN UINTN SigSize
);

+/**
+ Verifies the RSA signature with RSASSA-PSS signature scheme
+defined in RFC
8017.
+ Implementation determines salt length automatically from the
+ signature
encoding.
+ Mask generation function is the same as the message digest algorithm.
+
+ @param[in] RsaContext Pointer to RSA context for signature
verification.
+ @param[in] Message Pointer to octet message to be verified.
+ @param[in] MsgSize Size of the message in bytes.
+ @param[in] Signature Pointer to RSASSA-PSS signature to be verified.
+ @param[in] SigSize Size of signature in bytes.
+ @param[in] DigestLen Length of digest for RSA operation.
+
+ @retval TRUE Valid signature encoded in RSASSA-PSS.
+ @retval FALSE Invalid signature or invalid RSA context.
+
+**/
+BOOLEAN
+EFIAPI
+RsaPssVerify (
+ IN VOID *RsaContext,
+ IN CONST UINT8 *Message,
+ IN UINTN MsgSize,
+ IN CONST UINT8 *Signature,
+ IN UINTN SigSize,
+ IN UINT16 DigestLen
+ );
+
/**
Retrieve the RSA Private Key from the password-protected PEM key data.


Re: [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

Lendacky, Thomas
 

On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
On 04/21/21 00:54, Lendacky, Thomas wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C19a7d97e2a7b461830ed08d905528472%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546674232278910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&amp;reserved=0

Enabling TPM support results in guest termination of an SEV-ES guest
because it uses MMIO opcodes that are not currently supported.

Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
use a memory offset directly encoded in the instruction. Also, add a DEBUG
statement to identify an unsupported MMIO opcode being used.

Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 99 +++++++++++++++++++
1 file changed, 99 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 273f36499988..f9660b757d8e 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -678,6 +678,7 @@ MmioExit (
UINTN Bytes;
UINT64 *Register;
UINT8 OpCode, SignByte;
+ UINTN Address;

Bytes = 0;

@@ -727,6 +728,51 @@ MmioExit (
}
break;

+ //
+ // MMIO write (MOV moffsetX, aX)
+ //
+ case 0xA2:
+ Bytes = 1;
+ //
+ // fall through
+ //
+ case 0xA3:
+ Bytes = ((Bytes != 0) ? Bytes :
+ (InstructionData->DataSize == Size16Bits) ? 2 :
+ (InstructionData->DataSize == Size32Bits) ? 4 :
+ (InstructionData->DataSize == Size64Bits) ? 8 :
+ 0);
+
+ InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+ InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
+
+ if (InstructionData->AddrSize == Size8Bits) {
+ Address = *(UINT8 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size16Bits) {
+ Address = *(UINT16 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size32Bits) {
+ Address = *(UINT32 *) InstructionData->Immediate;
+ } else {
+ Address = *(UINTN *) InstructionData->Immediate;
+ }
(1) Can we simplify this as follows?

InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
InstructionData->End += InstructionData->ImmediateSize;
Address = 0;
CopyMem (&Address, InstructionData->Immediate,
InstructionData->ImmediateSize);
Yup, that can be done.


+
+ Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+ if (Status != 0) {
+ return Status;
+ }
+
+ ExitInfo1 = Address;
+ ExitInfo2 = Bytes;
+ CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
+ if (Status != 0) {
+ return Status;
+ }
+ break;
+
//
// MMIO write (MOV reg/memX, immX)
//
@@ -809,6 +855,58 @@ MmioExit (
CopyMem (Register, Ghcb->SharedBuffer, Bytes);
break;

+ //
+ // MMIO read (MOV aX, moffsetX)
+ //
+ case 0xA0:
+ Bytes = 1;
+ //
+ // fall through
+ //
+ case 0xA1:
+ Bytes = ((Bytes != 0) ? Bytes :
+ (InstructionData->DataSize == Size16Bits) ? 2 :
+ (InstructionData->DataSize == Size32Bits) ? 4 :
+ (InstructionData->DataSize == Size64Bits) ? 8 :
+ 0);
+
+ InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+ InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
+
+ if (InstructionData->AddrSize == Size8Bits) {
+ Address = *(UINT8 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size16Bits) {
+ Address = *(UINT16 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size32Bits) {
+ Address = *(UINT32 *) InstructionData->Immediate;
+ } else {
+ Address = *(UINTN *) InstructionData->Immediate;
+ }
(2) Similar question as (1).
Will do.


+
+ Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+ if (Status != 0) {
+ return Status;
+ }
+
+ ExitInfo1 = Address;
+ ExitInfo2 = Bytes;
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
+ if (Status != 0) {
+ return Status;
+ }
+
+ if (Bytes == 4) {
+ //
+ // Zero-extend for 32-bit operation
+ //
+ Regs->Rax = 0;
+ }
(3) This is also seen with opcode 0x8B, but can you remind me please why
we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
32-bit operand size the 32-bit register results are zero-extended to 64-bits.


+ CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
+ break;
+
//
// MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
//
@@ -886,6 +984,7 @@ MmioExit (
break;

default:
+ DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
Status = GP_EXCEPTION;
ASSERT (FALSE);
}
(4) We should use the DEBUG_ERROR log mask here.
Will change.

Thanks,
Tom


Thanks
Laszlo






Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

Sami Mujawar
 

Hi Jianyong,

 

You need to check if you're subscribed to the EDK II development mailing list. Otherwise, your patch email will get rejected. You can subscribe here: https://edk2.groups.io/g/devel.

Make sure that you reply to the email with subscription confirmation sent from noreply@groups.io. Unless you do it, you won't become a member of the mailing list.

I would also recommend that you wait for a day after confirming, as I believe the edk2.groups.io admin will need to approve your membership.

 

Regards,

 

Sami Mujawar

 

From: Laszlo Ersek <lersek@...>
Date: Thursday, 22 April 2021 at 14:57
To: Jianyong Wu <Jianyong.Wu@...>, edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Justin He <Justin.He@...>, Ard Biesheuvel <ardb+tianocore@...>, Leif Lindholm <leif@...>, Sami Mujawar <Sami.Mujawar@...>
Subject: Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

Hi Jianyong,

On 04/22/21 10:24, Jianyong Wu wrote:
> Cloud Hypervisor is kvm based VMM implemented in rust.
>
> This library populates the system memory map for the
> Cloud Hypervisor virtual platform.
>
> Cc: Laszlo Ersek <lersek@...>
> Cc: Ard Biesheuvel <ardb+tianocore@...>
> Cc: Leif Lindholm <leif@...>
> Signed-off-by: Jianyong Wu <jianyong.wu@...>
> ---
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf          |  47 +++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c               |  94 ++++++++++++++++++
>  ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c | 100 ++++++++++++++++++++
>  3 files changed, 241 insertions(+)

let's sort out the meta-problems first:

(1) you need a Feature Request BZ for this;
<https://bugzilla.tianocore.org/>. The commit messages should reference
the specific bugzilla ticket URL.

(2) "Clh" is a catastrophically bad abbreviation. The whole point of
your work is to add Cloud Hypervisor support, so why trash the most
relevant information in the file names with an inane abbreviation?

(Not to mention that the name "Cloud Hypervisor" itself is as
nondescript as possible. :/)

(3) I have not received a cover letter (0/4). Not sure if you sent one.

(4) I don't see the messages in my edk2-devel folder, or in the mailing
list archives, or in the messages held for moderation at the groups.io
WebUI.

(5) "Cloud Hypervisor" is not something that I can justifiably spend
much time on. I'm willing to review this series at the level at which
I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing
on style and potential regressions. However, that's not enough for the
long term: someone from ARM (or elsewhere) will have to step up for
permanent reviewership. Please add a patch for extending
"Maintainers.txt" appropriately. Example subsystems:

- ArmVirtPkg: modules used on Xen
- ArmVirtPkg: Kvmtool emulated platform support
- OvmfPkg: bhyve-related modules
- OvmfPkg: Xen-related modules

Please keep the subsystem titles alphabetically sorted in the file.

Please resend.

(I'm posting these comments at once because they are understandable to
the community even in the absence of your patches on the list.)

Thanks
Laszlo

>
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> new file mode 100644
> index 000000000000..04cb1f2a581a
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
> @@ -0,0 +1,47 @@
> +#/* @file
> +#
> +#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#*/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = ClhVirtMemInfoPeiLib
> +  FILE_GUID                      = 3E29D940-0591-EE6A-CAD4-223A9CF55E75
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmVirtMemInfoLib|PEIM
> +  CONSTRUCTOR                    = ClhVirtMemInfoPeiLibConstructor
> +
> +[Sources]
> +  ClhVirtMemInfoLib.c
> +  ClhVirtMemInfoPeiLibConstructor.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  BaseMemoryLib
> +  DebugLib
> +  FdtLib
> +  PcdLib
> +  MemoryAllocationLib
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdFvBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFdSize
> +  gArmTokenSpaceGuid.PcdFvSize
> +  gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> new file mode 100644
> index 000000000000..829d7d7aa259
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
> @@ -0,0 +1,94 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/ArmLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +
> +// Number of Virtual Memory Map Descriptors
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          5
> +
> +//
> +// mach-virt's core peripherals such as the UART, the GIC and the RTC are
> +// all mapped in the 'miscellaneous device I/O' region, which we just map
> +// in its entirety rather than device by device. Note that it does not
> +// cover any of the NOR flash banks or PCI resource windows.
> +//
> +#define MACH_VIRT_PERIPH_BASE       0x08000000
> +#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB
> +
> +//
> +// in cloud-hypervisor, 0x0 ~ 0x8000000 is reserved as normal memory for UEFI
> +//
> +#define CLH_UEFI_MEM_BASE       0x0
> +#define CLH_UEFI_MEM_SIZE       0x08000000
> +
> +/**
> +  Return the Virtual Memory Map of your platform
> +
> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
> +  on your platform.
> +
> +  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR
> +                                    describing a Physical-to-Virtual Memory
> +                                    mapping. This array must be ended by a
> +                                    zero-filled entry. The allocated memory
> +                                    will not be freed.
> +
> +**/
> +VOID
> +ArmVirtGetMemoryMap (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
> +  )
> +{
> +  ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> +
> +  ASSERT (VirtualMemoryMap != NULL);
> +
> +  VirtualMemoryTable = AllocatePool (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
> +                                     MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
> +
> +  if (VirtualMemoryTable == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePool()\n", __FUNCTION__));
> +    return;
> +  }
> +
> +  // System DRAM
> +  VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
> +  VirtualMemoryTable[0].VirtualBase  = VirtualMemoryTable[0].PhysicalBase;
> +  VirtualMemoryTable[0].Length       = PcdGet64 (PcdSystemMemorySize);
> +  VirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  DEBUG ((DEBUG_INFO, "%a: Dumping System DRAM Memory Map:\n"
> +      "\tPhysicalBase: 0x%lX\n"
> +      "\tVirtualBase: 0x%lX\n"
> +      "\tLength: 0x%lX\n",
> +      __FUNCTION__,
> +      VirtualMemoryTable[0].PhysicalBase,
> +      VirtualMemoryTable[0].VirtualBase,
> +      VirtualMemoryTable[0].Length));
> +
> +  // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
> +  VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].VirtualBase  = MACH_VIRT_PERIPH_BASE;
> +  VirtualMemoryTable[1].Length       = MACH_VIRT_PERIPH_SIZE;
> +  VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // Map the FV region as normal executable memory
> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);
> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  // End of Table
> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> +
> +  *VirtualMemoryMap = VirtualMemoryTable;
> +}
> diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> new file mode 100644
> index 000000000000..5f89b70df990
> --- /dev/null
> +++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
> @@ -0,0 +1,100 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <libfdt.h>
> +
> +RETURN_STATUS
> +EFIAPI
> +ClhVirtMemInfoPeiLibConstructor (
> +  VOID
> +  )
> +{
> +  VOID          *DeviceTreeBase;
> +  INT32         Node, Prev;
> +  UINT64        NewBase, CurBase;
> +  UINT64        NewSize, CurSize;
> +  CONST CHAR8   *Type;
> +  INT32         Len;
> +  CONST UINT64  *RegProp;
> +  RETURN_STATUS PcdStatus;
> +
> +  NewBase = 0;
> +  NewSize = 0;
> +
> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> +  ASSERT (DeviceTreeBase != NULL);
> +
> +  //
> +  // Make sure we have a valid device tree blob
> +  //
> +  ASSERT (fdt_check_header (DeviceTreeBase) == 0);
> +
> +  //
> +  // Look for the lowest memory node
> +  //
> +  for (Prev = 0;; Prev = Node) {
> +    Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
> +    if (Node < 0) {
> +      break;
> +    }
> +
> +    //
> +    // Check for memory node
> +    //
> +    Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
> +    if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
> +      //
> +      // Get the 'reg' property of this node. For now, we will assume
> +      // two 8 byte quantities for base and size, respectively.
> +      //
> +      RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> +      if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
> +
> +        CurBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
> +        CurSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));
> +
> +        DEBUG ((DEBUG_INFO, "%a: System RAM @ 0x%lx - 0x%lx\n",
> +          __FUNCTION__, CurBase, CurBase + CurSize - 1));
> +
> +        if (NewBase > CurBase || NewBase == 0) {
> +          NewBase = CurBase;
> +          NewSize = CurSize;
> +        }
> +      } else {
> +        DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",
> +          __FUNCTION__));
> +      }
> +    }
> +  }
> +
> +  //
> +  // Make sure the start of DRAM matches our expectation
> +  //
> +  ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
> +  PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  //
> +  // We need to make sure that the machine we are running on has at least
> +  // 128 MB of memory configured, and is currently executing this binary from
> +  // NOR flash. This prevents a device tree image in DRAM from getting
> +  // clobbered when our caller installs permanent PEI RAM, before we have a
> +  // chance of marking its location as reserved or copy it to a freshly
> +  // allocated block in the permanent PEI RAM in the platform PEIM.
> +  //
> +  ASSERT (NewSize >= SIZE_128MB);
> +  ASSERT (
> +    (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> +      (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
> +    ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
> +
> +  return RETURN_SUCCESS;
> +}
>


Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor

Laszlo Ersek
 

Hi Jianyong,

On 04/22/21 10:24, Jianyong Wu wrote:
Cloud Hypervisor is kvm based VMM implemented in rust.

This library populates the system memory map for the
Cloud Hypervisor virtual platform.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Leif Lindholm <leif@nuviainc.com>
Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf | 47 +++++++++
ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c | 94 ++++++++++++++++++
ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c | 100 ++++++++++++++++++++
3 files changed, 241 insertions(+)
let's sort out the meta-problems first:

(1) you need a Feature Request BZ for this;
<https://bugzilla.tianocore.org/>. The commit messages should reference
the specific bugzilla ticket URL.

(2) "Clh" is a catastrophically bad abbreviation. The whole point of
your work is to add Cloud Hypervisor support, so why trash the most
relevant information in the file names with an inane abbreviation?

(Not to mention that the name "Cloud Hypervisor" itself is as
nondescript as possible. :/)

(3) I have not received a cover letter (0/4). Not sure if you sent one.

(4) I don't see the messages in my edk2-devel folder, or in the mailing
list archives, or in the messages held for moderation at the groups.io
WebUI.

(5) "Cloud Hypervisor" is not something that I can justifiably spend
much time on. I'm willing to review this series at the level at which
I've reviewed (for example) XenPVH or Bhyve in the past, mainly focusing
on style and potential regressions. However, that's not enough for the
long term: someone from ARM (or elsewhere) will have to step up for
permanent reviewership. Please add a patch for extending
"Maintainers.txt" appropriately. Example subsystems:

- ArmVirtPkg: modules used on Xen
- ArmVirtPkg: Kvmtool emulated platform support
- OvmfPkg: bhyve-related modules
- OvmfPkg: Xen-related modules

Please keep the subsystem titles alphabetically sorted in the file.

Please resend.

(I'm posting these comments at once because they are understandable to
the community even in the absence of your patches on the list.)

Thanks
Laszlo


diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
new file mode 100644
index 000000000000..04cb1f2a581a
--- /dev/null
+++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLib.inf
@@ -0,0 +1,47 @@
+#/* @file
+#
+# Copyright (c) 2011-2015, ARM Limited. All rights reserved.
+# Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#*/
+
+[Defines]
+ INF_VERSION = 0x0001001A
+ BASE_NAME = ClhVirtMemInfoPeiLib
+ FILE_GUID = 3E29D940-0591-EE6A-CAD4-223A9CF55E75
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmVirtMemInfoLib|PEIM
+ CONSTRUCTOR = ClhVirtMemInfoPeiLibConstructor
+
+[Sources]
+ ClhVirtMemInfoLib.c
+ ClhVirtMemInfoPeiLibConstructor.c
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ ArmLib
+ BaseMemoryLib
+ DebugLib
+ FdtLib
+ PcdLib
+ MemoryAllocationLib
+
+[Pcd]
+ gArmTokenSpaceGuid.PcdFdBaseAddress
+ gArmTokenSpaceGuid.PcdFvBaseAddress
+ gArmTokenSpaceGuid.PcdSystemMemoryBase
+ gArmTokenSpaceGuid.PcdSystemMemorySize
+
+[FixedPcd]
+ gArmTokenSpaceGuid.PcdFdSize
+ gArmTokenSpaceGuid.PcdFvSize
+ gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
new file mode 100644
index 000000000000..829d7d7aa259
--- /dev/null
+++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoLib.c
@@ -0,0 +1,94 @@
+/** @file
+
+ Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <Library/ArmLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+
+// Number of Virtual Memory Map Descriptors
+#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 5
+
+//
+// mach-virt's core peripherals such as the UART, the GIC and the RTC are
+// all mapped in the 'miscellaneous device I/O' region, which we just map
+// in its entirety rather than device by device. Note that it does not
+// cover any of the NOR flash banks or PCI resource windows.
+//
+#define MACH_VIRT_PERIPH_BASE 0x08000000
+#define MACH_VIRT_PERIPH_SIZE SIZE_128MB
+
+//
+// in cloud-hypervisor, 0x0 ~ 0x8000000 is reserved as normal memory for UEFI
+//
+#define CLH_UEFI_MEM_BASE 0x0
+#define CLH_UEFI_MEM_SIZE 0x08000000
+
+/**
+ Return the Virtual Memory Map of your platform
+
+ This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
+ on your platform.
+
+ @param[out] VirtualMemoryMap Array of ARM_MEMORY_REGION_DESCRIPTOR
+ describing a Physical-to-Virtual Memory
+ mapping. This array must be ended by a
+ zero-filled entry. The allocated memory
+ will not be freed.
+
+**/
+VOID
+ArmVirtGetMemoryMap (
+ OUT ARM_MEMORY_REGION_DESCRIPTOR **VirtualMemoryMap
+ )
+{
+ ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
+
+ ASSERT (VirtualMemoryMap != NULL);
+
+ VirtualMemoryTable = AllocatePool (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
+ MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
+
+ if (VirtualMemoryTable == NULL) {
+ DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePool()\n", __FUNCTION__));
+ return;
+ }
+
+ // System DRAM
+ VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
+ VirtualMemoryTable[0].VirtualBase = VirtualMemoryTable[0].PhysicalBase;
+ VirtualMemoryTable[0].Length = PcdGet64 (PcdSystemMemorySize);
+ VirtualMemoryTable[0].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+
+ DEBUG ((DEBUG_INFO, "%a: Dumping System DRAM Memory Map:\n"
+ "\tPhysicalBase: 0x%lX\n"
+ "\tVirtualBase: 0x%lX\n"
+ "\tLength: 0x%lX\n",
+ __FUNCTION__,
+ VirtualMemoryTable[0].PhysicalBase,
+ VirtualMemoryTable[0].VirtualBase,
+ VirtualMemoryTable[0].Length));
+
+ // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
+ VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;
+ VirtualMemoryTable[1].VirtualBase = MACH_VIRT_PERIPH_BASE;
+ VirtualMemoryTable[1].Length = MACH_VIRT_PERIPH_SIZE;
+ VirtualMemoryTable[1].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+
+ // Map the FV region as normal executable memory
+ VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
+ VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase;
+ VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFvSize);
+ VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+
+ // End of Table
+ ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
+
+ *VirtualMemoryMap = VirtualMemoryTable;
+}
diff --git a/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
new file mode 100644
index 000000000000..5f89b70df990
--- /dev/null
+++ b/ArmVirtPkg/Library/ClhVirtMemInfoLib/ClhVirtMemInfoPeiLibConstructor.c
@@ -0,0 +1,100 @@
+/** @file
+
+ Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <libfdt.h>
+
+RETURN_STATUS
+EFIAPI
+ClhVirtMemInfoPeiLibConstructor (
+ VOID
+ )
+{
+ VOID *DeviceTreeBase;
+ INT32 Node, Prev;
+ UINT64 NewBase, CurBase;
+ UINT64 NewSize, CurSize;
+ CONST CHAR8 *Type;
+ INT32 Len;
+ CONST UINT64 *RegProp;
+ RETURN_STATUS PcdStatus;
+
+ NewBase = 0;
+ NewSize = 0;
+
+ DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
+ ASSERT (DeviceTreeBase != NULL);
+
+ //
+ // Make sure we have a valid device tree blob
+ //
+ ASSERT (fdt_check_header (DeviceTreeBase) == 0);
+
+ //
+ // Look for the lowest memory node
+ //
+ for (Prev = 0;; Prev = Node) {
+ Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
+ if (Node < 0) {
+ break;
+ }
+
+ //
+ // Check for memory node
+ //
+ Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
+ if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
+ //
+ // Get the 'reg' property of this node. For now, we will assume
+ // two 8 byte quantities for base and size, respectively.
+ //
+ RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
+ if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
+
+ CurBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
+ CurSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));
+
+ DEBUG ((DEBUG_INFO, "%a: System RAM @ 0x%lx - 0x%lx\n",
+ __FUNCTION__, CurBase, CurBase + CurSize - 1));
+
+ if (NewBase > CurBase || NewBase == 0) {
+ NewBase = CurBase;
+ NewSize = CurSize;
+ }
+ } else {
+ DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",
+ __FUNCTION__));
+ }
+ }
+ }
+
+ //
+ // Make sure the start of DRAM matches our expectation
+ //
+ ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
+ PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
+ ASSERT_RETURN_ERROR (PcdStatus);
+
+ //
+ // We need to make sure that the machine we are running on has at least
+ // 128 MB of memory configured, and is currently executing this binary from
+ // NOR flash. This prevents a device tree image in DRAM from getting
+ // clobbered when our caller installs permanent PEI RAM, before we have a
+ // chance of marking its location as reserved or copy it to a freshly
+ // allocated block in the permanent PEI RAM in the platform PEIM.
+ //
+ ASSERT (NewSize >= SIZE_128MB);
+ ASSERT (
+ (((UINT64)PcdGet64 (PcdFdBaseAddress) +
+ (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
+ ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
+
+ return RETURN_SUCCESS;
+}


Re: [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes

Lendacky, Thomas
 

On 4/22/21 12:28 AM, Laszlo Ersek wrote:
On 04/21/21 00:54, Lendacky, Thomas wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C22bf3a3ae9cb4421e93208d9054f79c8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546661229697941%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=1EmUDf%2FfuCuu%2BkXPZijzatfliplMhKEQH8kiZ9Z8ZF0%3D&amp;reserved=0

The MOVZX and MOVSX instructions use the ModRM byte in the instruction,
but the instruction decoding support was not decoding it. This resulted
in invalid decoding and failing of the MMIO operation. Also, when
performing the zero-extend or sign-extend operation, the memory operation
should be using the size, and not the size enumeration value.

Add the ModRM byte decoding for the MOVZX and MOVSX opcodes and use the
true data size to perform the extend operations. Additionally, add a
DEBUG statement identifying the MMIO address being flagged as encrypted
during the MMIO address validation.

Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 24259060fd65..273f36499988 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -643,6 +643,7 @@ ValidateMmioMemory (
//
// Any state other than unencrypted is an error, issue a #GP.
//
+ DEBUG ((DEBUG_INFO, "MMIO using encrypted memory: %lx\n", MemoryAddress));
GpEvent.Uint64 = 0;
GpEvent.Elements.Vector = GP_EXCEPTION;
GpEvent.Elements.Type = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
(1) This can potentially generate a large number of debug messages;
please use the DEBUG_VERBOSE log mask.
Actually, you will see this only once since the code will propagate a GP
and the guest will terminate in this situation.


(2) "MemoryAddress" has type UINTN, but %lx takes UINT64. Given that
this is X64-only code, functionally there is no bug, but it's still
cleaner to pass "(UINT64)MemoryAddress" to %lx.
Will do.

Thanks,
Tom


With that:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


@@ -817,6 +818,7 @@ MmioExit (
// fall through
//
case 0xB7:
+ DecodeModRm (Regs, InstructionData);
Bytes = (Bytes != 0) ? Bytes : 2;

Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
@@ -835,7 +837,7 @@ MmioExit (
}

Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
- SetMem (Register, InstructionData->DataSize, 0);
+ SetMem (Register, (UINTN) (1 << InstructionData->DataSize), 0);
CopyMem (Register, Ghcb->SharedBuffer, Bytes);
break;

@@ -848,6 +850,7 @@ MmioExit (
// fall through
//
case 0xBF:
+ DecodeModRm (Regs, InstructionData);
Bytes = (Bytes != 0) ? Bytes : 2;

Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
@@ -878,7 +881,7 @@ MmioExit (
}

Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
- SetMem (Register, InstructionData->DataSize, SignByte);
+ SetMem (Register, (UINTN) (1 << InstructionData->DataSize), SignByte);
CopyMem (Register, Ghcb->SharedBuffer, Bytes);
break;


Re: [PATCH v1 12/12] AzurePipelines: Add support for ArmPlatformPkg

Sami Mujawar
 

Hi Pierre,

 

 Thank you for this patch.

 

Reviewed-by: Sami Mujawar <sami.mujawar@...>

 

Regards,

 

Sami Mujawar

 

 

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 12/12] AzurePipelines: Add support for ArmPlatformPkg

From: Pierre Gondois <Pierre.Gondois@...>

Add an entry to build the ArmPlatformPkg in the CI.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=3349
Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .azurepipelines/templates/pr-gate-build-job.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.azurepipelines/templates/pr-gate-build-job.yml b/.azurepipelines/templates/pr-gate-build-job.yml
index 837079e7bd97..16d197cde913 100644
--- a/.azurepipelines/templates/pr-gate-build-job.yml
+++ b/.azurepipelines/templates/pr-gate-build-job.yml
@@ -22,7 +22,7 @@ jobs:
   strategy:
     matrix:
       TARGET_ARM:
-        Build.Pkgs: 'ArmPkg'
+        Build.Pkgs: 'ArmPkg,ArmPlatformPkg'
         Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
       TARGET_MDE_CPU:
         Build.Pkgs: 'MdePkg,UefiCpuPkg'
--
2.17.1


Re: [PATCH v1 11/12] AzurePipelines: Add support for ArmPkg

Sami Mujawar
 

Hi Pierre,

 

Thank you for this patch.

 

Reviewed-by: Sami Mujawar <sami.mujawar@...>

 

Regards,

 

Sami Mujawar

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 11/12] AzurePipelines: Add support for ArmPkg

From: Pierre Gondois <Pierre.Gondois@...>

Add an entry to build the ArmPkg in the CI.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=3348
Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .azurepipelines/templates/pr-gate-build-job.yml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.azurepipelines/templates/pr-gate-build-job.yml b/.azurepipelines/templates/pr-gate-build-job.yml
index 3e6d275b1b9a..837079e7bd97 100644
--- a/.azurepipelines/templates/pr-gate-build-job.yml
+++ b/.azurepipelines/templates/pr-gate-build-job.yml
@@ -21,6 +21,9 @@ jobs:
   #Use matrix to speed up the build process
   strategy:
     matrix:
+      TARGET_ARM:
+        Build.Pkgs: 'ArmPkg'
+        Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
       TARGET_MDE_CPU:
         Build.Pkgs: 'MdePkg,UefiCpuPkg'
         Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
--
2.17.1


Re: [PATCH v1 10/12] .pytool: Document LicenseCheck and EccCheck

Sami Mujawar
 

Hi Pierre,

 

Thank you for this patch.

 

Reviewed-by: Sami Mujawar <sami.mujawar@...>

 

Regards,

 

Sami Mujawar

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 10/12] .pytool: Document LicenseCheck and EccCheck

From: Pierre Gondois <Pierre.Gondois@...>

Add an entry in the documentation for the LicenseCheck and
EccCheck plugins.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .pytool/Readme.md | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/.pytool/Readme.md b/.pytool/Readme.md
index eca86c6a822d..f6505507966a 100644
--- a/.pytool/Readme.md
+++ b/.pytool/Readme.md
@@ -254,6 +254,16 @@ Install
 
   More cspell info: https://github.com/streetsidesoftware/cspell
 
+### License Checking - LicenseCheck
+
+Scans all new added files in a package to make sure code is contributed under
+BSD-2-Clause-Patent.
+
+### Ecc tool - EccCheck
+
+Run the Ecc tool on the package. The Ecc tool is available in the BaseTools
+package. It checks that the code complies to the EDKII coding standard.
+
 ## PyTool Scopes
 
 Scopes are how the PyTool ext_dep, path_env, and plugins are activated.  Meaning
--
2.17.1


Re: [PATCH v1 09/12] .pytool: Enable CI for ArmPlatformPkg

Sami Mujawar
 

Hi Pierre,

 

This patch looks good to me.

 

Reviewed-by: Sami Mujawar <sami.mujawar@...>

 

Regards,

 

Sami Mujawar

 

 

 

From: Pierre.Gondois@... <Pierre.Gondois@...>
Date: Wednesday, 21 April 2021 at 13:21
To: devel@edk2.groups.io <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@...>, leif@... <leif@...>, ardb+tianocore@... <ardb+tianocore@...>, sean.brogan@... <sean.brogan@...>, Bret.Barkelew@... <Bret.Barkelew@...>
Subject: [PATCH v1 09/12] .pytool: Enable CI for ArmPlatformPkg

From: Pierre Gondois <Pierre.Gondois@...>

Enable the CI for the ArmPlatformPkg.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
 .pytool/CISettings.py | 1 +
 .pytool/Readme.md     | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/.pytool/CISettings.py b/.pytool/CISettings.py
index 6f7daeca076b..96e6baa5190d 100644
--- a/.pytool/CISettings.py
+++ b/.pytool/CISettings.py
@@ -50,6 +50,7 @@ class Settings(CiBuildSettingsManager, UpdateSettingsManager, SetupSettingsManag
         These should be edk2 workspace relative paths '''
 
         return ("ArmPkg",
+                "ArmPlatformPkg",
                 "ArmVirtPkg",
                 "DynamicTablesPkg",
                 "EmulatorPkg",
diff --git a/.pytool/Readme.md b/.pytool/Readme.md
index cbce1f6cd54a..eca86c6a822d 100644
--- a/.pytool/Readme.md
+++ b/.pytool/Readme.md
@@ -5,7 +5,7 @@
 | Package              | Windows VS2019 (IA32/X64)| Ubuntu GCC (IA32/X64/ARM/AARCH64) | Known Issues |
 | :----                | :-----                   | :----                             | :---         |
 | ArmPkg               |                    | :heavy_check_mark: |
-| ArmPlatformPkg       |
+| ArmPlatformPkg       |                    | :heavy_check_mark: |
 | ArmVirtPkg           | SEE PACKAGE README | SEE PACKAGE README |
 | CryptoPkg            | :heavy_check_mark: | :heavy_check_mark: | Spell checking in audit mode
 | DynamicTablesPkg     |                    | :heavy_check_mark: |
--
2.17.1

6481 - 6500 of 80786