Date   

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

Laszlo Ersek
 

Hi Jianyong,

On 04/22/21 15:56, Laszlo Ersek wrote:

(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. :/)
In an attempt to approach this constructively, I've given it more
thought. Does "CloudHv" sound acceptable to the community? I've seen
"hv" stand for "hypervisor" frequently.


I have another high-level note. I could delay it until after you post
v2, but I figure I could save you some time by sharing my observation
with you right now.

I think that the ACPI platform stuff, in patch#2, does not belong in
OvmfPkg/AcpiPlatformDxe. What's more, I don't think it belongs in
OvmfPkg, even.

The CloudHvAcpiPlatformDxe and CloudHvPlatformHasAcpiDtDxe drivers
should exist as stand-alone, self-contained drivers; they should be as
minimal as possible. This is already a given for
"CloudHvPlatformHasAcpiDtDxe", but it should also be possible for
"CloudHvAcpiPlatformDxe". OvmfPkg/AcpiPlatformDxe is a complex driver,
and the overlap between what OvmfPkg/AcpiPlatformDxe currently does, and
what CloudHvAcpiPlatformDxe actually *needs*, is virtually nil.

And so, the series shouldn't touch OvmfPkg at all.

Ultimately I suggest following the Xen pattern that can be seen under
ArmVirtPkg already. In detail, the following files and directories
should contain the new platform:

ArmVirtPkg/ArmVirtCloudHv.dsc
ArmVirtPkg/ArmVirtCloudHv.fdf
ArmVirtPkg/CloudHvAcpiPlatformDxe/
ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/
ArmVirtPkg/Library/CloudHvVirtMemInfoLib/

(And I don't really see the point of an FDF include file.)

Thanks!
Laszlo


[PATCH v2] BaseTools: Add support for version 3 of FMP Image Header structure

Sughosh Ganu
 

Add support for the ImageCapsuleSupport field, introduced in version 3
of the EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER structure. This
structure member is used to indicate if the corresponding payload has
support for authentication and dependency.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since v1:
- Reword the patch header to get rid of the PatchCheck warning
- Make passing of ImageCapsuleSupport parameter to the AddPayload
function as an optional parameter to maintain backward compatibility
- Declare the values of CAPSULE_SUPPORT_DEPENDENCY and
CAPSULE_SUPPORT_AUTHENTICATION in the FmpCapsuleHeaderClass and use
those in the GenerateCapsule script

.../Source/Python/Capsule/GenerateCapsule.py | 5 +++-
.../Common/Uefi/Capsule/FmpCapsuleHeader.py | 28 +++++++++++++------
2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/BaseTools/Source/Python/Capsule/GenerateCapsule.py b/BaseTools/Source/Python/Capsule/GenerateCapsule.py
index a8de988253..b8039db878 100644
--- a/BaseTools/Source/Python/Capsule/GenerateCapsule.py
+++ b/BaseTools/Source/Python/Capsule/GenerateCapsule.py
@@ -561,6 +561,7 @@ if __name__ == '__main__':
print ('GenerateCapsule: error:' + str(Msg))
sys.exit (1)
for SinglePayloadDescriptor in PayloadDescriptorList:
+ ImageCapsuleSupport = 0x0000000000000000
Result = SinglePayloadDescriptor.Payload
try:
FmpPayloadHeader.FwVersion = SinglePayloadDescriptor.FwVersion
@@ -575,6 +576,7 @@ if __name__ == '__main__':
if SinglePayloadDescriptor.UseDependency:
CapsuleDependency.Payload = Result
CapsuleDependency.DepexExp = SinglePayloadDescriptor.DepexExp
+ ImageCapsuleSupport |= FmpCapsuleHeader.CAPSULE_SUPPORT_DEPENDENCY
Result = CapsuleDependency.Encode ()
if args.Verbose:
CapsuleDependency.DumpInfo ()
@@ -607,13 +609,14 @@ if __name__ == '__main__':
FmpAuthHeader.MonotonicCount = SinglePayloadDescriptor.MonotonicCount
FmpAuthHeader.CertData = CertData
FmpAuthHeader.Payload = Result
+ ImageCapsuleSupport |= FmpCapsuleHeader.CAPSULE_SUPPORT_AUTHENTICATION
Result = FmpAuthHeader.Encode ()
if args.Verbose:
FmpAuthHeader.DumpInfo ()
except:
print ('GenerateCapsule: error: can not encode FMP Auth Header')
sys.exit (1)
- FmpCapsuleHeader.AddPayload (SinglePayloadDescriptor.Guid, Result, HardwareInstance = SinglePayloadDescriptor.HardwareInstance, UpdateImageIndex = SinglePayloadDescriptor.UpdateImageIndex)
+ FmpCapsuleHeader.AddPayload (SinglePayloadDescriptor.Guid, Result, HardwareInstance = SinglePayloadDescriptor.HardwareInstance, UpdateImageIndex = SinglePayloadDescriptor.UpdateImageIndex, CapsuleSupport = ImageCapsuleSupport)
try:
for EmbeddedDriver in EmbeddedDriverDescriptorList:
FmpCapsuleHeader.AddEmbeddedDriver(EmbeddedDriver)
diff --git a/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py b/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py
index 91d24919c4..8abb449c6f 100644
--- a/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py
+++ b/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py
@@ -47,14 +47,19 @@ class FmpCapsuleImageHeaderClass (object):
# /// therefore can be modified without changing the Auth data.
# ///
# UINT64 UpdateHardwareInstance;
+ #
+ # ///
+ # /// Bits which indicate authentication and depex information for the image that follows this structure
+ # ///
+ # UINT64 ImageCapsuleSupport
# } EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER;
#
- # #define EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION 0x00000002
+ # #define EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION 0x00000003

- _StructFormat = '<I16sB3BIIQ'
+ _StructFormat = '<I16sB3BIIQQ'
_StructSize = struct.calcsize (_StructFormat)

- EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION = 0x00000002
+ EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION = 0x00000003

def __init__ (self):
self._Valid = False
@@ -64,6 +69,7 @@ class FmpCapsuleImageHeaderClass (object):
self.UpdateImageSize = 0
self.UpdateVendorCodeSize = 0
self.UpdateHardwareInstance = 0x0000000000000000
+ self.ImageCapsuleSupport = 0x0000000000000000
self.Payload = b''
self.VendorCodeBytes = b''

@@ -78,7 +84,8 @@ class FmpCapsuleImageHeaderClass (object):
0,0,0,
self.UpdateImageSize,
self.UpdateVendorCodeSize,
- self.UpdateHardwareInstance
+ self.UpdateHardwareInstance,
+ self.ImageCapsuleSupport
)
self._Valid = True
return FmpCapsuleImageHeader + self.Payload + self.VendorCodeBytes
@@ -86,7 +93,7 @@ class FmpCapsuleImageHeaderClass (object):
def Decode (self, Buffer):
if len (Buffer) < self._StructSize:
raise ValueError
- (Version, UpdateImageTypeId, UpdateImageIndex, r0, r1, r2, UpdateImageSize, UpdateVendorCodeSize, UpdateHardwareInstance) = \
+ (Version, UpdateImageTypeId, UpdateImageIndex, r0, r1, r2, UpdateImageSize, UpdateVendorCodeSize, UpdateHardwareInstance, ImageCapsuleSupport) = \
struct.unpack (
self._StructFormat,
Buffer[0:self._StructSize]
@@ -105,6 +112,7 @@ class FmpCapsuleImageHeaderClass (object):
self.UpdateImageSize = UpdateImageSize
self.UpdateVendorCodeSize = UpdateVendorCodeSize
self.UpdateHardwareInstance = UpdateHardwareInstance
+ self.ImageCapsuleSupport = ImageCapsuleSupport
self.Payload = Buffer[self._StructSize:self._StructSize + UpdateImageSize]
self.VendorCodeBytes = Buffer[self._StructSize + UpdateImageSize:]
self._Valid = True
@@ -119,6 +127,7 @@ class FmpCapsuleImageHeaderClass (object):
print ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.UpdateImageSize = {UpdateImageSize:08X}'.format (UpdateImageSize = self.UpdateImageSize))
print ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.UpdateVendorCodeSize = {UpdateVendorCodeSize:08X}'.format (UpdateVendorCodeSize = self.UpdateVendorCodeSize))
print ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.UpdateHardwareInstance = {UpdateHardwareInstance:016X}'.format (UpdateHardwareInstance = self.UpdateHardwareInstance))
+ print ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.ImageCapsuleSupport = {ImageCapsuleSupport:016X}'.format (ImageCapsuleSupport = self.ImageCapsuleSupport))
print ('sizeof (Payload) = {Size:08X}'.format (Size = len (self.Payload)))
print ('sizeof (VendorCodeBytes) = {Size:08X}'.format (Size = len (self.VendorCodeBytes)))

@@ -153,6 +162,8 @@ class FmpCapsuleHeaderClass (object):
_ItemOffsetSize = struct.calcsize (_ItemOffsetFormat)

EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER_INIT_VERSION = 0x00000001
+ CAPSULE_SUPPORT_AUTHENTICATION = 0x0000000000000001
+ CAPSULE_SUPPORT_DEPENDENCY = 0x0000000000000002

def __init__ (self):
self._Valid = False
@@ -172,8 +183,8 @@ class FmpCapsuleHeaderClass (object):
raise ValueError
return self._EmbeddedDriverList[Index]

- def AddPayload (self, UpdateImageTypeId, Payload = b'', VendorCodeBytes = b'', HardwareInstance = 0, UpdateImageIndex = 1):
- self._PayloadList.append ((UpdateImageTypeId, Payload, VendorCodeBytes, HardwareInstance, UpdateImageIndex))
+ def AddPayload (self, UpdateImageTypeId, Payload = b'', VendorCodeBytes = b'', HardwareInstance = 0, UpdateImageIndex = 1, CapsuleSupport = 0):
+ self._PayloadList.append ((UpdateImageTypeId, Payload, VendorCodeBytes, HardwareInstance, UpdateImageIndex, CapsuleSupport))

def GetFmpCapsuleImageHeader (self, Index):
if Index >= len (self._FmpCapsuleImageHeaderList):
@@ -198,13 +209,14 @@ class FmpCapsuleHeaderClass (object):
self._ItemOffsetList.append (Offset)
Offset = Offset + len (EmbeddedDriver)
Index = 1
- for (UpdateImageTypeId, Payload, VendorCodeBytes, HardwareInstance, UpdateImageIndex) in self._PayloadList:
+ for (UpdateImageTypeId, Payload, VendorCodeBytes, HardwareInstance, UpdateImageIndex, CapsuleSupport) in self._PayloadList:
FmpCapsuleImageHeader = FmpCapsuleImageHeaderClass ()
FmpCapsuleImageHeader.UpdateImageTypeId = UpdateImageTypeId
FmpCapsuleImageHeader.UpdateImageIndex = UpdateImageIndex
FmpCapsuleImageHeader.Payload = Payload
FmpCapsuleImageHeader.VendorCodeBytes = VendorCodeBytes
FmpCapsuleImageHeader.UpdateHardwareInstance = HardwareInstance
+ FmpCapsuleImageHeader.ImageCapsuleSupport = CapsuleSupport
FmpCapsuleImage = FmpCapsuleImageHeader.Encode ()
FmpCapsuleData = FmpCapsuleData + FmpCapsuleImage

--
2.17.1


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

Laszlo Ersek
 

review#2 from scratch:

On 04/21/21 00:54, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345

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) As discussed, please update the commit message, for more clarify
about SEV vs. SEV-ES.


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) {
It's OK to keep this as a sanity check, yes.

+ RETURN_STATUS DecryptStatus;
+
+ DecryptStatus = MemEncryptSevClearPageEncMask (
+ 0,
+ TpmBaseAddress,
+ EFI_SIZE_TO_PAGES (0x5000),
(2) Should be (UINTN)0x5000, as discussed earlier.

+ FALSE
+ );
+
+ ASSERT_RETURN_ERROR (DecryptStatus);
(3) So this is where the mess begins.

The idea is to delay the dispatch of Tcg2ConfigPei until after
PlatformPei determines if SEV is active, and (in case SEV is active)
PlatformPei decrypts the MMIO range of the TPM.

For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
the current TRUE, to some PPI GUID.

There are two choices for that PPI:

(a) gEfiPeiMemoryDiscoveredPpiGuid

Advantages:

- no new PPI definition needed,

- no new PPI installation needed,

- OvmfPkg/Bhyve/PlatformPei needs no separate change

Disadvantages:

- total abuse of gEfiPeiMemoryDiscoveredPpiGuid


(b) gOvmfTpmMmioAccessiblePpiGuid

Disadvantages:

- this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
in a separate patch; its comment should say "this PPI signals that
accessing the MMIO range of the TPM is possible in the PEI phase,
regardless of memory encryption". The PPI definitions should be kept
alphabetically ordered.

- OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
(See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
install this new PPI either when the SEV check at the top of
AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.

- OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
"Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
"OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
stricter-than-before depex, so something on the bhyve platform too must
produce the new PPI.

Advantages:

- more or less palatable as a concept, with the new PPI precisely
expressing the dependency we have.


In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
to the Bhyve reviewers. If the Bhyve reviewers determine that such an
update is actually unnecessary, because on Bhyve, there is no TPM
support and/or no SEV support in fact, then *first* we have to create an
independent Bhyve cleanup series, that rips out the TPM and/or SEV
remnants from the OvmfPkg/Bhyve sub-tree.


I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve,
but I strongly believe in keeping all platforms in the tree, and that
means we need to spend time on such changes.

I'm not CC'ing Rebecca and Peter on this message -- we're deep into this
patch review thread, and they'd have no useful context. I suggest simply
including the "OvmfPkg/Bhyve/PlatformPei" patch in the next version of
this series, with a proper explanation in the blurb (patch#0) and on the
"OvmfPkg/Bhyve/PlatformPei" patch. That should give them enough context
to evaluate whether the change is necessary, or whether we should purge
the TPM and/or the SEV bits from Bhyve. You could also ask them just
this question in advance, in a separate email on the list (with
distilled context). Personally I'm unsure if the TPM and SEV bits
survived into Bhyve because those bits are actually put to use there, or
because the initial platform creation / cloning wasn't as minimal as it
could have been.

Note that in case TPM makes sense on bhyve but SEV doesn't, then
"OvmfPkg/Bhyve/PlatformPei" will have to install the new PPI
unconditionally.

Thanks
Laszlo


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

Laszlo Ersek
 

On 04/22/21 21:10, Tom Lendacky wrote:
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 !!!!
Thank you for checking this approach.

Let me re-review this patch from scratch.

Thanks
Laszlo


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

Laszlo Ersek
 

On 04/22/21 17:42, Tom Lendacky wrote:
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?
Yes, I had the exact same thought process :)

The comment looks good, but how about expressing it as a STATIC_ASSERT,
with sizeof (UINTN) and sizeof (UINT64) being equal? (Alternatively,
about MAX_UINT64 being equal to MAX_UINTN.)

If you find that too verbose, a comment is good enough too, of course.

Thanks!
Laszlo


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 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes

Laszlo Ersek
 

On 04/22/21 15:35, Tom Lendacky wrote:
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.
Ugh, sorry, I must have completely lost track of the context here. I
apologize.

In that case however, it should be DEBUG_ERROR.

Thanks,
Laszlo



(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 1/2] BaseTools: Make undefined VFR macro an error (GCC)

Daniel Schaefer
 

Ok sure, let's make only undef an error, not all other warnings. Then the behaviour will also be the same as on MSVC.


From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Daniel Schaefer <daniel.schaefer@...>
Sent: Monday, March 8, 2021 11:44
To: Feng, Bob C <bob.c.feng@...>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Liming Gao <gaoliming@...>; Chen, Christine <yuwei.chen@...>; Lin, Derek (HPS SW) <derek.lin2@...>
Subject: Re: [edk2-devel] [PATCH v1 1/2] BaseTools: Make undefined VFR macro an error (GCC)
 
It didn't cause any other errors for the huge HPE codebase. Only undefined macros. I don't believe the preprocessor has so many warnings anyways.
So -Werror should be fine.


From: Feng, Bob C <bob.c.feng@...>
Sent: Monday, March 8, 2021 09:05
To: devel@edk2.groups.io <devel@edk2.groups.io>; Schaefer, Daniel <daniel.schaefer@...>
Cc: Liming Gao <gaoliming@...>; Chen, Christine <yuwei.chen@...>; Lin, Derek (HPS SW) <derek.lin2@...>
Subject: RE: [edk2-devel] [PATCH v1 1/2] BaseTools: Make undefined VFR macro an error (GCC)
 
Hi Derek,

-Werror.  Make all warnings into errors.

Should here be that only treat undef warning as error?

Thanks,
Bob

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Daniel Schaefer
Sent: Tuesday, March 2, 2021 4:22 PM
To: devel@edk2.groups.io
Cc: Feng, Bob C <bob.c.feng@...>; Liming Gao <gaoliming@...>; Chen, Christine <yuwei.chen@...>; Derek Lin <derek.lin2@...>
Subject: [edk2-devel] [PATCH v1 1/2] BaseTools: Make undefined VFR macro an error (GCC)

VFR successfully compiles if we forget to include a header that defines a macro. In that case the HII option was hidden when it shouldn't be just because the macro was used but not defined.

The behaviour is totally intended by the C/PP standard. When a macro is undefined it evaluates to 0.
GCC, MSVC and Clang have warnings to catch this type of mistake. With this commit we enable this warning and make it a compiler error.

Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>
Cc: Yuwei Chen <yuwei.chen@...>
Cc: Derek Lin <derek.lin2@...>
---
 BaseTools/Conf/tools_def.template | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 933b3160fd2b..728c1d3119e4 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3,7 +3,7 @@
 #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>  #  Portions copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>  #  Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> -#  (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR>
+#  (C) Copyright 2020-2021, Hewlett Packard Enterprise Development
+LP<BR>
 #  Copyright (c) Microsoft Corporation
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -1938,7 +1938,7 @@ DEFINE GCC_AARCH64_ASLDLINK_FLAGS  = DEF(GCC_AARCH64_DLINK_FLAGS) -Wl,--entry,Re
 DEFINE GCC_IA32_X64_DLINK_FLAGS    = DEF(GCC_IA32_X64_DLINK_COMMON) --entry _$(IMAGE_ENTRY_POINT) --file-alignment 0x20 --section-alignment 0x20 -Map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
 DEFINE GCC_ASM_FLAGS               = -c -x assembler -imacros AutoGen.h
 DEFINE GCC_PP_FLAGS                = -E -x assembler-with-cpp -include AutoGen.h
-DEFINE GCC_VFRPP_FLAGS             = -x c -E -P -DVFRCOMPILE --include $(MODULE_NAME)StrDefs.h
+DEFINE GCC_VFRPP_FLAGS             = -x c -E -P -DVFRCOMPILE --include $(MODULE_NAME)StrDefs.h -Wundef -Werror
 DEFINE GCC_ASLPP_FLAGS             = -x c -E -include AutoGen.h
 DEFINE GCC_ASLCC_FLAGS             = -x c
 DEFINE GCC_WINDRES_FLAGS           = -J rc -O coff
--
2.30.0







Re: 回复: [PATCH v1 2/2] BaseTools: Make undefined VFR macro an error (MSVC)

Daniel Schaefer
 

Ok, I'll send a new series without EBC. Can't find anything about it and we don't use it.


From: Schaefer, Daniel <daniel.schaefer@...>
Sent: Thursday, March 4, 2021 10:46
To: gaoliming <gaoliming@...>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: 'Bob Feng' <bob.c.feng@...>; 'Yuwei Chen' <yuwei.chen@...>; Lin, Derek (HPS SW) <derek.lin2@...>
Subject: Re: 回复: [PATCH v1 2/2] BaseTools: Make undefined VFR macro an error (MSVC)
 
Hi Liming,

as stated in the coverletter, "I only tested GCC5, CLANPDB and VS2015 toolchains."

Clang support is documented here: https://clang.llvm.org/docs/DiagnosticsReference.html#wundef
GCC support is documented here: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
MSVC support is documented here: https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4668?view=msvc-160

I'm sorry, I'm unable to find documentation for, or even the EBC compiler itself.
Can you please help me with this?

Thanks,
Daniel

On 3/4/21 10:12 AM, gaoliming wrote:
> Do you check whether EBC compiler supports this warning?
>
> And, do you evaluate CLANG compiler support for this warning?
>
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: Daniel Schaefer <daniel.schaefer@...>
>> 发送时间: 2021年3月2日 16:22
>> 收件人: devel@edk2.groups.io
>> 抄送: Bob Feng <bob.c.feng@...>; Liming Gao
>> <gaoliming@...>; Yuwei Chen <yuwei.chen@...>; Derek
>> Lin <derek.lin2@...>
>> 主题: [PATCH v1 2/2] BaseTools: Make undefined VFR macro an error (MSVC)
>>
>> VFR successfully compiles if we forget to include a header that defines
>> a macro. In that case the HII option was hidden when it shouldn't be
>> just because the macro was used but not defined.
>>
>> The behaviour is totally intended by the C/PP standard. When a macro is
>> undefined it evaluates to 0.
>> GCC, MSVC and Clang have warnings to catch this type of mistake. With
>> this commit we enable this warning and make it a compiler error.
>>
>> Cc: Bob Feng <bob.c.feng@...>
>> Cc: Liming Gao <gaoliming@...>
>> Cc: Yuwei Chen <yuwei.chen@...>
>> Cc: Derek Lin <derek.lin2@...>
>> ---
>>   BaseTools/Conf/tools_def.template | 46 ++++++++++----------
>>   1 file changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/BaseTools/Conf/tools_def.template
>> b/BaseTools/Conf/tools_def.template
>> index 728c1d3119e4..56c7bd13f157 100755
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -422,7 +422,7 @@ DEFINE DTC_BIN                 =
>> ENV(DTC_PREFIX)dtc
>>   *_VS2008_*_SLINK_FLAGS            = /NOLOGO /LTCG
>>
>>   *_VS2008_*_APP_FLAGS              = /nologo /E /TC
>>
>>   *_VS2008_*_PP_FLAGS               = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2008_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2008_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2008_*_DEPS_FLAGS            = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2008_*_ASM16_PATH             = DEF(VS2008_BIN)\ml.exe
>>
>>
>>
>> @@ -518,7 +518,7 @@ NOOPT_VS2008_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2008_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2008_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2008_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2008_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2008_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2008_EBC_SLINK_FLAGS         = /lib /NOLOGO /MACHINE:EBC
>>
>>   *_VS2008_EBC_DLINK_FLAGS         = "C:\Program
>> Files\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -538,7 +538,7 @@ NOOPT_VS2008_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2008x86_*_SLINK_FLAGS     = /NOLOGO /LTCG
>>
>>   *_VS2008x86_*_APP_FLAGS       = /nologo /E /TC
>>
>>   *_VS2008x86_*_PP_FLAGS        = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2008x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2008x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2008x86_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2008x86_*_ASM16_PATH      = DEF(VS2008x86_BIN)\ml.exe
>>
>>
>>
>> @@ -633,7 +633,7 @@ NOOPT_VS2008x86_X64_DLINK_FLAGS    =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2008x86_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2008x86_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2008x86_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2008x86_EBC_VFRPP_FLAGS         = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2008x86_EBC_VFRPP_FLAGS         = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2008x86_EBC_SLINK_FLAGS         = /lib /NOLOGO
>> /MACHINE:EBC
>>
>>   *_VS2008x86_EBC_DLINK_FLAGS         = "C:\Program Files
>> (x86)\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -656,7 +656,7 @@ NOOPT_VS2008x86_X64_DLINK_FLAGS    =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2010_*_SLINK_FLAGS            = /NOLOGO /LTCG
>>
>>   *_VS2010_*_APP_FLAGS              = /nologo /E /TC
>>
>>   *_VS2010_*_PP_FLAGS               = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2010_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2010_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2010_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2010_*_ASM16_PATH             = DEF(VS2010_BIN)\ml.exe
>>
>>
>>
>> @@ -752,7 +752,7 @@ NOOPT_VS2010_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2010_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2010_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2010_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2010_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2010_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2010_EBC_SLINK_FLAGS         = /lib /NOLOGO /MACHINE:EBC
>>
>>   *_VS2010_EBC_DLINK_FLAGS         = "C:\Program
>> Files\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -772,7 +772,7 @@ NOOPT_VS2010_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2010x86_*_SLINK_FLAGS     = /NOLOGO /LTCG
>>
>>   *_VS2010x86_*_APP_FLAGS       = /nologo /E /TC
>>
>>   *_VS2010x86_*_PP_FLAGS        = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2010x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2010x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2010x86_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2010x86_*_ASM16_PATH      = DEF(VS2010x86_BIN)\ml.exe
>>
>>
>>
>> @@ -868,7 +868,7 @@ NOOPT_VS2010x86_X64_DLINK_FLAGS    =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2010x86_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2010x86_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2010x86_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2010x86_EBC_VFRPP_FLAGS         = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2010x86_EBC_VFRPP_FLAGS         = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2010x86_EBC_SLINK_FLAGS         = /lib /NOLOGO
>> /MACHINE:EBC
>>
>>   *_VS2010x86_EBC_DLINK_FLAGS         = "C:\Program Files
>> (x86)\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -890,7 +890,7 @@ NOOPT_VS2010x86_X64_DLINK_FLAGS    =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2012_*_SLINK_FLAGS            = /NOLOGO /LTCG
>>
>>   *_VS2012_*_APP_FLAGS              = /nologo /E /TC
>>
>>   *_VS2012_*_PP_FLAGS               = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2012_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2012_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2012_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2012_*_ASM16_PATH             = DEF(VS2012_BIN)\ml.exe
>>
>>
>>
>> @@ -986,7 +986,7 @@ NOOPT_VS2012_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2012_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2012_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2012_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2012_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2012_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2012_EBC_SLINK_FLAGS         = /lib /NOLOGO /MACHINE:EBC
>>
>>   *_VS2012_EBC_DLINK_FLAGS         = "C:\Program
>> Files\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -1006,7 +1006,7 @@ NOOPT_VS2012_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2012x86_*_SLINK_FLAGS     = /NOLOGO /LTCG
>>
>>   *_VS2012x86_*_APP_FLAGS       = /nologo /E /TC
>>
>>   *_VS2012x86_*_PP_FLAGS        = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2012x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2012x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2012x86_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2012x86_*_ASM16_PATH      = DEF(VS2012x86_BIN)\ml.exe
>>
>>
>>
>> @@ -1102,7 +1102,7 @@ NOOPT_VS2012x86_X64_DLINK_FLAGS    =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2012x86_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2012x86_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2012x86_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2012x86_EBC_VFRPP_FLAGS         = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2012x86_EBC_VFRPP_FLAGS         = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2012x86_EBC_SLINK_FLAGS         = /lib /NOLOGO
>> /MACHINE:EBC
>>
>>   *_VS2012x86_EBC_DLINK_FLAGS         = "C:\Program Files
>> (x86)\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -1124,7 +1124,7 @@ NOOPT_VS2012x86_X64_DLINK_FLAGS    =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2013_*_SLINK_FLAGS            = /NOLOGO /LTCG
>>
>>   *_VS2013_*_APP_FLAGS              = /nologo /E /TC
>>
>>   *_VS2013_*_PP_FLAGS               = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2013_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2013_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2013_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2013_*_ASM16_PATH             = DEF(VS2013_BIN)\ml.exe
>>
>>
>>
>> @@ -1220,7 +1220,7 @@ NOOPT_VS2013_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2013_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2013_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2013_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2013_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2013_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2013_EBC_SLINK_FLAGS         = /lib /NOLOGO /MACHINE:EBC
>>
>>   *_VS2013_EBC_DLINK_FLAGS         = "C:\Program
>> Files\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -1240,7 +1240,7 @@ NOOPT_VS2013_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2013x86_*_SLINK_FLAGS     = /NOLOGO /LTCG
>>
>>   *_VS2013x86_*_APP_FLAGS       = /nologo /E /TC
>>
>>   *_VS2013x86_*_PP_FLAGS        = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2013x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2013x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2013x86_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2013x86_*_ASM16_PATH      = DEF(VS2013x86_BIN)\ml.exe
>>
>>
>>
>> @@ -1336,7 +1336,7 @@ NOOPT_VS2013x86_X64_DLINK_FLAGS    =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2013x86_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2013x86_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2013x86_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2013x86_EBC_VFRPP_FLAGS         = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2013x86_EBC_VFRPP_FLAGS         = /nologo /E /TC
>> /DVFRCOMPILE /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2013x86_EBC_SLINK_FLAGS         = /lib /NOLOGO
>> /MACHINE:EBC
>>
>>   *_VS2013x86_EBC_DLINK_FLAGS         = "C:\Program Files
>> (x86)\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -1358,7 +1358,7 @@ NOOPT_VS2013x86_X64_DLINK_FLAGS    =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2015_*_SLINK_FLAGS            = /NOLOGO /LTCG
>>
>>   *_VS2015_*_APP_FLAGS              = /nologo /E /TC
>>
>>   *_VS2015_*_PP_FLAGS               = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2015_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2015_*_VFRPP_FLAGS            = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2015_*_DLINK2_FLAGS           =
>>
>>   *_VS2015_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2015_*_ASM16_PATH             = DEF(VS2015_BIN)\ml.exe
>>
>> @@ -1455,7 +1455,7 @@ NOOPT_VS2015_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2015_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2015_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2015_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2015_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2015_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2015_EBC_SLINK_FLAGS         = /lib /NOLOGO /MACHINE:EBC
>>
>>   *_VS2015_EBC_DLINK_FLAGS         = "C:\Program
>> Files\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -1475,7 +1475,7 @@ NOOPT_VS2015_X64_DLINK_FLAGS  = /NOLOGO
>> /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT
>>   *_VS2015x86_*_SLINK_FLAGS     = /NOLOGO /LTCG
>>
>>   *_VS2015x86_*_APP_FLAGS       = /nologo /E /TC
>>
>>   *_VS2015x86_*_PP_FLAGS        = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2015x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2015x86_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2015x86_*_DLINK2_FLAGS    =
>>
>>   *_VS2015x86_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>>   *_VS2015x86_*_ASM16_PATH      = DEF(VS2015x86_BIN)\ml.exe
>>
>> @@ -1593,7 +1593,7 @@ NOOPT_VS2015x86_X64_DLINK_FLAGS    =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2017_*_SLINK_FLAGS     = /NOLOGO /LTCG
>>
>>   *_VS2017_*_APP_FLAGS       = /nologo /E /TC
>>
>>   *_VS2017_*_PP_FLAGS        = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2017_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2017_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2017_*_DLINK2_FLAGS    = /WHOLEARCHIVE
>>
>>   *_VS2017_*_ASM16_PATH      = DEF(VS2017_BIN_IA32)\ml.exe
>>
>>   *_VS2017_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>> @@ -1736,7 +1736,7 @@ NOOPT_VS2017_AARCH64_DLINK_FLAGS   =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2017_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2017_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2017_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2017_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2017_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2017_EBC_SLINK_FLAGS         = /lib /NOLOGO /MACHINE:EBC
>>
>>   *_VS2017_EBC_DLINK_FLAGS         = "C:\Program Files
>> (x86)\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> @@ -1756,7 +1756,7 @@ NOOPT_VS2017_AARCH64_DLINK_FLAGS   =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2019_*_SLINK_FLAGS     = /NOLOGO /LTCG
>>
>>   *_VS2019_*_APP_FLAGS       = /nologo /E /TC
>>
>>   *_VS2019_*_PP_FLAGS        = /nologo /E /TC /FIAutoGen.h
>>
>> -*_VS2019_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2019_*_VFRPP_FLAGS     = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2019_*_DLINK2_FLAGS    = /WHOLEARCHIVE
>>
>>   *_VS2019_*_ASM16_PATH      = DEF(VS2019_BIN_IA32)\ml.exe
>>
>>   *_VS2019_*_DEPS_FLAGS      = DEF(MSFT_DEPS_FLAGS)
>>
>> @@ -1899,7 +1899,7 @@ NOOPT_VS2019_AARCH64_DLINK_FLAGS   =
>> /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF
>>   *_VS2019_EBC_MAKE_FLAGS          = /nologo
>>
>>   *_VS2019_EBC_PP_FLAGS            = /nologo /E /TC /FIAutoGen.h
>>
>>   *_VS2019_EBC_CC_FLAGS            = /nologo /c /WX /W3
>> /FIAutoGen.h /D$(MODULE_ENTRY_POINT)=$(ARCH_ENTRY_POINT)
>>
>> -*_VS2019_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h
>>
>> +*_VS2019_EBC_VFRPP_FLAGS         = /nologo /E /TC /DVFRCOMPILE
>> /FI$(MODULE_NAME)StrDefs.h /we4668
>>
>>   *_VS2019_EBC_SLINK_FLAGS         = /lib /NOLOGO /MACHINE:EBC
>>
>>   *_VS2019_EBC_DLINK_FLAGS         = "C:\Program Files
>> (x86)\Intel\EBC\Lib\EbcLib.lib" /NOLOGO /NODEFAULTLIB /MACHINE:EBC
>> /OPT:REF /ENTRY:$(IMAGE_ENTRY_POINT)
>> /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /MAP /ALIGN:32 /DRIVER
>>
>>
>>
>> --
>> 2.30.0
>
>
>


Re: [PATCH] SecurityPkg: Add constraints on PK strength

Yao, Jiewen
 

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Friday, April 23, 2021 9:36 AM
To: Gao, Jiaqi <jiaqi.gao@intel.com>; devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength

This patch is good to me.
Reviewed-by: Min Xu <min.m.xu@intel.com>

-----Original Message-----
From: Gao, Jiaqi <jiaqi.gao@intel.com>
Sent: Monday, April 19, 2021 9:31 AM
To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength

Hi,

The patch has been built and tested with several toolchains:
1. GCC5 on Linux, both DEBUG and RELEASE.
2. VS2017 on Windows, both DEBUG and RELEASE.
3. VS2019 on Windows, both DEBUG and RELEASE.

To make sure the program can cope with various input, test cases consist of
different PK certificate enrollment , which are:
1. Platform Keys (PKs) with RSA public key length less than 2048 bits, include
RSA-512 and RSA-1024, etc. These kind of certificates were rejected during
user
enrollment.
2. PKs with RSA public key length equal to or greater than 2048 bits, include
RSA-
2048, RSA-3072 and RSA-4096, etc. These kind of certificates were
successfully
enrolled.
3. PKs which are not DER encoded, such as PEM encoded certificates
with .cer/.der/.crt file suffix.
4. Empty PKs.
5. Empty inputs.

All the test cases were performed as expected. Test cases with unqualified
key
strength pop up the prompt of unqualified key, and the others with
unsupported
encode format or illegal input act as previous program.


Best Regards,
Jiaqi

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Monday, April 19, 2021 7:52 AM
To: Gao, Jiaqi <jiaqi.gao@intel.com>; devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength

Have you tested the patch? Would you please post the test result in the
mail
thread?
Thanks.

-----Original Message-----
From: Gao, Jiaqi <jiaqi.gao@intel.com>
Sent: Friday, April 16, 2021 3:56 PM
To: devel@edk2.groups.io
Cc: Gao, Jiaqi <jiaqi.gao@intel.com>; Xu, Min M <min.m.xu@intel.com>;
Yao, Jiewen <jiewen.yao@intel.com>
Subject: [PATCH] SecurityPkg: Add constraints on PK strength

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

Add constraints on the key strength of enrolled platform key(PK),
which must be greater than or equal to 2048 bit.PK key strength is
required by Intel SDL and MSFT, etc. This limitation prevents user from
using
weak keys as PK.

The original code to check the certificate file type is placed in a
new function CheckX509Certificate(), which checks if the X.509
certificate meets the requirements of encode type, RSA-Key strengh, etc.

Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Jiaqi Gao <jiaqi.gao@intel.com>
---
.../SecureBootConfigImpl.c | 165 +++++++++++++++---
.../SecureBootConfigImpl.h | 21 +++
2 files changed, 160 insertions(+), 26 deletions(-)

diff --git
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.c
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.c
index 4f01a2ed67..1304e21266 100644
---
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.c
+++
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
+++ Co
+++ nfigImpl.c
@@ -90,6 +90,22 @@ CHAR16* mDerEncodedSuffix[] = { };
CHAR16* mSupportX509Suffix = L"*.cer/der/crt";

+//
+// Prompt strings during certificate enrollment.
+//
+CHAR16* mX509EnrollPromptTitle[] = {
+ L"",
+ L"ERROR: Unsupported file type!",
+ L"ERROR: Unsupported certificate!",
+ NULL
+};
+CHAR16* mX509EnrollPromptString[] = {
+ L"",
+ L"Only DER encoded certificate file (*.cer/der/crt) is supported.",
+ L"Public key length should be equal to or greater than 2048 bits.",
+ NULL
+};
+
SECUREBOOT_CONFIG_PRIVATE_DATA *gSecureBootPrivateData = NULL;

/**
@@ -383,6 +399,102 @@ SetSecureBootMode (
);
}

+/**
+ This code checks if the encode type and key strength of X.509
+ certificate is qualified.
+
+ @param[in] X509FileContext FileContext of X.509 certificate storing
+ file.
+ @param[out] Error Error type checked in the certificate.
+
+ @return EFI_SUCCESS The certificate checked successfully.
+ @return EFI_INVALID_PARAMETER The parameter is invalid.
+ @return EFI_OUT_OF_RESOURCES Memory allocation failed.
+
+**/
+EFI_STATUS
+CheckX509Certificate (
+ IN SECUREBOOT_FILE_CONTEXT* X509FileContext,
+ OUT ENROLL_KEY_ERROR* Error
+)
+{
+ EFI_STATUS Status;
+ UINT16* FilePostFix;
+ UINTN NameLength;
+ UINT8* X509Data;
+ UINTN X509DataSize;
+ void* X509PubKey;
+ UINTN PubKeyModSize;
+
+ if (X509FileContext->FileName == NULL) {
+ *Error = Unsupported_Type;
+ return EFI_INVALID_PARAMETER;
+ }
+
+ X509Data = NULL;
+ X509DataSize = 0;
+ X509PubKey = NULL;
+ PubKeyModSize = 0;
+
+ //
+ // Parse the file's postfix. Only support DER encoded X.509 certificate
files.
+ //
+ NameLength = StrLen (X509FileContext->FileName); if (NameLength <=
+ 4) {
+ DEBUG ((DEBUG_ERROR, "Wrong X509 NameLength\n"));
+ *Error = Unsupported_Type;
+ return EFI_INVALID_PARAMETER;
+ }
+ FilePostFix = X509FileContext->FileName + NameLength - 4; if
+ (!IsDerEncodeCertificate (FilePostFix)) {
+ DEBUG ((DEBUG_ERROR, "Unsupported file type, only DER encoded
certificate (%s) is supported.\n", mSupportX509Suffix));
+ *Error = Unsupported_Type;
+ return EFI_INVALID_PARAMETER;
+ }
+ DEBUG ((DEBUG_INFO, "FileName= %s\n", X509FileContext->FileName));
+ DEBUG ((DEBUG_INFO, "FilePostFix = %s\n", FilePostFix));
+
+ //
+ // Read the certificate file content // Status = ReadFileContent
+ (X509FileContext->FHandle, (VOID**) &X509Data, &X509DataSize, 0); if
+ (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Error occured while reading the file.\n"));
+ goto ON_EXIT;
+ }
+
+ //
+ // Parse the public key context.
+ //
+ if (RsaGetPublicKeyFromX509 (X509Data, X509DataSize, &X509PubKey)
+ ==
FALSE) {
+ DEBUG ((DEBUG_ERROR, "Error occured while parsing the pubkey from
certificate.\n"));
+ Status = EFI_INVALID_PARAMETER;
+ *Error = Unsupported_Type;
+ goto ON_EXIT;
+ }
+
+ //
+ // Parse Module size of public key using interface provided by
+ CryptoPkg, which is // actually the size of public key.
+ //
+ if (X509PubKey != NULL) {
+ RsaGetKey (X509PubKey, RsaKeyN, NULL, &PubKeyModSize);
+ if (PubKeyModSize < CER_PUBKEY_MIN_SIZE) {
+ DEBUG ((DEBUG_ERROR, "Unqualified PK size, key size should be
+ equal to
or greater than 2048 bits.\n"));
+ Status = EFI_INVALID_PARAMETER;
+ *Error = Unqualified_Key;
+ }
+ RsaFree (X509PubKey);
+ }
+
+ ON_EXIT:
+ if (X509Data != NULL) {
+ FreePool (X509Data);
+ }
+
+ return Status;
+}
+
/**
Generate the PK signature list from the X509 Certificate storing
file (.cer)

@@ -461,7 +573,10 @@ ON_EXIT:

The SignatureOwner GUID will be the same with PK's vendorguid.

- @param[in] PrivateData The module's private data.
+ @param[in] PrivateData The module's private data.
+ @param[out] Error Point to the error code which indicates the
+ error during enroll process.
+

@retval EFI_SUCCESS New PK enrolled successfully.
@retval EFI_INVALID_PARAMETER The parameter is invalid.
@@ -477,12 +592,6 @@ EnrollPlatformKey (
UINT32 Attr;
UINTN DataSize;
EFI_SIGNATURE_LIST *PkCert;
- UINT16* FilePostFix;
- UINTN NameLength;
-
- if (Private->FileContext->FileName == NULL) {
- return EFI_INVALID_PARAMETER;
- }

PkCert = NULL;

@@ -491,21 +600,6 @@ EnrollPlatformKey (
return Status;
}

- //
- // Parse the file's postfix. Only support DER encoded X.509 certificate
files.
- //
- NameLength = StrLen (Private->FileContext->FileName);
- if (NameLength <= 4) {
- return EFI_INVALID_PARAMETER;
- }
- FilePostFix = Private->FileContext->FileName + NameLength - 4;
- if (!IsDerEncodeCertificate(FilePostFix)) {
- DEBUG ((EFI_D_ERROR, "Unsupported file type, only DER encoded
certificate (%s) is supported.", mSupportX509Suffix));
- return EFI_INVALID_PARAMETER;
- }
- DEBUG ((EFI_D_INFO, "FileName= %s\n",
Private->FileContext->FileName));
- DEBUG ((EFI_D_INFO, "FilePostFix = %s\n", FilePostFix));
-
//
// Prase the selected PK file and generate PK certificate list.
//
@@ -4300,12 +4394,14 @@ SecureBootCallback (
UINT16 *FilePostFix;
SECUREBOOT_CONFIG_PRIVATE_DATA *PrivateData;
BOOLEAN GetBrowserDataResult;
+ ENROLL_KEY_ERROR EnrollKeyErrorCode;

Status = EFI_SUCCESS;
SecureBootEnable = NULL;
SecureBootMode = NULL;
SetupMode = NULL;
File = NULL;
+ EnrollKeyErrorCode = None_Error;

if ((This == NULL) || (Value == NULL) || (ActionRequest == NULL)) {
return EFI_INVALID_PARAMETER;
@@ -4718,18 +4814,35 @@ SecureBootCallback (
}
break;
case KEY_VALUE_SAVE_AND_EXIT_PK:
- Status = EnrollPlatformKey (Private);
+ //
+ // Check the suffix, encode type and the key strength of PK certificate.
+ //
+ Status = CheckX509Certificate (Private->FileContext,
&EnrollKeyErrorCode);
+ if (EFI_ERROR (Status)) {
+ if (EnrollKeyErrorCode != None_Error && EnrollKeyErrorCode <
Enroll_Error_Max) {
+ CreatePopUp (
+ EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
+ &Key,
+ mX509EnrollPromptTitle[EnrollKeyErrorCode],
+ mX509EnrollPromptString[EnrollKeyErrorCode],
+ NULL
+ );
+ break;
+ }
+ } else {
+ Status = EnrollPlatformKey (Private);
+ }
if (EFI_ERROR (Status)) {
UnicodeSPrint (
PromptString,
sizeof (PromptString),
- L"Only DER encoded certificate file (%s) is supported.",
- mSupportX509Suffix
+ L"Error status: %x.",
+ Status
);
CreatePopUp (
EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
&Key,
- L"ERROR: Unsupported file type!",
+ L"ERROR: Enrollment failed!",
PromptString,
NULL
);
diff --git
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.h
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.h
index 1fafae07ac..268f015e8e 100644
---
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.h
+++
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
+++ Co
+++ nfigImpl.h
@@ -93,6 +93,27 @@ extern EFI_IFR_GUID_LABEL *mEndLabel;
#define HASHALG_RAW 0x00000004
#define HASHALG_MAX 0x00000004

+//
+// Certificate public key minimum size (bytes) //
+#define CER_PUBKEY_MIN_SIZE 256
+
+//
+// Types of errors may occur during certificate enrollment.
+//
+typedef enum {
+ None_Error = 0,
+ //
+ // Unsupported_type indicates the certificate type is not supported.
+ //
+ Unsupported_Type,
+ //
+ // Unqualified_key indicates the key strength of certificate is not
+ // strong enough.
+ //
+ Unqualified_Key,
+ Enroll_Error_Max
+}ENROLL_KEY_ERROR;

typedef struct {
UINTN Signature;
--
2.31.1.windows.1


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

Wu, Hao A
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
gaoliming
Sent: Friday, April 23, 2021 8:53 AM
To: 'zhoucheng' <zhoucheng@phytium.com.cn>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [edk2-devel] 回复: [PATCH v1 1/2] MdePkg:Update
IndustryStandard/Nvme.h with Nvme amdin controller data

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
*/

I did not see the origin patch was delivered to the mailing list. (Not sure if there is something wrong with the mail client on my side.)

One minor comment:
Do you think we can use UINT32 for definition 'Hmpre' & 'Hmmin'?

Best Regards,
Hao Wu


+ UINT8 Tnvmcap[16]; /* Total NVM Capacity */
+ UINT8 Rsvd2[216]; /* Reserved as of Nvm Express 1.3 Spec
*/
//
// NVM Command Set Attributes
//
--
1.9.1






Re: [PATCH] SecurityPkg: Add constraints on PK strength

Min Xu
 

This patch is good to me.
Reviewed-by: Min Xu <min.m.xu@intel.com>

-----Original Message-----
From: Gao, Jiaqi <jiaqi.gao@intel.com>
Sent: Monday, April 19, 2021 9:31 AM
To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength

Hi,

The patch has been built and tested with several toolchains:
1. GCC5 on Linux, both DEBUG and RELEASE.
2. VS2017 on Windows, both DEBUG and RELEASE.
3. VS2019 on Windows, both DEBUG and RELEASE.

To make sure the program can cope with various input, test cases consist of
different PK certificate enrollment , which are:
1. Platform Keys (PKs) with RSA public key length less than 2048 bits, include
RSA-512 and RSA-1024, etc. These kind of certificates were rejected during user
enrollment.
2. PKs with RSA public key length equal to or greater than 2048 bits, include RSA-
2048, RSA-3072 and RSA-4096, etc. These kind of certificates were successfully
enrolled.
3. PKs which are not DER encoded, such as PEM encoded certificates
with .cer/.der/.crt file suffix.
4. Empty PKs.
5. Empty inputs.

All the test cases were performed as expected. Test cases with unqualified key
strength pop up the prompt of unqualified key, and the others with unsupported
encode format or illegal input act as previous program.


Best Regards,
Jiaqi

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Monday, April 19, 2021 7:52 AM
To: Gao, Jiaqi <jiaqi.gao@intel.com>; devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength

Have you tested the patch? Would you please post the test result in the mail
thread?
Thanks.

-----Original Message-----
From: Gao, Jiaqi <jiaqi.gao@intel.com>
Sent: Friday, April 16, 2021 3:56 PM
To: devel@edk2.groups.io
Cc: Gao, Jiaqi <jiaqi.gao@intel.com>; Xu, Min M <min.m.xu@intel.com>;
Yao, Jiewen <jiewen.yao@intel.com>
Subject: [PATCH] SecurityPkg: Add constraints on PK strength

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

Add constraints on the key strength of enrolled platform key(PK),
which must be greater than or equal to 2048 bit.PK key strength is
required by Intel SDL and MSFT, etc. This limitation prevents user from using
weak keys as PK.

The original code to check the certificate file type is placed in a
new function CheckX509Certificate(), which checks if the X.509
certificate meets the requirements of encode type, RSA-Key strengh, etc.

Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Jiaqi Gao <jiaqi.gao@intel.com>
---
.../SecureBootConfigImpl.c | 165 +++++++++++++++---
.../SecureBootConfigImpl.h | 21 +++
2 files changed, 160 insertions(+), 26 deletions(-)

diff --git
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.c
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.c
index 4f01a2ed67..1304e21266 100644
---
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.c
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
+++ Co
+++ nfigImpl.c
@@ -90,6 +90,22 @@ CHAR16* mDerEncodedSuffix[] = { };
CHAR16* mSupportX509Suffix = L"*.cer/der/crt";

+//
+// Prompt strings during certificate enrollment.
+//
+CHAR16* mX509EnrollPromptTitle[] = {
+ L"",
+ L"ERROR: Unsupported file type!",
+ L"ERROR: Unsupported certificate!",
+ NULL
+};
+CHAR16* mX509EnrollPromptString[] = {
+ L"",
+ L"Only DER encoded certificate file (*.cer/der/crt) is supported.",
+ L"Public key length should be equal to or greater than 2048 bits.",
+ NULL
+};
+
SECUREBOOT_CONFIG_PRIVATE_DATA *gSecureBootPrivateData = NULL;

/**
@@ -383,6 +399,102 @@ SetSecureBootMode (
);
}

+/**
+ This code checks if the encode type and key strength of X.509
+ certificate is qualified.
+
+ @param[in] X509FileContext FileContext of X.509 certificate storing
+ file.
+ @param[out] Error Error type checked in the certificate.
+
+ @return EFI_SUCCESS The certificate checked successfully.
+ @return EFI_INVALID_PARAMETER The parameter is invalid.
+ @return EFI_OUT_OF_RESOURCES Memory allocation failed.
+
+**/
+EFI_STATUS
+CheckX509Certificate (
+ IN SECUREBOOT_FILE_CONTEXT* X509FileContext,
+ OUT ENROLL_KEY_ERROR* Error
+)
+{
+ EFI_STATUS Status;
+ UINT16* FilePostFix;
+ UINTN NameLength;
+ UINT8* X509Data;
+ UINTN X509DataSize;
+ void* X509PubKey;
+ UINTN PubKeyModSize;
+
+ if (X509FileContext->FileName == NULL) {
+ *Error = Unsupported_Type;
+ return EFI_INVALID_PARAMETER;
+ }
+
+ X509Data = NULL;
+ X509DataSize = 0;
+ X509PubKey = NULL;
+ PubKeyModSize = 0;
+
+ //
+ // Parse the file's postfix. Only support DER encoded X.509 certificate files.
+ //
+ NameLength = StrLen (X509FileContext->FileName); if (NameLength <=
+ 4) {
+ DEBUG ((DEBUG_ERROR, "Wrong X509 NameLength\n"));
+ *Error = Unsupported_Type;
+ return EFI_INVALID_PARAMETER;
+ }
+ FilePostFix = X509FileContext->FileName + NameLength - 4; if
+ (!IsDerEncodeCertificate (FilePostFix)) {
+ DEBUG ((DEBUG_ERROR, "Unsupported file type, only DER encoded
certificate (%s) is supported.\n", mSupportX509Suffix));
+ *Error = Unsupported_Type;
+ return EFI_INVALID_PARAMETER;
+ }
+ DEBUG ((DEBUG_INFO, "FileName= %s\n", X509FileContext->FileName));
+ DEBUG ((DEBUG_INFO, "FilePostFix = %s\n", FilePostFix));
+
+ //
+ // Read the certificate file content // Status = ReadFileContent
+ (X509FileContext->FHandle, (VOID**) &X509Data, &X509DataSize, 0); if
+ (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Error occured while reading the file.\n"));
+ goto ON_EXIT;
+ }
+
+ //
+ // Parse the public key context.
+ //
+ if (RsaGetPublicKeyFromX509 (X509Data, X509DataSize, &X509PubKey)
+ ==
FALSE) {
+ DEBUG ((DEBUG_ERROR, "Error occured while parsing the pubkey from
certificate.\n"));
+ Status = EFI_INVALID_PARAMETER;
+ *Error = Unsupported_Type;
+ goto ON_EXIT;
+ }
+
+ //
+ // Parse Module size of public key using interface provided by
+ CryptoPkg, which is // actually the size of public key.
+ //
+ if (X509PubKey != NULL) {
+ RsaGetKey (X509PubKey, RsaKeyN, NULL, &PubKeyModSize);
+ if (PubKeyModSize < CER_PUBKEY_MIN_SIZE) {
+ DEBUG ((DEBUG_ERROR, "Unqualified PK size, key size should be
+ equal to
or greater than 2048 bits.\n"));
+ Status = EFI_INVALID_PARAMETER;
+ *Error = Unqualified_Key;
+ }
+ RsaFree (X509PubKey);
+ }
+
+ ON_EXIT:
+ if (X509Data != NULL) {
+ FreePool (X509Data);
+ }
+
+ return Status;
+}
+
/**
Generate the PK signature list from the X509 Certificate storing
file (.cer)

@@ -461,7 +573,10 @@ ON_EXIT:

The SignatureOwner GUID will be the same with PK's vendorguid.

- @param[in] PrivateData The module's private data.
+ @param[in] PrivateData The module's private data.
+ @param[out] Error Point to the error code which indicates the
+ error during enroll process.
+

@retval EFI_SUCCESS New PK enrolled successfully.
@retval EFI_INVALID_PARAMETER The parameter is invalid.
@@ -477,12 +592,6 @@ EnrollPlatformKey (
UINT32 Attr;
UINTN DataSize;
EFI_SIGNATURE_LIST *PkCert;
- UINT16* FilePostFix;
- UINTN NameLength;
-
- if (Private->FileContext->FileName == NULL) {
- return EFI_INVALID_PARAMETER;
- }

PkCert = NULL;

@@ -491,21 +600,6 @@ EnrollPlatformKey (
return Status;
}

- //
- // Parse the file's postfix. Only support DER encoded X.509 certificate files.
- //
- NameLength = StrLen (Private->FileContext->FileName);
- if (NameLength <= 4) {
- return EFI_INVALID_PARAMETER;
- }
- FilePostFix = Private->FileContext->FileName + NameLength - 4;
- if (!IsDerEncodeCertificate(FilePostFix)) {
- DEBUG ((EFI_D_ERROR, "Unsupported file type, only DER encoded
certificate (%s) is supported.", mSupportX509Suffix));
- return EFI_INVALID_PARAMETER;
- }
- DEBUG ((EFI_D_INFO, "FileName= %s\n",
Private->FileContext->FileName));
- DEBUG ((EFI_D_INFO, "FilePostFix = %s\n", FilePostFix));
-
//
// Prase the selected PK file and generate PK certificate list.
//
@@ -4300,12 +4394,14 @@ SecureBootCallback (
UINT16 *FilePostFix;
SECUREBOOT_CONFIG_PRIVATE_DATA *PrivateData;
BOOLEAN GetBrowserDataResult;
+ ENROLL_KEY_ERROR EnrollKeyErrorCode;

Status = EFI_SUCCESS;
SecureBootEnable = NULL;
SecureBootMode = NULL;
SetupMode = NULL;
File = NULL;
+ EnrollKeyErrorCode = None_Error;

if ((This == NULL) || (Value == NULL) || (ActionRequest == NULL)) {
return EFI_INVALID_PARAMETER;
@@ -4718,18 +4814,35 @@ SecureBootCallback (
}
break;
case KEY_VALUE_SAVE_AND_EXIT_PK:
- Status = EnrollPlatformKey (Private);
+ //
+ // Check the suffix, encode type and the key strength of PK certificate.
+ //
+ Status = CheckX509Certificate (Private->FileContext,
&EnrollKeyErrorCode);
+ if (EFI_ERROR (Status)) {
+ if (EnrollKeyErrorCode != None_Error && EnrollKeyErrorCode <
Enroll_Error_Max) {
+ CreatePopUp (
+ EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
+ &Key,
+ mX509EnrollPromptTitle[EnrollKeyErrorCode],
+ mX509EnrollPromptString[EnrollKeyErrorCode],
+ NULL
+ );
+ break;
+ }
+ } else {
+ Status = EnrollPlatformKey (Private);
+ }
if (EFI_ERROR (Status)) {
UnicodeSPrint (
PromptString,
sizeof (PromptString),
- L"Only DER encoded certificate file (%s) is supported.",
- mSupportX509Suffix
+ L"Error status: %x.",
+ Status
);
CreatePopUp (
EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
&Key,
- L"ERROR: Unsupported file type!",
+ L"ERROR: Enrollment failed!",
PromptString,
NULL
);
diff --git
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.h
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.h
index 1fafae07ac..268f015e8e 100644
---
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igI
mpl.h
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
+++ Co
+++ nfigImpl.h
@@ -93,6 +93,27 @@ extern EFI_IFR_GUID_LABEL *mEndLabel;
#define HASHALG_RAW 0x00000004
#define HASHALG_MAX 0x00000004

+//
+// Certificate public key minimum size (bytes) //
+#define CER_PUBKEY_MIN_SIZE 256
+
+//
+// Types of errors may occur during certificate enrollment.
+//
+typedef enum {
+ None_Error = 0,
+ //
+ // Unsupported_type indicates the certificate type is not supported.
+ //
+ Unsupported_Type,
+ //
+ // Unqualified_key indicates the key strength of certificate is not
+ // strong enough.
+ //
+ Unqualified_Key,
+ Enroll_Error_Max
+}ENROLL_KEY_ERROR;

typedef struct {
UINTN Signature;
--
2.31.1.windows.1


回复: [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

8001 - 8020 of 82317