Date   

[PATCH 3/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP if possible

Ard Biesheuvel
 

Use a pool allocation for the RSDP ACPI root pointer structure if no
memory limit is in effect that forces us to use page based allocation,
which may be wasteful if they get rounded up to 64 KB as is the case
on AArch64.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 30 ++++++++++++++------
1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
index 22f49a8489e7..fb939aa00f49 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
@@ -1737,19 +1737,26 @@ AcpiTableAcpiTableConstructor (
RsdpTableSize += sizeof (EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_POINTER);
}

- PageAddress = 0xFFFFFFFF;
- Status = gBS->AllocatePages (
- mAcpiTableAllocType,
- EfiACPIReclaimMemory,
- EFI_SIZE_TO_PAGES (RsdpTableSize),
- &PageAddress
- );
+ if (mAcpiTableAllocType != AllocateAnyPages) {
+ PageAddress = 0xFFFFFFFF;
+ Status = gBS->AllocatePages (
+ mAcpiTableAllocType,
+ EfiACPIReclaimMemory,
+ EFI_SIZE_TO_PAGES (RsdpTableSize),
+ &PageAddress
+ );
+ Pointer = (UINT8 *)(UINTN)PageAddress;
+ } else {
+ Status = gBS->AllocatePool (
+ EfiACPIReclaimMemory,
+ RsdpTableSize,
+ (VOID **)&Pointer);
+ }

if (EFI_ERROR (Status)) {
return EFI_OUT_OF_RESOURCES;
}

- Pointer = (UINT8 *) (UINTN) PageAddress;
ZeroMem (Pointer, RsdpTableSize);

AcpiTableInstance->Rsdp1 = (EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_POINTER *) Pointer;
@@ -1797,7 +1804,12 @@ AcpiTableAcpiTableConstructor (
}

if (EFI_ERROR (Status)) {
- gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize));
+ if (mAcpiTableAllocType != AllocateAnyPages) {
+ gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1,
+ EFI_SIZE_TO_PAGES (RsdpTableSize));
+ } else {
+ gBS->FreePool (AcpiTableInstance->Rsdp1);
+ }
return EFI_OUT_OF_RESOURCES;
}

--
2.17.1


[PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible

Ard Biesheuvel
 

If no memory allocation limit is in effect for ACPI tables, prefer
pool allocations over page allocations, to avoid wasting memory on
systems where page based allocations are rounded up to 64 KB, such
as AArch64.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 111 ++++++++++++--------
1 file changed, 65 insertions(+), 46 deletions(-)

diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
index b05e57e6584f..22f49a8489e7 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
@@ -355,28 +355,35 @@ ReallocateAcpiTableBuffer (
NewMaxTableNumber * sizeof (UINT32);
}

- //
- // Allocate memory in the lower 32 bit of address range for
- // compatibility with ACPI 1.0 OS.
- //
- // This is done because ACPI 1.0 pointers are 32 bit values.
- // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses.
- // There is no architectural reason these should be below 4GB, it is purely
- // for convenience of implementation that we force memory below 4GB.
- //
- PageAddress = 0xFFFFFFFF;
- Status = gBS->AllocatePages (
- mAcpiTableAllocType,
- EfiACPIReclaimMemory,
- EFI_SIZE_TO_PAGES (TotalSize),
- &PageAddress
- );
+ if (mAcpiTableAllocType != AllocateAnyPages) {
+ //
+ // Allocate memory in the lower 32 bit of address range for
+ // compatibility with ACPI 1.0 OS.
+ //
+ // This is done because ACPI 1.0 pointers are 32 bit values.
+ // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses.
+ // There is no architectural reason these should be below 4GB, it is purely
+ // for convenience of implementation that we force memory below 4GB.
+ //
+ PageAddress = 0xFFFFFFFF;
+ Status = gBS->AllocatePages (
+ mAcpiTableAllocType,
+ EfiACPIReclaimMemory,
+ EFI_SIZE_TO_PAGES (TotalSize),
+ &PageAddress
+ );
+ Pointer = (UINT8 *)(UINTN)PageAddress;
+ } else {
+ Status = gBS->AllocatePool (
+ EfiACPIReclaimMemory,
+ TotalSize,
+ (VOID **)&Pointer);
+ }

if (EFI_ERROR (Status)) {
return EFI_OUT_OF_RESOURCES;
}

- Pointer = (UINT8 *) (UINTN) PageAddress;
ZeroMem (Pointer, TotalSize);

AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer;
@@ -406,21 +413,26 @@ ReallocateAcpiTableBuffer (
}
CopyMem (AcpiTableInstance->Xsdt, TempPrivateData.Xsdt, (sizeof (EFI_ACPI_DESCRIPTION_HEADER) + mEfiAcpiMaxNumTables * sizeof (UINT64)));

- //
- // Calculate orignal ACPI table buffer size
- //
- TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT
- mEfiAcpiMaxNumTables * sizeof (UINT64);
+ if (mAcpiTableAllocType != AllocateAnyPages) {
+ //
+ // Calculate orignal ACPI table buffer size
+ //
+ TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT
+ mEfiAcpiMaxNumTables * sizeof (UINT64);

- if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) {
- TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT
- mEfiAcpiMaxNumTables * sizeof (UINT32) +
- sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT
- mEfiAcpiMaxNumTables * sizeof (UINT32);
+ if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) {
+ TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT
+ mEfiAcpiMaxNumTables * sizeof (UINT32) +
+ sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT
+ mEfiAcpiMaxNumTables * sizeof (UINT32);
+ }
+
+ gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1,
+ EFI_SIZE_TO_PAGES (TotalSize));
+ } else {
+ gBS->FreePool (TempPrivateData.Rsdt1);
}

- gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, EFI_SIZE_TO_PAGES (TotalSize));
-
//
// Update the Max ACPI table number
//
@@ -1759,29 +1771,36 @@ AcpiTableAcpiTableConstructor (
mEfiAcpiMaxNumTables * sizeof (UINT32);
}

- //
- // Allocate memory in the lower 32 bit of address range for
- // compatibility with ACPI 1.0 OS.
- //
- // This is done because ACPI 1.0 pointers are 32 bit values.
- // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses.
- // There is no architectural reason these should be below 4GB, it is purely
- // for convenience of implementation that we force memory below 4GB.
- //
- PageAddress = 0xFFFFFFFF;
- Status = gBS->AllocatePages (
- mAcpiTableAllocType,
- EfiACPIReclaimMemory,
- EFI_SIZE_TO_PAGES (TotalSize),
- &PageAddress
- );
+ if (mAcpiTableAllocType != AllocateAnyPages) {
+ //
+ // Allocate memory in the lower 32 bit of address range for
+ // compatibility with ACPI 1.0 OS.
+ //
+ // This is done because ACPI 1.0 pointers are 32 bit values.
+ // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses.
+ // There is no architectural reason these should be below 4GB, it is purely
+ // for convenience of implementation that we force memory below 4GB.
+ //
+ PageAddress = 0xFFFFFFFF;
+ Status = gBS->AllocatePages (
+ mAcpiTableAllocType,
+ EfiACPIReclaimMemory,
+ EFI_SIZE_TO_PAGES (TotalSize),
+ &PageAddress
+ );
+ Pointer = (UINT8 *)(UINTN)PageAddress;
+ } else {
+ Status = gBS->AllocatePool (
+ EfiACPIReclaimMemory,
+ TotalSize,
+ (VOID **)&Pointer);
+ }

if (EFI_ERROR (Status)) {
gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize));
return EFI_OUT_OF_RESOURCES;
}

- Pointer = (UINT8 *) (UINTN) PageAddress;
ZeroMem (Pointer, TotalSize);

AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer;
--
2.17.1


[PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible

Ard Biesheuvel
 

On AArch64 systems, page based allocations for memory types that are
relevant to the OS are rounded up to 64 KB multiples. This wastes
some space in the ACPI table memory allocator, since it uses page
based allocations in order to be able to place the ACPI tables low
in memory.

Since the latter requirement does not exist on AArch64, switch to pool
allocations for all ACPI tables except the root tables if the active
allocation policy permits them to be anywhere in memory. The root
tables will be handled in a subsequent patch.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 +-
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c | 4 +-
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 75 ++++++++++++++------
3 files changed, 57 insertions(+), 26 deletions(-)

diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
index 425a462fd92b..6e600e7acd24 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
@@ -64,9 +64,9 @@ typedef struct {
LIST_ENTRY Link;
EFI_ACPI_TABLE_VERSION Version;
EFI_ACPI_COMMON_HEADER *Table;
- EFI_PHYSICAL_ADDRESS PageAddress;
- UINTN NumberOfPages;
+ UINTN TableSize;
UINTN Handle;
+ BOOLEAN PoolAllocation;
} EFI_ACPI_TABLE_LIST;

//
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
index b1cba20c8ed7..14ced68e645f 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
@@ -68,8 +68,8 @@ FindTableByBuffer (

while (CurrentLink != StartLink) {
CurrentTableList = EFI_ACPI_TABLE_LIST_FROM_LINK (CurrentLink);
- if (((UINTN)CurrentTableList->PageAddress <= (UINTN)Buffer) &&
- ((UINTN)CurrentTableList->PageAddress + EFI_PAGES_TO_SIZE(CurrentTableList->NumberOfPages) > (UINTN)Buffer)) {
+ if (((UINTN)CurrentTableList->Table <= (UINTN)Buffer) &&
+ ((UINTN)CurrentTableList->Table + CurrentTableList->TableSize > (UINTN)Buffer)) {
//
// Good! Found Table.
//
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
index ad7baf8205b4..b05e57e6584f 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
@@ -428,6 +428,26 @@ ReallocateAcpiTableBuffer (
return EFI_SUCCESS;
}

+/**
+ Free the memory associated with provided the EFI_ACPI_TABLE_LIST instance
+
+ @param TableEntry EFI_ACPI_TABLE_LIST instance pointer
+
+**/
+STATIC
+VOID
+FreeTableMemory (
+ EFI_ACPI_TABLE_LIST *TableEntry
+ )
+{
+ if (TableEntry->PoolAllocation) {
+ gBS->FreePool (TableEntry->Table);
+ } else {
+ gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TableEntry->Table,
+ EFI_SIZE_TO_PAGES (TableEntry->TableSize));
+ }
+}
+
/**
This function adds an ACPI table to the table list. It will detect FACS and
allocate the correct type of memory and properly align the table.
@@ -454,14 +474,15 @@ AddTableToList (
OUT UINTN *Handle
)
{
- EFI_STATUS Status;
- EFI_ACPI_TABLE_LIST *CurrentTableList;
- UINT32 CurrentTableSignature;
- UINT32 CurrentTableSize;
- UINT32 *CurrentRsdtEntry;
- VOID *CurrentXsdtEntry;
- UINT64 Buffer64;
- BOOLEAN AddToRsdt;
+ EFI_STATUS Status;
+ EFI_ACPI_TABLE_LIST *CurrentTableList;
+ UINT32 CurrentTableSignature;
+ UINT32 CurrentTableSize;
+ UINT32 *CurrentRsdtEntry;
+ VOID *CurrentXsdtEntry;
+ EFI_PHYSICAL_ADDRESS AllocPhysAddress;
+ UINT64 Buffer64;
+ BOOLEAN AddToRsdt;

//
// Check for invalid input parameters
@@ -496,8 +517,8 @@ AddTableToList (
// There is no architectural reason these should be below 4GB, it is purely
// for convenience of implementation that we force memory below 4GB.
//
- CurrentTableList->PageAddress = 0xFFFFFFFF;
- CurrentTableList->NumberOfPages = EFI_SIZE_TO_PAGES (CurrentTableSize);
+ AllocPhysAddress = 0xFFFFFFFF;
+ CurrentTableList->TableSize = CurrentTableSize;

//
// Allocation memory type depends on the type of the table
@@ -518,9 +539,22 @@ AddTableToList (
Status = gBS->AllocatePages (
AllocateMaxAddress,
EfiACPIMemoryNVS,
- CurrentTableList->NumberOfPages,
- &CurrentTableList->PageAddress
+ EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
+ &AllocPhysAddress
);
+ CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *)(UINTN)AllocPhysAddress;
+ } else if (mAcpiTableAllocType == AllocateAnyPages) {
+ //
+ // If there is no allocation limit, there is also no need to use page
+ // based allocations for ACPI tables, which may be wasteful on platforms
+ // such as AArch64 that allocate multiples of 64 KB
+ //
+ Status = gBS->AllocatePool (
+ EfiACPIReclaimMemory,
+ CurrentTableList->TableSize,
+ (VOID **)&CurrentTableList->Table
+ );
+ CurrentTableList->PoolAllocation = TRUE;
} else {
//
// All other tables are ACPI reclaim memory, no alignment requirements.
@@ -528,9 +562,10 @@ AddTableToList (
Status = gBS->AllocatePages (
mAcpiTableAllocType,
EfiACPIReclaimMemory,
- CurrentTableList->NumberOfPages,
- &CurrentTableList->PageAddress
+ EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
+ &AllocPhysAddress
);
+ CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *)(UINTN)AllocPhysAddress;
}
//
// Check return value from memory alloc.
@@ -539,10 +574,6 @@ AddTableToList (
gBS->FreePool (CurrentTableList);
return EFI_OUT_OF_RESOURCES;
}
- //
- // Update the table pointer with the allocated memory start
- //
- CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *) (UINTN) CurrentTableList->PageAddress;

//
// Initialize the table contents
@@ -575,7 +606,7 @@ AddTableToList (
if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && AcpiTableInstance->Fadt1 != NULL) ||
((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && AcpiTableInstance->Fadt3 != NULL)
) {
- gBS->FreePages (CurrentTableList->PageAddress, CurrentTableList->NumberOfPages);
+ FreeTableMemory (CurrentTableList);
gBS->FreePool (CurrentTableList);
return EFI_ACCESS_DENIED;
}
@@ -729,7 +760,7 @@ AddTableToList (
if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && AcpiTableInstance->Facs1 != NULL) ||
((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && AcpiTableInstance->Facs3 != NULL)
) {
- gBS->FreePages (CurrentTableList->PageAddress, CurrentTableList->NumberOfPages);
+ FreeTableMemory (CurrentTableList);
gBS->FreePool (CurrentTableList);
return EFI_ACCESS_DENIED;
}
@@ -813,7 +844,7 @@ AddTableToList (
if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && AcpiTableInstance->Dsdt1 != NULL) ||
((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && AcpiTableInstance->Dsdt3 != NULL)
) {
- gBS->FreePages (CurrentTableList->PageAddress, CurrentTableList->NumberOfPages);
+ FreeTableMemory (CurrentTableList);
gBS->FreePool (CurrentTableList);
return EFI_ACCESS_DENIED;
}
@@ -1449,7 +1480,7 @@ DeleteTable (
//
// Free the Table
//
- gBS->FreePages (Table->PageAddress, Table->NumberOfPages);
+ FreeTableMemory (Table);
RemoveEntryList (&(Table->Link));
gBS->FreePool (Table);
}
--
2.17.1


[PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables

Ard Biesheuvel
 

Currently, the AcpiTableDxe memory allocator uses page based allocations,
for which the only reason seems to be that it permits the use of a memory
limit, which is necessary for ACPI 1.0 tables that need to reside in the
first 4 GB of memory.

That requirement does not exist on AArch64, and since page based allocations
are rounded up to 64 KB multiples, this wastes some memory in a way that
can easily be avoided. So let's use the existing 'mAcpiTableAllocType'
policy variable, and switch to pool allocations if it is set to
'AllocateAnyPages'

Example output from Linux booting on ArmVirtQemu:

Before:
ACPI: RSDP 0x0000000078510000 000024 (v02 BOCHS )
ACPI: XSDT 0x0000000078500000 00004C (v01 BOCHS BXPCFACP 00000001 01000013)
ACPI: FACP 0x00000000784C0000 00010C (v05 BOCHS BXPCFACP 00000001 BXPC 00000001)
ACPI: DSDT 0x00000000784D0000 0014BB (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001)
ACPI: APIC 0x00000000784B0000 0000A8 (v03 BOCHS BXPCAPIC 00000001 BXPC 00000001)
ACPI: GTDT 0x00000000784A0000 000060 (v02 BOCHS BXPCGTDT 00000001 BXPC 00000001)
ACPI: MCFG 0x0000000078490000 00003C (v01 BOCHS BXPCMCFG 00000001 BXPC 00000001)
ACPI: SPCR 0x0000000078480000 000050 (v02 BOCHS BXPCSPCR 00000001 BXPC 00000001)

After:
ACPI: RSDP 0x000000007C030018 000024 (v02 BOCHS )
ACPI: XSDT 0x000000007C03FE98 00004C (v01 BOCHS BXPCFACP 00000001 01000013)
ACPI: FACP 0x000000007C03FA98 00010C (v05 BOCHS BXPCFACP 00000001 BXPC 00000001)
ACPI: DSDT 0x000000007C037518 0014BB (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001)
ACPI: APIC 0x000000007C03FC18 0000A8 (v03 BOCHS BXPCAPIC 00000001 BXPC 00000001)
ACPI: GTDT 0x000000007C03FD18 000060 (v02 BOCHS BXPCGTDT 00000001 BXPC 00000001)
ACPI: MCFG 0x000000007C03FE18 00003C (v01 BOCHS BXPCMCFG 00000001 BXPC 00000001)
ACPI: SPCR 0x000000007C03FF98 000050 (v02 BOCHS BXPCSPCR 00000001 BXPC 00000001)

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>

Ard Biesheuvel (3):
MdeModulePkg/AcpiTableDxe: use pool allocations when possible
MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if
possible
MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP if possible

.../Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 +-
.../Universal/Acpi/AcpiTableDxe/AcpiSdt.c | 4 +-
.../Acpi/AcpiTableDxe/AcpiTableProtocol.c | 216 +++++++++++-------
3 files changed, 143 insertions(+), 81 deletions(-)

--
2.17.1


Re: [PATCH v4 0/6] Extend Last Attempt Status Usage

Michael D Kinney
 

Hi Michael,

Thank you for the updates. A couple minor comments:

1) FmpDevicePkg/Library/FmpDeviceLibNull

In order to demonstrate the preferred implementation I think we should
have FmpDeviceCheckImage() call FmpDeviceCheckImageWithStatus() and
FmpDeviceSetImage() call FmpDeviceSetImageWithStatus(). This way, it
will be clear to FmpDeviceLib developers that they only need to
implement the *WithStatus() version.

2) FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h

FmpDependencyCheckLib comment block should use /// instead of // to be
consistent with the rest of the include file.

Thanks,

Mike

-----Original Message-----
From: Michael Kubacki <michael.kubacki@outlook.com>
Sent: Thursday, October 15, 2020 2:20 PM
To: devel@edk2.groups.io
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>;
Xu, Wei6 <wei6.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: Re: [PATCH v4 0/6] Extend Last Attempt Status Usage

Hi all,

It's been about two weeks since this email and I still haven't seen any
feedback on v4.

I made a very small update that resulted in a v5 series today:
https://edk2.groups.io/g/devel/message/66287

If you could please provide timely feedback on v5 it would be appreciated.

Also, there's an issue building FmpDevicePkg at the moment that you'll
need this patch to fix:
https://edk2.groups.io/g/devel/message/66286

That patch needs review as well.

Thanks,
Michael

On 10/2/2020 9:26 AM, Michael Kubacki wrote:
It is going on a week now and I haven't seen a response to this patch
series yet. Please review it when possible.

On a somewhat related note, I made the changes that should be necessary
in edk2-platforms for backward compatibility in this patch:
https://edk2.groups.io/g/devel/message/65821

Thanks,
Michael

On 9/25/2020 7:19 PM, michael.kubacki@outlook.com wrote:
From: Michael Kubacki <michael.kubacki@microsoft.com>

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

This patch series adds more granularity to Last Attempt Status
codes reported during FMP check image and set image operations
that greatly improve precision of the status codes.

The unsuccessful vendor range (0x1000 - 0x4000) was introduced
in UEFI Specification 2.8. At a high-level, two subranges are
defined within that range in this patch series:
   1. The FMP Reserved range - reserved for components implemented
      in FmpDevicePkg.
   2. The FMP Device Library Reserved range - reserved for
      FmpDeviceLib instance-specific usage.

The ranges are described in a public header file LastAttemptStatus.h
while the specific codes used within FmpDevicePkg implementation
are defined in a private header file FmpLastAttemptStatus.h.

FmpDeviceLib instances should use the range definition from the
public header file to define Last Attempt Status codes local to
their library instance.

Of note, there's multiple approaches to assigning private status
codes in the FMP Reserved range. For example, individual components
could define their last attempt status codes locally with the
range allocated to the component defined in a package-wide private
header file. However, one goal of the granularity being introduced
is to provide straightforward traceability to an error source.

For that reason, it was chosen to define a constant set of codes at
the package level in FmpLastAttemptStatus.h. For example, if a new
FmpDependencyLib instance is added, it would not be able to reassign
status code values in the pre-existing FMP Dependency range; it
would reuse codes for the same error source and be able to add new
codes onto the range for its usage.

V4 changes:
   1. Simplified range value definitions in LastAttemptStatus.h.
      Directly assign the values in the macro definition instead
      of using calculations.
   2. Adjusted range sizes to leave more room for future expansion.

      OLD:
      START     | END       | Usage
      ------------------------------------------------|
      0x1000    | 0x1FFF    | FmpDevicePkg            |
         0x1000 |    0x107F | FmpDxe driver           |
         0x1080 |    0x109F | FMP dependency Libs     |
      0x2000    | 0x3FFF    | FmpDeviceLib instances  |

      NEW:
      START     | END       | Usage
      ----------------------------------------------------------------|
      0x1000    | 0x17FF    | FmpDevicePkg                            |
         0x1000 |    0x107F | FmpDxe driver                           |
         0x1080 |    0x109F | FmpDependencyLib                        |
         0x10A0 |    0x10BF | FmpDependencyCheckLib                   |
         0x10C0 |    0x17FF | Unused. Available for future expansion. |
      0x1800    | 0x1FFF    | FmpDeviceLib instances implementation   |
      0x2000    | 0x3FFF    | Unused. Available for future expansion. |

   3. Broke the single range in v3 for FMP Dependency libraries into
      separate ranges.
   4. Clarified LastAttemptStatus return values in each function
      description.
   5. Returned an expected LastAttemptStatus value for some functions
      that previously did not return a value.
   6. Reverted changes in FmpDxe to call the new FmpDeviceLib APIs
      for FmpDeviceCheckImage () and FmpDeviceSetImage (). These will
      be added in a future series after impacted platforms in
      edk2-platforms are updated to use the new APIs.
   7. Instead of directly changing the pre-existing APIs in
      FmpDeviceLib to add a LastAttemptStatus parameter, the new
      functions were added to the library interface:
        * FmpDeviceCheckImageWithStatus ()
        * FmpDeviceSetImageWithStatus ()

V3 changes:
   1. Enhanced range definitions in LastAttemptStatus.h with more
      completeness providing length, min, and max values.
   2. Moved the actual Last Attempt Status code assignments to a
      private header file PrivateInclude/FmpLastAttemptStatus.h.
   3. Changed the value of
      LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
      to 0x3FFF instead of 0x4000 even though 0x4000 is defined in
      the UEFI specification. The length is 0x4000 but the max
      allowed value should be 0x3FFF. This change was made now to
      prevent implementation compatibility issues in the future.
   4. Included "DEVICE" in the following macro name to clearly
      associate it with the FmpDeviceLib library class:
      LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_xxx
   5. Included a map to help the reader better visualize the range
      definitions in LastAttemptStatus.h.
   6. Included additional documentation describing the enum in
      FmpLastAttemptStatus.h. An explicit statement stating that new
      codes should be added onto the end of ranges to preserve the
      values was added.
   7. Simplified error handling logic in FmpDxe for FmpDeviceLib
      calls that return Last Attempt Status.
   8. V2 had a single memory allocation failure code used for
      different memory allocations in CheckFmpDependency () in
      FmpDependencyLib. Each potential allocation failure was
      assigned a unique code.

V2 changes:
   1. Consolidate all previous incremental updates to
      LastAttemptStatus.h into one patch (patch 2)
   2. Move LastAttemptStatus.h from Include to PrivateInclude
   3. Correct patch 1 subject from "FmpDevicePkg" to "MdePkg"

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Michael Kubacki (6):
   MdePkg/SystemResourceTable.h: Add vendor range values
   FmpDevicePkg: Add Last Attempt Status header files
   FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status
     capability
   FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status
     granularity
   FmpDevicePkg: Add Last Attempt Status support to dependency libs
   FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API


FmpDevicePkg/FmpDxe/FmpDxe.c
| 146 +++++++++++++++++---

FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
|  39 ++++--

FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c
|  14 +-

FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
|  93 +++++++++++--

FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
| 132 +++++++++++++++++-

FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
|   7 +-

FmpDevicePkg/FmpDxe/FmpDxe.h
|   4 +-

FmpDevicePkg/Include/LastAttemptStatus.h
|  81 +++++++++++

FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h
|   8 +-

FmpDevicePkg/Include/Library/FmpDependencyLib.h
|  44 ++++--

FmpDevicePkg/Include/Library/FmpDeviceLib.h
| 121 +++++++++++++++-

FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
|  81 +++++++++++

MdePkg/Include/Guid/SystemResourceTable.h
|  13 ++
  13 files changed, 718 insertions(+), 65 deletions(-)
  create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
  create mode 100644 FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h


Re: [PATCH v1 1/1] FmpDevicePkg: Add RngLib instance to DSC

Michael D Kinney
 

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

-----Original Message-----
From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
Sent: Thursday, October 15, 2020 1:43 PM
To: devel@edk2.groups.io
Cc: Jiang, Guomin <guomin.jiang@intel.com>; Xu, Wei6 <wei6.xu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Kinney, Michael D
<michael.d.kinney@intel.com>; Matthew Carlson <matthewfcarlson@gmail.com>
Subject: [PATCH v1 1/1] FmpDevicePkg: Add RngLib instance to DSC

From: Michael Kubacki <michael.kubacki@microsoft.com>

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

Commit b5701a4 in CryptoPkg introduced a dependency on RngLib.

The FmpDevicePkg build currently fails since it does not specify
a RngLib instance and OpensslLib links against RngLib.

Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Matthew Carlson <matthewfcarlson@gmail.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
FmpDevicePkg/FmpDevicePkg.dsc | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/FmpDevicePkg/FmpDevicePkg.dsc b/FmpDevicePkg/FmpDevicePkg.dsc
index bdb73f28288f..cfeadd833038 100644
--- a/FmpDevicePkg/FmpDevicePkg.dsc
+++ b/FmpDevicePkg/FmpDevicePkg.dsc
@@ -6,7 +6,7 @@
# Capsules. The behavior of the Firmware Management Protocol instance is
# customized using libraries and PCDs.
#
-# Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
+# Copyright (c) Microsoft Corporation.<BR>
# Copyright (c) 2018 - 2020, Intel Corporation. All rights reserved.<BR>
# Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
#
@@ -57,6 +57,7 @@ [LibraryClasses]
IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+ RngLib|MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
!endif
FmpAuthenticationLib|SecurityPkg/Library/FmpAuthenticationLibPkcs7/FmpAuthenticationLibPkcs7.inf
CapsuleUpdatePolicyLib|FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull.inf
--
2.28.0.windows.1


Re: Issues with edk2 compilation with xcode5 on mac OS - files compiled but strange behaviors

Daniele Crudo
 

mmm..I think I won't be able to build edk2 with a different clang toolchain, sorry from what I read it seems too far for my knowledge.
Since as you pointed out this should be related to optimization taking place for the release version, is there a way, if you know and if it's possible, to disable optimization, or set different optimization levels when building edk2 with the actual clang?


Re: [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

Lee, Terry <terry.lee@...>
 

Jiewen,

I tested this patch on HPE Superdome Flex with both Linux and Windows.

Terry

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Marc-André Lureau <marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

Hello
Is there any one can share the information on what test has been done for this ?

Thank you
Yao Jiewen

-----Original Message-----
From: Lee, Terry <terry.lee@hpe.com>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com;
Gao, Zhichao <zhichao.gao@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Marc-
André Lureau <marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

Could the package maintainer merge this patch? Thanks.

Terry

-----Original Message-----
From: Stefan Berger [mailto:stefanb@linux.ibm.com]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Lee, Terry <terry.lee@hpe.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Zhang,
Chao B <chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

On 7/10/20 9:53 AM, Stefan Berger wrote:
On 7/10/20 1:43 AM, Laszlo Ersek wrote:
(+Marc-André, Stefan)

On 07/10/20 02:44, Gao, Zhichao wrote:
This bug is not obeserved by me. But I view the code. The
condition is incorrect and it would affect the TCG operation:
if (!mIsTcg2PPVerLowerThan_1_3) {
if (OperationRequest <
TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
//
// TCG2 PP1.3 spec defined operations that are reserved
or un-implemented
//
return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
}
} else {
//
// TCG PP lower than 1.3. (1.0, 1.1, 1.2)
//
if (OperationRequest <=
TCG2_PHYSICAL_PRESENCE_NO_ACTION_MAX) {
RequestConfirmed = TRUE;
} else if (OperationRequest <
TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
}
}
I've found that code myself, but I'm not familiar enough with TPM
PPI stuff to understand immediately the effects of this change. I
can see that where we used to return
TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED before, we
could now
assign "RequestConfirmed = TRUE", and vice versa, due to
"mIsTcg2PPVerLowerThan_1_3" being potentially inverted.

But what does that *mean*? What is the behavioral change that human
end-users, or software components, will experience?

The above code snipped is located in a default branch of a large
switch statement that handles most of the common PPI operations
independent of this change, so that at least is good.

I would say that in the worst case some of the operations not
otherwise handled may have mistakenly failed or could have been
executed without user confirmation/interaction. On Linux at least
PPI requests can only be sent by root.

I am running a somewhat dated version of edk2 (Fedora 31). The
operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these
are individually handled in the switch statement, so there should no
be any impact. I am currently not aware of whether this list can be
extended with some sort of module.





Thanks
Laszlo

So I think it should be fixed.

Thanks,
Zhichao

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Laszlo Ersek
Sent: Thursday, July 9, 2020 6:02 PM
To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Terry Lee <terry.lee@hpe.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
Zhang, Chao B <chao.b.zhang@intel.com>
Subject: Re: [edk2-devel] [PATCH]
SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER
comparision

On 07/09/20 04:46, Gao, Zhichao wrote:
From: Terry Lee <terry.lee@hpe.com>

REF:
INVALID URI REMOVED
an
ocore.org_show-5Fbug.cgi-3Fid-
3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
LFWg&r=Jlc0Jxr620EZ-
CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
s-
W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
48
Bj8YhXhQAI&e=

Tcg2PhysicalPresenceLibConstructor set the module variable
mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version comparision.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git
a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
index 1c46d5e69d..8afaa0a785 100644
---
a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
+++
b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
+++ esenceLib.c
@@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor ( {
EFI_STATUS Status;

- if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8
*)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer),
sizeof(PP_INF_VERSION_1_2) - 1) <= 0) {
+ if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8
+*)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer),
+ sizeof(PP_INF_VERSION_1_2) - 1) >= 0) {
mIsTcg2PPVerLowerThan_1_3 = TRUE;
}

What is the practical impact of this bug / fix?

Thanks
Laszlo






Re: [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

Lee, Terry <terry.lee@...>
 

Jiewen,

I have only PP1.3 configuration. The only WHCK test failure is a known Windows issue that I believe is unrelated to PP.

Terry

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
Sent: Thursday, October 15, 2020 7:31 PM
To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Marc-André Lureau <marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

Thanks Terry.
I tend to give R-B. I read the code it seems no impact.

Would you please confirm you have tested both PP1.2 and PP1.3 configuration, with windows WHCK test pass?

Thank you
Yao Jiewen

-----Original Message-----
From: Lee, Terry <terry.lee@hpe.com>
Sent: Friday, October 16, 2020 10:25 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
<chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

Jiewen,

I tested this patch on HPE Superdome Flex with both Linux and Windows.

Terry

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
<chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

Hello
Is there any one can share the information on what test has been done
for this ?

Thank you
Yao Jiewen

-----Original Message-----
From: Lee, Terry <terry.lee@hpe.com>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com;
Gao, Zhichao <zhichao.gao@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
Marc- André Lureau <marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

Could the package maintainer merge this patch? Thanks.

Terry

-----Original Message-----
From: Stefan Berger [mailto:stefanb@linux.ibm.com]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Lee, Terry <terry.lee@hpe.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Zhang,
Chao B <chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

On 7/10/20 9:53 AM, Stefan Berger wrote:
On 7/10/20 1:43 AM, Laszlo Ersek wrote:
(+Marc-André, Stefan)

On 07/10/20 02:44, Gao, Zhichao wrote:
This bug is not obeserved by me. But I view the code. The
condition is incorrect and it would affect the TCG operation:
if (!mIsTcg2PPVerLowerThan_1_3) {
if (OperationRequest <
TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
//
// TCG2 PP1.3 spec defined operations that are
reserved or un-implemented
//
return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
}
} else {
//
// TCG PP lower than 1.3. (1.0, 1.1, 1.2)
//
if (OperationRequest <=
TCG2_PHYSICAL_PRESENCE_NO_ACTION_MAX) {
RequestConfirmed = TRUE;
} else if (OperationRequest <
TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
}
}
I've found that code myself, but I'm not familiar enough with TPM
PPI stuff to understand immediately the effects of this change. I
can see that where we used to return
TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED before, we
could now
assign "RequestConfirmed = TRUE", and vice versa, due to
"mIsTcg2PPVerLowerThan_1_3" being potentially inverted.

But what does that *mean*? What is the behavioral change that
human end-users, or software components, will experience?

The above code snipped is located in a default branch of a large
switch statement that handles most of the common PPI operations
independent of this change, so that at least is good.

I would say that in the worst case some of the operations not
otherwise handled may have mistakenly failed or could have been
executed without user confirmation/interaction. On Linux at least
PPI requests can only be sent by root.

I am running a somewhat dated version of edk2 (Fedora 31). The
operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these
are individually handled in the switch statement, so there should no
be any impact. I am currently not aware of whether this list can be
extended with some sort of module.





Thanks
Laszlo

So I think it should be fixed.

Thanks,
Zhichao

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Laszlo Ersek
Sent: Thursday, July 9, 2020 6:02 PM
To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Terry Lee <terry.lee@hpe.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
Zhang, Chao B <chao.b.zhang@intel.com>
Subject: Re: [edk2-devel] [PATCH]
SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER
comparision

On 07/09/20 04:46, Gao, Zhichao wrote:
From: Terry Lee <terry.lee@hpe.com>

REF:
INVALID URI REMOVED.
ti
an
ocore.org_show-5Fbug.cgi-3Fid-
3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
LFWg&r=Jlc0Jxr620EZ-
CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
s-
W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
48
Bj8YhXhQAI&e=

Tcg2PhysicalPresenceLibConstructor set the module variable
mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version
comparision.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c |
2
+-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git
a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
index 1c46d5e69d..8afaa0a785 100644
---
a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
+++
b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
+++ esenceLib.c
@@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor ( {
EFI_STATUS Status;

- if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8
*)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer),
sizeof(PP_INF_VERSION_1_2) - 1) <= 0) {
+ if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8
+*)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer),
+ sizeof(PP_INF_VERSION_1_2) - 1) >= 0) {
mIsTcg2PPVerLowerThan_1_3 = TRUE;
}

What is the practical impact of this bug / fix?

Thanks
Laszlo






Re: [edk2-rfc] [edk2-devel] [RFC] Support Both MM Traditional and Standalone Drivers with One MM Core

Siyuan, Fu
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
Biesheuvel
Sent: 2020年10月16日 15:04
To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io;
sami.mujawar@arm.com; lersek@redhat.com; Yao, Jiewen
<jiewen.yao@intel.com>; rfc@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Supreeth
Venkatesh <Supreeth.Venkatesh@arm.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Support Both MM Traditional and
Standalone Drivers with One MM Core

On 10/16/20 3:36 AM, Fu, Siyuan wrote:
Hi, Sami

I know the traditional MM is planned to be deprecated but the reality is that
there
are many existing traditional MM platforms/drivers and the migration has to
happen
step-by-step. It may take a long time like several years, not days or months.
Not sure
if we really want to maintain duplicate code in 3 different MM cores in EDK2
for
such a long time.
Could you explain more about the gap that needs to be bridged here? I
suppose the desire is to be able to reuse existing DXE_SMM_DRIVER
modules, and deploy them unmodified in a standalone MM context?
Yes you are correct. Without a hybrid MM core, the MM drivers must be
either all traditional MM or all standalone MM. While a platform may have
tens or even hundreds of traditional MM drivers for now, it's hard to covert
all of them into standalone as a single step. It looks ideal from architecture
perspective but actually very hard for a read product execution. So it will be
good if there is a hybrid MM core which can support both the unmodified
traditional MM driver, and converted standalone MM driver.


So would you expect runtime dispatch for these drivers? What about any
accesses to EFI boot services, which are no longer possible when running
under standalone MM? Do you have any reason to believe that this hybrid
MM core will be able to run a significant fraction of those existing
drivers?
Runtime dispatch for standalone MM driver can be supported, but runtime
dispatch for traditional MM driver is not required (actually prohibited).
The hybrid MM core can distinguish the type of an MM module and decide
if it can be dispatched at a given execution point.


Would you think it's acceptable if we put the traditional MM related code
controlled
by a feature flag PCD in StandaloneMmPkg core? The default value of the PCD
can
be set as disabled so those existing platforms doesn't need to be changed.
This is security critical code, and having PCD controlled behavior like
this makes it much hard to reason about correctness in all its
instantiation. I guess I would have to see what the code looks like, but
having PCD checks all over the place does not seem like a great way to
do this IMHO.
I understand your concern. So I assume we all understand the value for
having a hybrid MM core, but the question is whether adding to existing
StandaloneMmPkg Core or a 3rd MM core in EDK2, right? I think we can
leave this as an open and make decision when reviewing the patch.

Thanks.
Siyuan


--
Ard.





[PATCH] BaseTools: fix decoding issue in file operation

Wang, Jian J
 

The build tool reports failure upon file read, such as calling trim
to clean preprocessed source files, if the tool is running on OS with
non-western code-page and the source file has non-ascii characters.

Even if utf-8 has also problem when encountering some characters
encoded in cp1252 (such 0x92, 0x96, 0xa0, etc).

Currently, the safest way to read file in python code is using
'latin-1' (iso-8859-1) because it uses every byte between 00-FF
and then won't cause encoding/decoding issue. It behaves almost
the same as reading file in binary mode.=0D
=0D
cp1252 is similar to latin-1 but it doesn't support encoding '\x80'=0D
to '\xff' and doesn't support decoding following bytes:=0D
=0D
'\x81', '\x8d', '\x8f', '\x90', '\x9d'
=0D
So if there're utf-8/16 encoded characters in file, it will fail=0D
sometimes.=0D
=0D
Refer to following links for details:=0D
https://en.wikipedia.org/wiki/Latin-1_Supplement_(Unicode_block)=0D
https://en.wikipedia.org/wiki/Windows-1252=0D
https://kb.iu.edu/d/aepu=0D
https://www.i18nqa.com/debug/table-iso8859-1-vs-windows-1252.html=0D

One can use following python code to verify this.

for i in range(0x100):
try:
chr(i).encode('latin-1')
except:
print(" %s cannot encode %02x" % ('latin-1', i))

for i in range(0x100):
try:
b =3D bytes([i])
b.decode('latin-1')
except:
print(" %s cannot decode %02x" % ('latin-1', i))

This patch add code to enforce using 'latin-1' as encoding argument
of open() in function OpenLongFilePath(), if the open mode is for
text file only. This can solve the file decoding issue completely.
=0D
The possible related BZs:=0D
https://bugzilla.tianocore.org/show_bug.cgi?id=3D1434=0D
https://bugzilla.tianocore.org/show_bug.cgi?id=3D1637=0D
https://bugzilla.tianocore.org/show_bug.cgi?id=3D2578=0D
https://bugzilla.tianocore.org/show_bug.cgi?id=3D2709=0D
https://bugzilla.tianocore.org/show_bug.cgi?id=3D2829=0D

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
BaseTools/Source/Python/Common/LongFilePathSupport.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/Common/LongFilePathSupport.py b/BaseTo=
ols/Source/Python/Common/LongFilePathSupport.py
index 38c4396544..c8dce077f2 100644
--- a/BaseTools/Source/Python/Common/LongFilePathSupport.py
+++ b/BaseTools/Source/Python/Common/LongFilePathSupport.py
@@ -30,7 +30,8 @@ def LongFilePath(FileName):
# wrap open to support opening a long file path=0D
#=0D
def OpenLongFilePath(FileName, Mode=3D'r', Buffer=3D -1):=0D
- return open(LongFilePath(FileName), Mode, Buffer)=0D
+ Encoding =3D None if 'b' in Mode else 'latin-1'=0D
+ return open(LongFilePath(FileName), Mode, Buffer, Encoding)=0D
=0D
def CodecOpenLongFilePath(Filename, Mode=3D'rb', Encoding=3DNone, Errors=
=3D'strict', Buffering=3D1):=0D
return codecs.open(LongFilePath(Filename), Mode, Encoding, Errors, Buf=
fering)=0D
--=20
2.24.0.windows.2


[edk2-staging]: [PATCH] UnitTestFrameworkPkg: Add unit test for Authenticated Variables

Wadhawan, Divneil R
 

Hi all,

 

I am working on extending the digest algorithm for Authenticated Variables.

Existing Authenticated variables only support signing SHA256 digest.

The 2 patches below in github follows the edk2 code first approach.

 

The below patch is huge because of the test Authenticated Variables (.auth) header file being > 130 lines * 10 test cases.

I can push the patch in the mail for review, in case its recommended.

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2949

Patch: https://github.com/divneil/edk2-staging/pull/new/Bugzilla2949-Unit-Test-for-Authenticated-Variables-MdeModulePkg

 

The below patch modifies AuthService to allow SHA384/SHA512 digest algorithm.

I will send it for email review later, but in order to keep the related information together and easily accessible, I am sharing here for completeness.

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2950

Patch: https://github.com/divneil/edk2-staging/pull/new/Bugzilla2950-AuthService-extending-digest-algorithm-support

 

How to test the full feature with EmulatorPkg is detailed in: https://bugzilla.tianocore.org/show_bug.cgi?id=2950#c5

Thanks for your review.

 

Regards,

Divneil


Re: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed

Wu, Hao A
 

-----Original Message-----
From: Luo, Heng <heng.luo@intel.com>
Sent: Thursday, October 15, 2020 9:49 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is
failed

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

Thanks for the patch Heng.

After looking into the analysis at https://bugzilla.tianocore.org/show_bug.cgi?id=3007#c1:
|> Enable Slot Successfully, The Slot ID = 0x3
|> Address 3 assigned successfully
|> UsbEnumerateNewDev: hub port 15 is reset
|> UsbEnumerateNewDev: device is of 3 speed
|> UsbEnumerateNewDev: device uses translator (0, 0)
|> XhcControlTransfer: SlotId == 2 DeviceAddress=0

The wrong 'SlotId' is used for the control transfer command, where SlotId 2 is for the device failed to be initialized.
Heng, could you help to check whether it is possible for the next device to use SlotId 2 again? Thanks in advance.

Best Regards,
Hao Wu



Currently UsbDevContext is not cleaned up if USB slot initialization is failed,
the wrong context data will affect next USB devices and the USB devices can
not be enumerated.
Need to clean up UsbDevContext if USB slot initialization is failed.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Heng Luo <heng.luo@intel.com>
---
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 9cb115363c..1e8430ac34 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -2,7 +2,7 @@
XHCI transfer scheduling routines. -Copyright (c) 2011 - 2018, Intel
Corporation. All rights reserved.<BR>+Copyright (c) 2011 - 2020, Intel
Corporation. All rights reserved.<BR> Copyright (c) Microsoft
Corporation.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent @@ -2279,6
+2279,9 @@ XhcInitializeDeviceSlot (
DeviceAddress = (UINT8) ((DEVICE_CONTEXT *) OutputContext)-
Slot.DeviceAddress; DEBUG ((EFI_D_INFO, " Address %d assigned
successfully\n", DeviceAddress)); Xhc-
UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;+ } else {+ DEBUG
((DEBUG_INFO, " Address %d assigned unsuccessfully, clean up context
data.\n"));+ ZeroMem (&Xhc->UsbDevContext[SlotId], sizeof
(USB_DEV_CONTEXT)); } return Status;@@ -2489,7 +2492,11 @@
XhcInitializeDeviceSlot64 (
DeviceAddress = (UINT8) ((DEVICE_CONTEXT_64 *) OutputContext)-
Slot.DeviceAddress; DEBUG ((EFI_D_INFO, " Address %d assigned
successfully\n", DeviceAddress)); Xhc-
UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;+ } else {+ DEBUG
((DEBUG_INFO, " Address %d assigned unsuccessfully, clean up context
data.\n"));+ ZeroMem (&Xhc->UsbDevContext[SlotId], sizeof
(USB_DEV_CONTEXT)); }+ return Status; } --
2.24.0.windows.2


Re: [edk2-rfc] [edk2-devel] [RFC] Support Both MM Traditional and Standalone Drivers with One MM Core

Ard Biesheuvel
 

On 10/16/20 3:36 AM, Fu, Siyuan wrote:
Hi, Sami
I know the traditional MM is planned to be deprecated but the reality is that there
are many existing traditional MM platforms/drivers and the migration has to happen
step-by-step. It may take a long time like several years, not days or months. Not sure
if we really want to maintain duplicate code in 3 different MM cores in EDK2 for
such a long time.
Could you explain more about the gap that needs to be bridged here? I suppose the desire is to be able to reuse existing DXE_SMM_DRIVER modules, and deploy them unmodified in a standalone MM context?

So would you expect runtime dispatch for these drivers? What about any accesses to EFI boot services, which are no longer possible when running under standalone MM? Do you have any reason to believe that this hybrid MM core will be able to run a significant fraction of those existing drivers?

Would you think it's acceptable if we put the traditional MM related code controlled
by a feature flag PCD in StandaloneMmPkg core? The default value of the PCD can
be set as disabled so those existing platforms doesn't need to be changed.
This is security critical code, and having PCD controlled behavior like this makes it much hard to reason about correctness in all its instantiation. I guess I would have to see what the code looks like, but having PCD checks all over the place does not seem like a great way to do this IMHO.

--
Ard.


Re: [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Gary Lin
 

On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote:
On 10/12/20 09:22, Gao, Zhichao wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843

Revert the patch to change the block size in child handler. It would
block the CD (Eltorito) Hard disk media type's sub partition being
observed.
The blocksize patch used to fix the CD image's MBR table issue. The
CD MBR table would always be ignored because it would be handled
by the Eltorito partition handler first and never go into the MBR
handler. So directly revert it.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
index f10ce7c65b..473e091320 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
@@ -1149,8 +1149,8 @@ PartitionInstallChildHandle (

Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE;

- Private->Start = MultU64x32 (Start, BlockSize);
- Private->End = MultU64x32 (End + 1, BlockSize);
+ Private->Start = MultU64x32 (Start, ParentBlockIo->Media->BlockSize);
+ Private->End = MultU64x32 (End + 1, ParentBlockIo->Media->BlockSize);

Private->BlockSize = BlockSize;
Private->ParentBlockIo = ParentBlockIo;
@@ -1187,7 +1187,13 @@ PartitionInstallChildHandle (

Private->Media.IoAlign = 0;
Private->Media.LogicalPartition = TRUE;
- Private->Media.LastBlock = End - Start;
+ Private->Media.LastBlock = DivU64x32 (
+ MultU64x32 (
+ End - Start + 1,
+ ParentBlockIo->Media->BlockSize
+ ),
+ BlockSize
+ ) - 1;

Private->Media.BlockSize = (UINT32) BlockSize;

Hi Laszlo,

(1) Adding Gary Lin to the CC list.
Thanks for noticing me :)


(2) I can see that the TianoCore bugzilla ticket, namely
<https://bugzilla.tianocore.org/show_bug.cgi?id=2843>;, has been reopened.

That's wrong.

TianoCore#2843 was about calculating the starting and ending LBAs with
incorrect block sizes. It was fixed by commit e0eacd7daa6f. Therefore
TianoCore#2843 should stay in RESOLVED|FIXED status.

Now that we have realized that commit e0eacd7daa6f caused a regression,
a *new BZ* should be filed, stating the particular compatibility issue
(regression). It is a different symptom from the symptom originally
reported under TianoCore#2843, so it belongs in a different ticket.

In particular, the statement in
<https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8>; that the
original commit (which now should be reverted) "doesn't fix any specific
issue", is *completely wrong*. If you look at commit e0eacd7daa6f, it
contains the tag

Tested-by: Gary Lin <glin@suse.com>

Furthermore, if you look at the mailing list archive, you will find the
following confirmation from Gary:

After applying this patch series, the firmware recognizes
openSUSE/SUSE iso images again.

In the v1 thread at

[edk2-devel] [PATCH 0/3]
MdeModulePkg/PartitionDxe: Make the parition driver match the spec

https://edk2.groups.io/g/devel/message/63972
http://mid.mail-archive.com/20200811075443.GG21538@GaryWorkstation

And then, in the v2 thread, Gary wrote

I've tested the following ISO images and all booted as expected.
[...]

again giving a Tested-by:

[edk2-devel] [PATCH V2 0/3]
MdeModulePkg/PartitionDxe: Make the parition driver match the spec

https://edk2.groups.io/g/devel/message/64047
http://mid.mail-archive.com/20200812062652.GL21538@GaryWorkstation


Now that you are proposing a revert, you have missed all of the above
feedback from Gary. That's because you never bothered to link the v1 and
v2 mailing list threads into the bugzilla ticket.

So this patch risks reintroducing the issue that Gary reported originally.

(Of course, the original bug report from Gary is *also* not linked into
TianoCore#2843:

https://edk2.groups.io/g/devel/message/62648
https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6
http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation

so it's no wonder we have no idea whose use case we could regress with a
revert!)

This patch should *NOT* be merged until Gary confirms it's OK.

(And if it's not OK, then a solution is needed that fixes both Gary's
use case, and the compatibility regression. It might even need a PCD, if
there is media out there that needs one kind of logic, and other media
that needs the other kind of logic.)
I just tested the patch with the ISO files with SLE15-SP2, openSUSE Leap
15.2, Fedora 32, and ubuntu 20.04, and the VM loads them without any
problem, so there is no regression I had before.

I'd give it my Tested-by.

Tested-by: Gary Lin <glin@suse.com>

Gary Lin


(3) If this patch is a revert of commit e0eacd7daa6f, then the revert
should be prepared with the "git revert" command. In particular, the
commit message should very clearly state that this patch reverts commit
e0eacd7daa6f.


Thanks
Laszlo


Re: Issues with edk2 compilation with xcode5 on mac OS - files compiled but strange behaviors

Daniele Crudo
 

Hi, thank you Andrew.
The debug version doesn't have any issue, so the issue is only with release.
The mac version seems to skip tftpDynamicCommand.efi and LinuxInitrdDynamicShellCommand.efi (see attached debug logs), but it's not related to this and it's not related to the logo, because the debug version as I said it just works.
I will try another toolchain, just to have some time to figure out how to setup the environment.

Thank you

Il giorno ven 16 ott 2020 alle ore 04:11 Andrew Fish <afish@...> ha scritto:
Daniele,

The logo issue is due to the  [Hii-Binary-Package.UEFI_HII] build rule not being supported by XCODE and RVCT those toolchains don’t contain resource compilers for PE/COFF files so they can’t inject HII into PE/COFF images. I submitted patches to fix this a while back and they got stuck and I got bogged down on my day job. This reminds me I need to get those patches moving again.

Due to the issue above the XCODE build skips some drivers/apps in OVMF [1] but I don’t think that is your issue?

I don’t think it is likely that your graphics issue is related to HII resources. Most of the times when I track down the “it does not work with clang (Xcode uses clang)” it ends up being the clang compiler optimizing away undefined behavior that the other compilers ignored. I’m not sure if there is a another clang toolchain you could try? That would be helpful to know if it is likely an undefined behavior difference.

Unfortunately the graphics issue is going to be hard to track down if we can’t reproduce it locally.

[1] Easiest way to find them is:
/Volumes/Case/edk2-github(master)>git grep '$(TOOL_CHAIN_TAG) == "XCODE5"'
OvmfPkg/OvmfPkgIa32.dsc:252:!if $(TOOL_CHAIN_TAG) == "XCODE5"
OvmfPkg/OvmfPkgIa32X64.dsc:256:!if $(TOOL_CHAIN_TAG) == "XCODE5"
OvmfPkg/OvmfPkgX64.dsc:256:!if $(TOOL_CHAIN_TAG) == "XCODE5"
OvmfPkg/OvmfXen.dsc:233:!if $(TOOL_CHAIN_TAG) == "XCODE5"
UefiCpuPkg/UefiCpuPkg.dsc:63:!if $(TOOL_CHAIN_TAG) == "XCODE5"

Thanks,

Andrew Fish

On Oct 15, 2020, at 8:22 AM, crudo.daniele@... wrote:

Good morning, I have an issue with compiled OVMF_CODE.fd and OVMF_VARS.fd with xcode5 on mac os - Catalina 10.15.7 (I tried v. 202005 and v. 202008 stable releases from github).
My commands for v. 202008, on mac OS:

git clone https://github.com/tianocore/edk2.git cd edk2 git clean -ffdx git reset --hard git submodule deinit --force --all git checkout edk2-stable202008 git submodule update --init --force source edksetup.sh nice make -C "$EDK_TOOLS_PATH" -j $(getconf _NPROCESSORS_ONLN) build -a X64 -b RELEASE -p OvmfPkg/OvmfPkgX64.dsc -t XCODE5

Files are compiled and saved (apparently correctly).
And I'm using OVMF_CODE.fd and OVMF_VARS.fd to boot a mac OS vm with QEMU/KVM.

I noticed that with that files I have no tianocore logo at boot and moreover, since I have gpu passthrough I cannot boot the vm if hdmi is not attached (2 monitors setup, hdmi<-->hdmi and dvi<-->dvi to vga adapter<-->vga).
If I have hdmi attached I can boot without any problem.

I tried to compile with the same commands edk2 on kali linux, except for the last line which became:
build -a X64 -b RELEASE -p OvmfPkg/OvmfPkgX64.dsc -t GCC5

and the generated files work well (I have the tianocore logo and I can boot without hdmi attached, i.e. only one monitor).
What's going on?
Anybody can explain?
Is xcode5 incompatible, is there something wrong in the code or is it my fault?
At the beginning I thought about an edk2 bug, so I opened an issue into the bugtracker, full history here:
Bug 3006

I attach also the compilation log from mac OS.

Thanks

Daniele
<log-macOs.txt.zip>


Re: [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

Yao, Jiewen
 

Fair enough.

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

I will wait for a few more days to see if there is any feedback from Laszlo or Marc-Andre.

-----Original Message-----
From: Lee, Terry <terry.lee@hpe.com>
Sent: Friday, October 16, 2020 1:32 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
<chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
incorrect TCG VER comparision

Jiewen,

I have only PP1.3 configuration. The only WHCK test failure is a known
Windows issue that I believe is unrelated to PP.

Terry

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
Sent: Thursday, October 15, 2020 7:31 PM
To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
<chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
incorrect TCG VER comparision

Thanks Terry.
I tend to give R-B. I read the code it seems no impact.

Would you please confirm you have tested both PP1.2 and PP1.3
configuration, with windows WHCK test pass?

Thank you
Yao Jiewen

-----Original Message-----
From: Lee, Terry <terry.lee@hpe.com>
Sent: Friday, October 16, 2020 10:25 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
<chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

Jiewen,

I tested this patch on HPE Superdome Flex with both Linux and Windows.

Terry

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
<chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

Hello
Is there any one can share the information on what test has been done
for this ?

Thank you
Yao Jiewen

-----Original Message-----
From: Lee, Terry <terry.lee@hpe.com>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com;
Gao, Zhichao <zhichao.gao@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
Marc- André Lureau <marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

Could the package maintainer merge this patch? Thanks.

Terry

-----Original Message-----
From: Stefan Berger [mailto:stefanb@linux.ibm.com]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Lee, Terry <terry.lee@hpe.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Zhang,
Chao B <chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

On 7/10/20 9:53 AM, Stefan Berger wrote:
On 7/10/20 1:43 AM, Laszlo Ersek wrote:
(+Marc-André, Stefan)

On 07/10/20 02:44, Gao, Zhichao wrote:
This bug is not obeserved by me. But I view the code. The
condition is incorrect and it would affect the TCG operation:
if (!mIsTcg2PPVerLowerThan_1_3) {
if (OperationRequest <
TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
//
// TCG2 PP1.3 spec defined operations that are
reserved or un-implemented
//
return
TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
}
} else {
//
// TCG PP lower than 1.3. (1.0, 1.1, 1.2)
//
if (OperationRequest <=
TCG2_PHYSICAL_PRESENCE_NO_ACTION_MAX) {
RequestConfirmed = TRUE;
} else if (OperationRequest <
TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
return
TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
}
}
I've found that code myself, but I'm not familiar enough with TPM
PPI stuff to understand immediately the effects of this change. I
can see that where we used to return
TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED before, we
could now
assign "RequestConfirmed = TRUE", and vice versa, due to
"mIsTcg2PPVerLowerThan_1_3" being potentially inverted.

But what does that *mean*? What is the behavioral change that
human end-users, or software components, will experience?

The above code snipped is located in a default branch of a large
switch statement that handles most of the common PPI operations
independent of this change, so that at least is good.

I would say that in the worst case some of the operations not
otherwise handled may have mistakenly failed or could have been
executed without user confirmation/interaction. On Linux at least
PPI requests can only be sent by root.

I am running a somewhat dated version of edk2 (Fedora 31). The
operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these
are individually handled in the switch statement, so there should no
be any impact. I am currently not aware of whether this list can be
extended with some sort of module.





Thanks
Laszlo

So I think it should be fixed.

Thanks,
Zhichao

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Laszlo Ersek
Sent: Thursday, July 9, 2020 6:02 PM
To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Terry Lee <terry.lee@hpe.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
Zhang, Chao B <chao.b.zhang@intel.com>
Subject: Re: [edk2-devel] [PATCH]
SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER
comparision

On 07/09/20 04:46, Gao, Zhichao wrote:
From: Terry Lee <terry.lee@hpe.com>

REF:
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.
ti
an
ocore.org_show-5Fbug.cgi-3Fid-
3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
LFWg&r=Jlc0Jxr620EZ-
CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
s-
W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
48
Bj8YhXhQAI&e=

Tcg2PhysicalPresenceLibConstructor set the module variable
mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version
comparision.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c |
2
+-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git
a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
index 1c46d5e69d..8afaa0a785 100644
---
a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
+++
b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
+++ esenceLib.c
@@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor ( {
EFI_STATUS Status;

- if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8
*)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer),
sizeof(PP_INF_VERSION_1_2) - 1) <= 0) {
+ if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8
+*)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer),
+ sizeof(PP_INF_VERSION_1_2) - 1) >= 0) {
mIsTcg2PPVerLowerThan_1_3 = TRUE;
}

What is the practical impact of this bug / fix?

Thanks
Laszlo






[PATCH] CryptoPkg/BaseCryptLib: fix NULL dereference (CVE-2019-14584)

Wang, Jian J
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1914=0D
=0D
AuthenticodeVerify() calls OpenSSLs d2i_PKCS7() API to parse asn encoded=0D
signed authenticode pkcs#7 data. when this successfully returns, a type=0D
check is done by calling PKCS7_type_is_signed() and then=0D
Pkcs7->d.sign->contents->type is used. It is possible to construct an asn1=
=0D
blob that successfully decodes and have d2i_PKCS7() return a valid pointer=
=0D
and have PKCS7_type_is_signed() also return success but have Pkcs7->d.sign=
=0D
be a NULL pointer.=0D
=0D
Looking at how PKCS7_verify() [inside of OpenSSL] implements checking for=0D
pkcs7 structs it does the following:=0D
- call PKCS7_type_is_signed()=0D
- call PKCS7_get_detached()=0D
Looking into how PKCS7_get_detatched() is implemented, it checks to see if=
=0D
p7->d.sign is NULL or if p7->d.sign->contents->d.ptr is NULL.=0D
=0D
As such, the fix is to do the same as OpenSSL after calling d2i_PKCS7().=0D
- Add call to PKS7_get_detached() to existing error handling=0D
=0D
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>=0D
Cc: Guomin Jiang <guomin.jiang@intel.com>=0D
Cc: Jiewen Yao <jiewen.yao@intel.com>=0D
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>=0D
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c b/Crypto=
Pkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c
index 2772b1e2be..ae0ee61fb6 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c
@@ -9,7 +9,7 @@
AuthenticodeVerify() will get PE/COFF Authenticode and will do basic che=
ck for=0D
data structure.=0D
=0D
-Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.<BR>=0D
+Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
@@ -100,7 +100,7 @@ AuthenticodeVerify (
//=0D
// Check if it's PKCS#7 Signed Data (for Authenticode Scenario)=0D
//=0D
- if (!PKCS7_type_is_signed (Pkcs7)) {=0D
+ if (!PKCS7_type_is_signed (Pkcs7) || PKCS7_get_detached (Pkcs7)) {=0D
goto _Exit;=0D
}=0D
=0D
--=20
2.19.0.windows.1


Tianocore-docs Gitbook offline document status (PDF, EPUB, MOBI)

Michael D Kinney
 

Hello,

I have been working on addressing the gaps in the transition to
the new GitBook services for the TianoCore documents in the GitBook
markdown format. The major gap is the loss of the offline PDF,
EPUB, and MOBI formats.

I have found a GitHub action that performs the equivalent work
of the legacy GitBook server and it supports publishing the HTML,
PDF, EPUB, and MOBI formats in a gh-pages branch of a GitBook
document repository. The gh-pages branch supports the HTML
web view of the documents and is stored as part of the same
GitHub repository that hosts the document source files.

I have tried this out on the edk2-TemplateSpecification document
in the Tianocore-Docs GitHub organization.

https://github.com/tianocore-docs/edk2-TemplateSpecification

The following is the link to the GitHub actions YML file that
publishes a draft version of the document from the master branch
and the release versions of the document from and release/* branch.

https://github.com/tianocore-docs/edk2-TemplateSpecification/blob/master/.github/workflows/gitbook-action.yml

GitBook Action:
* Source: https://github.com/ZanderZhao/gitbook-action
* Docs: https://zlogs.net/gitbook-action/

I found a few issues with the support of embedded PlantUml
diagrams. A fork of the GitBook puml plugin is available
that addresses these issues. The book.json file is updated
to use this newer plugin.

"plugins": ["puml-aleung"],

Links to the GitBook puml pluigis:

Original: https://github.com/GitbookIO/plugin-puml
Updated: https://github.com/aleung/gitbook-plugin-puml

The following are the links to the EDK II Template Specification
documents published by this Gitbook Action. Notice that all the
links are to files in either GitHub repos or the web pages published
by GitHub when a gh-pages branch is present and updated.

Draft versions from master branch:

HTML: https://tianocore-docs.github.io/edk2-TemplateSpecification/master
PDF: https://github.com/tianocore-docs/edk2-TemplateSpecification/raw/gh-pages/master/mybook/ebook.pdf
EPUB: https://github.com/tianocore-docs/edk2-TemplateSpecification/raw/gh-pages/master/mybook/ebook.epub
MOBI: https://github.com/tianocore-docs/edk2-TemplateSpecification/raw/gh-pages/master/mybook/ebook.mobi

Release versions from release/0.2 branch:

HTML: https://tianocore-docs.github.io/edk2-TemplateSpecification/release-0.2
PDF: https://github.com/tianocore-docs/edk2-TemplateSpecification/raw/gh-pages/release-0.2/mybook/ebook.pdf
EPUB: https://github.com/tianocore-docs/edk2-TemplateSpecification/raw/gh-pages/release-0.2/mybook/ebook.epub
MOBI: https://github.com/tianocore-docs/edk2-TemplateSpecification/raw/gh-pages/release-0.2/mybook/ebook.mobi

In order to enable this on all the documents in Tianocore-docs, the
following tasks need to be performed on each document repo:
* Update book.json in master and release/* branches to use the
newer PlantUML plugin.
* Add the file .github/workflows/gitbook-action.yml to the
master and release/* branches.
* Force a document build on the master and release/* branches to
publish all draft and release versions of the documents.

Please review the content here and the published documents and let
me know if there are any concerns with switching to a GitHub
Action to publish all Tianocore Gitbook markdown based documents.

Thanks,

Mike


Re: [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

Yao, Jiewen
 

Thanks Terry.
I tend to give R-B. I read the code it seems no impact.

Would you please confirm you have tested both PP1.2 and PP1.3 configuration, with windows WHCK test pass?

Thank you
Yao Jiewen

-----Original Message-----
From: Lee, Terry <terry.lee@hpe.com>
Sent: Friday, October 16, 2020 10:25 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
<chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
incorrect TCG VER comparision

Jiewen,

I tested this patch on HPE Superdome Flex with both Linux and Windows.

Terry

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
Sent: Thursday, October 15, 2020 6:09 PM
To: Lee, Terry <terry.lee@hpe.com>; devel@edk2.groups.io;
stefanb@linux.ibm.com; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
<chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix
incorrect TCG VER comparision

Hello
Is there any one can share the information on what test has been done for
this ?

Thank you
Yao Jiewen

-----Original Message-----
From: Lee, Terry <terry.lee@hpe.com>
Sent: Friday, October 16, 2020 12:59 AM
To: devel@edk2.groups.io; stefanb@linux.ibm.com; lersek@redhat.com;
Gao, Zhichao <zhichao.gao@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Marc-
André Lureau <marcandre.lureau@redhat.com>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

Could the package maintainer merge this patch? Thanks.

Terry

-----Original Message-----
From: Stefan Berger [mailto:stefanb@linux.ibm.com]
Sent: Friday, July 10, 2020 7:27 AM
To: devel@edk2.groups.io; lersek@redhat.com; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Lee, Terry <terry.lee@hpe.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Zhang,
Chao B <chao.b.zhang@intel.com>; Marc-André Lureau
<marcandre.lureau@redhat.com>
Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib:
Fix incorrect TCG VER comparision

On 7/10/20 9:53 AM, Stefan Berger wrote:
On 7/10/20 1:43 AM, Laszlo Ersek wrote:
(+Marc-André, Stefan)

On 07/10/20 02:44, Gao, Zhichao wrote:
This bug is not obeserved by me. But I view the code. The
condition is incorrect and it would affect the TCG operation:
if (!mIsTcg2PPVerLowerThan_1_3) {
if (OperationRequest <
TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
//
// TCG2 PP1.3 spec defined operations that are reserved
or un-implemented
//
return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
}
} else {
//
// TCG PP lower than 1.3. (1.0, 1.1, 1.2)
//
if (OperationRequest <=
TCG2_PHYSICAL_PRESENCE_NO_ACTION_MAX) {
RequestConfirmed = TRUE;
} else if (OperationRequest <
TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
}
}
I've found that code myself, but I'm not familiar enough with TPM
PPI stuff to understand immediately the effects of this change. I
can see that where we used to return
TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED before, we
could now
assign "RequestConfirmed = TRUE", and vice versa, due to
"mIsTcg2PPVerLowerThan_1_3" being potentially inverted.

But what does that *mean*? What is the behavioral change that human
end-users, or software components, will experience?

The above code snipped is located in a default branch of a large
switch statement that handles most of the common PPI operations
independent of this change, so that at least is good.

I would say that in the worst case some of the operations not
otherwise handled may have mistakenly failed or could have been
executed without user confirmation/interaction. On Linux at least
PPI requests can only be sent by root.

I am running a somewhat dated version of edk2 (Fedora 31). The
operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these
are individually handled in the switch statement, so there should no
be any impact. I am currently not aware of whether this list can be
extended with some sort of module.





Thanks
Laszlo

So I think it should be fixed.

Thanks,
Zhichao

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Laszlo Ersek
Sent: Thursday, July 9, 2020 6:02 PM
To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Terry Lee <terry.lee@hpe.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
Zhang, Chao B <chao.b.zhang@intel.com>
Subject: Re: [edk2-devel] [PATCH]
SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER
comparision

On 07/09/20 04:46, Gao, Zhichao wrote:
From: Terry Lee <terry.lee@hpe.com>

REF:
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.ti
an
ocore.org_show-5Fbug.cgi-3Fid-
3D2697&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2
LFWg&r=Jlc0Jxr620EZ-
CppyrjGotnxH9DrT0KvwcLjekZ9Dow&m=WPv3vn5VEelRC
s-
W8pfNM00wMOfpKBesXnAhRfylF7g&s=iFUYthUCfHLeeQAvr_OhTPHTiA9hZvw
48
Bj8YhXhQAI&e=

Tcg2PhysicalPresenceLibConstructor set the module variable
mIsTcg2PPVerLowerThan_1_3 with incorrect TCG version
comparision.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 2
+-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git
a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
index 1c46d5e69d..8afaa0a785 100644
---
a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
esen

ceLib.c
+++
b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPr
+++ esenceLib.c
@@ -387,7 +387,7 @@ Tcg2PhysicalPresenceLibConstructor ( {
EFI_STATUS Status;

- if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8
*)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer),
sizeof(PP_INF_VERSION_1_2) - 1) <= 0) {
+ if (AsciiStrnCmp(PP_INF_VERSION_1_2, (CHAR8
+*)PcdGetPtr(PcdTcgPhysicalPresenceInterfaceVer),
+ sizeof(PP_INF_VERSION_1_2) - 1) >= 0) {
mIsTcg2PPVerLowerThan_1_3 = TRUE;
}

What is the practical impact of this bug / fix?

Thanks
Laszlo





4121 - 4140 of 70394