Date   

Re: [edk2-platforms] [PATCH v3 2/3] Platform/ARM/SgiPkg: Add HMAT ACPI table for RdN1EdgeX2

Vijayenthiran Subramaniam
 

Hi Sami,

 

Agree with the suggestion. Please make the changes.

 

Regards,

Vijay

 

From: "Sami Mujawar via Groups.Io" <sami.mujawar@...>
Reply to: Sami Mujawar <Sami.Mujawar@...>
Date: Thursday, 8 April 2021 at 12:07 AM
To: Vijayenthiran Subramaniam <Vijayenthiran.Subramaniam@...>, "devel@edk2.groups.io" <devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-platforms] [PATCH v3 2/3] Platform/ARM/SgiPkg: Add HMAT ACPI table for RdN1EdgeX2

 

Hi Vijay,

+#define HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES_INIT( \
+ TotalCacheLevels, CacheLevel, CacheAssociativity, WritePolicy, CacheLineSize \
+ ) \
+{ \
+ TotalCacheLevels, CacheLevel, CacheAssociativity, WritePolicy, CacheLineSize \
+}

This macros is again repeated in patch 3/3. I think this could be moved to Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h.
If you agree I can make this change locally before pushing this series.

Regards,

Sami Mujawar


Re: [edk2-platforms][PATCH v2 1/1] Platform/RaspberryPi: Fix mini UART clock divisor calculation

Pete Batard
 

On 2021.04.08 13:26, Ard Biesheuvel wrote:
On Thu, 8 Apr 2021 at 13:43, Pete Batard <pete@akeo.ie> wrote:

On 2021.04.08 12:06, Ard Biesheuvel wrote:
On Thu, 8 Apr 2021 at 11:47, Pete Batard <pete@akeo.ie> wrote:

On 2021.04.06 15:04, Mario Bălănică wrote:
The VPU clock divisor has changed in this commit:
https://github.com/raspberrypi/firmware/commit/1e5456a,
thus breaking the mini UART clock divisor calculation on the Pi 4.

Fix this by reading the core clock from the mailbox instead.

Signed-off-by: Mario Bălănică <mariobalanica02@gmail.com>
---
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 15 +++------------
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 4 ----
Platform/RaspberryPi/RPi4/RPi4.dsc | 6 ++++--
3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
index 5e83bbf022eb..d2f983bf0a9f 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
@@ -34,25 +34,16 @@ SerialPortGetDivisor (
UINT32 SerialBaudRate

)

{

- UINT64 BaseClockRate;

+ UINT32 BaseClockRate;

UINT32 Divisor;



- //

- // On the Raspberry Pi, the clock to use for the 16650-compatible UART

- // is the base clock divided by the 12.12 fixed point VPU clock divisor.

- //

- BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate);

-#if (RPI_MODEL == 4)

- Divisor = MmioRead32(BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;

- if (Divisor != 0)

- BaseClockRate = (BaseClockRate << 12) / Divisor;

-#endif

+ BaseClockRate = PcdGet32 (PcdSerialClockRate);



//

// As per the BCM2xxx datasheets:

// baudrate = system_clock_freq / (8 * (divisor + 1)).

//

- Divisor = (UINT32)BaseClockRate / (SerialBaudRate * 8);

+ Divisor = BaseClockRate / (SerialBaudRate * 8);

if (Divisor != 0) {

Divisor--;

}

diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
index 58351e4fb8cc..7008aaf8aa4c 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
+++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
@@ -85,13 +85,11 @@ ASM_FUNC (ArmPlatformPeiBootAction)
adr x2, mBoardRevision

str w0, [x2]



-#if (RPI_MODEL == 3)

run .Lclkinfo_buffer



ldr w0, .Lfrequency

adr x2, _gPcd_BinaryPatch_PcdSerialClockRate

str w0, [x2]

-#endif



ret



@@ -135,7 +133,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // end tag

.set .Lrevinfo_size, . - .Lrevinfo_buffer



-#if (RPI_MODEL == 3)

.align 4

.Lclkinfo_buffer:

.long .Lclkinfo_size

@@ -148,7 +145,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // frequency

.long 0 // end tag

.set .Lclkinfo_size, . - .Lclkinfo_buffer

-#endif



//UINTN

//ArmPlatformGetPrimaryCoreMpId (

diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 2c05c31118d2..ff802d8347ea 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -429,7 +429,6 @@ [PcdsFixedAtBuild.common]
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4

- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8

gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200

@@ -465,6 +464,9 @@ [PcdsFixedAtBuild.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"

gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE



+[PcdsPatchableInModule]

+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000

+

[PcdsDynamicHii.common.DEFAULT]



#

@@ -621,7 +623,7 @@ [Components.common]
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf

MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {

<LibraryClasses>

- SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf

+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf

}

Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf

EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf
Reviewed-by: Pete Batard <pete@akeo.ie>
Tested-by: Pete Batard <pete@akeo.ie>
Thanks Pete.

Could anyone get me a version of this patch that is not whitespace
mangled? This one does not apply with 'git am'
Done.

The version I sent is the one I applied & tested after I ran it through
my unmangling script to remove the extra lines.

Not sure if it'll still not be re-mangled by the list processing though.
Yes, which is odd given that I am receiving this email directly.
In any case, I still had to perform surgery on the patch to get it to apply.
Pushed as 7fe9704893f1..d2339f3c5f9a; mind double checking if I did
not break anything?
Just re-tested with edk2-platforms latest, and everything looks good. Thanks.

/Pete

Thanks,
Ard.


Re: [edk2-platforms][PATCH v2 1/1] Platform/RaspberryPi: Fix mini UART clock divisor calculation

Ard Biesheuvel
 

On Thu, 8 Apr 2021 at 13:43, Pete Batard <pete@akeo.ie> wrote:

On 2021.04.08 12:06, Ard Biesheuvel wrote:
On Thu, 8 Apr 2021 at 11:47, Pete Batard <pete@akeo.ie> wrote:

On 2021.04.06 15:04, Mario Bălănică wrote:
The VPU clock divisor has changed in this commit:
https://github.com/raspberrypi/firmware/commit/1e5456a,
thus breaking the mini UART clock divisor calculation on the Pi 4.

Fix this by reading the core clock from the mailbox instead.

Signed-off-by: Mario Bălănică <mariobalanica02@gmail.com>
---
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 15 +++------------
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 4 ----
Platform/RaspberryPi/RPi4/RPi4.dsc | 6 ++++--
3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
index 5e83bbf022eb..d2f983bf0a9f 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
@@ -34,25 +34,16 @@ SerialPortGetDivisor (
UINT32 SerialBaudRate

)

{

- UINT64 BaseClockRate;

+ UINT32 BaseClockRate;

UINT32 Divisor;



- //

- // On the Raspberry Pi, the clock to use for the 16650-compatible UART

- // is the base clock divided by the 12.12 fixed point VPU clock divisor.

- //

- BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate);

-#if (RPI_MODEL == 4)

- Divisor = MmioRead32(BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;

- if (Divisor != 0)

- BaseClockRate = (BaseClockRate << 12) / Divisor;

-#endif

+ BaseClockRate = PcdGet32 (PcdSerialClockRate);



//

// As per the BCM2xxx datasheets:

// baudrate = system_clock_freq / (8 * (divisor + 1)).

//

- Divisor = (UINT32)BaseClockRate / (SerialBaudRate * 8);

+ Divisor = BaseClockRate / (SerialBaudRate * 8);

if (Divisor != 0) {

Divisor--;

}

diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
index 58351e4fb8cc..7008aaf8aa4c 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
+++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
@@ -85,13 +85,11 @@ ASM_FUNC (ArmPlatformPeiBootAction)
adr x2, mBoardRevision

str w0, [x2]



-#if (RPI_MODEL == 3)

run .Lclkinfo_buffer



ldr w0, .Lfrequency

adr x2, _gPcd_BinaryPatch_PcdSerialClockRate

str w0, [x2]

-#endif



ret



@@ -135,7 +133,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // end tag

.set .Lrevinfo_size, . - .Lrevinfo_buffer



-#if (RPI_MODEL == 3)

.align 4

.Lclkinfo_buffer:

.long .Lclkinfo_size

@@ -148,7 +145,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // frequency

.long 0 // end tag

.set .Lclkinfo_size, . - .Lclkinfo_buffer

-#endif



//UINTN

//ArmPlatformGetPrimaryCoreMpId (

diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 2c05c31118d2..ff802d8347ea 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -429,7 +429,6 @@ [PcdsFixedAtBuild.common]
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4

- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8

gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200

@@ -465,6 +464,9 @@ [PcdsFixedAtBuild.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"

gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE



+[PcdsPatchableInModule]

+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000

+

[PcdsDynamicHii.common.DEFAULT]



#

@@ -621,7 +623,7 @@ [Components.common]
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf

MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {

<LibraryClasses>

- SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf

+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf

}

Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf

EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf
Reviewed-by: Pete Batard <pete@akeo.ie>
Tested-by: Pete Batard <pete@akeo.ie>
Thanks Pete.

Could anyone get me a version of this patch that is not whitespace
mangled? This one does not apply with 'git am'
Done.

The version I sent is the one I applied & tested after I ran it through
my unmangling script to remove the extra lines.

Not sure if it'll still not be re-mangled by the list processing though.
Yes, which is odd given that I am receiving this email directly.

In any case, I still had to perform surgery on the patch to get it to apply.

Pushed as 7fe9704893f1..d2339f3c5f9a; mind double checking if I did
not break anything?

Thanks,
Ard.


Re: [RFC PATCH 00/19] Add AMD Secure Nested Paging (SEV-SNP) support

Brijesh Singh
 

Hi Laszlo,

On 4/8/21 4:58 AM, Laszlo Ersek wrote:
Hi Brijesh,

On 03/24/21 16:31, Brijesh Singh wrote:
BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb8ee3abf81aa4e5b1e6008d8fa74dbbc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637534728122143364%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=G2AQ%2FCks3%2BbczHJXMwqlqpWpoBJmb0pmxb1VNLw6t%2BA%3D&amp;reserved=0

SEV-SNP builds upon existing SEV and SEV-ES functionality while adding
new hardware-based memory protections. SEV-SNP adds strong memory integrity
protection to help prevent malicious hypervisor-based attacks like data
replay, memory re-mapping and more in order to create an isolated memory
encryption environment.

This series provides the basic building blocks to support booting the SEV-SNP
VMs, it does not cover all the security enhancement introduced by the SEV-SNP
such as interrupt protection.

Many of the integrity guarantees of SEV-SNP are enforced through a new
structure called the Reverse Map Table (RMP). Adding a new page to SEV-SNP
VM requires a 2-step process. First, the hypervisor assigns a page to the
guest using the new RMPUPDATE instruction. This transitions the page to
guest-invalid. Second, the guest validates the page using the new PVALIDATE
instruction. The SEV-SNP VMs can use the new "Page State Change Request NAE"
defined in the GHCB specification to ask hypervisor to add or remove page
from the RMP table.

Each page assigned to the SEV-SNP VM can either be validated or unvalidated,
as indicated by the Validated flag in the page's RMP entry. There are two
approaches that can be taken for the page validation: Pre-validation and
Lazy Validation.

Under pre-validation, the pages are validated prior to first use. And under
lazy validation, pages are validated when first accessed. An access to a
unvalidated page results in a #VC exception, at which time the exception
handler may validate the page. Lazy validation requires careful tracking of
the validated pages to avoid validating the same GPA more than once. The
recently introduced "Unaccepted" memory type can be used to communicate the
unvalidated memory ranges to the Guest OS.

At this time we only support the pre-validation. OVMF detects all the available
system RAM in the PEI phase. When SEV-SNP is enabled, the memory is validated
before it is made available to the EDK2 core.
Can you describe this in a bit more detail, before I look at the
individual patches? Specifically, what existing logic in the PEI phase
was taken, and extended, and how?
One of the key requirement is that the guest private pages much be
validated before the access. If guest tries to access the pages before
the validation then it will result in #VC (page-not-validated)
exception. To avoid the #VC, we propose the validating the memory before
the access. We will incrementally add the support to lazy validate (i.e
validate on access).

Let me try explaining a bit, the page validation process consist of two
steps:

1. Add the pages in the RMP table -- must be done by the hypervisor
using the RMPUPDATE instruction. The guest can use VMGEXIT NAEs to ask
hypervisor to add or remove pages from the RMP table.

2. Guest issue the PVALIDATE instruction -- this sets the validate bit
in the RMP table.

Similar to SEV, the OVMF_CODE.fd is encrypted through the SNP firmware
before the launch. The SNP firmware also validates the memory page after
encrypting. This allows us to boot the initial entry code without guest
going through the validation process.

The OVMF reset vector uses few data pages (e.g page table, early Sec
stack). Access to these data pages will result in #VC. There are two
approaches we can take to validate these data pages:

1. Ask SNP firmware to pre-validate it -- SNP firmware provides an
special command that can be used to pre-validate the pages without
affecting the measurement.

2. Enhance the reset vector code to validate the pages.

For now I choose #1.

The pre-validation performed by the SNP firmware is sufficient to boot
through the SEC phase. The SEC phase later decompress the Fv to a new
memory location. Now we need the OVMF to take over the validation
procedure.  The series extends the MemEncryptSevLib to add a new helper
MemEncryptSevSnpValidateRam(). The helper is used to validate the system
RAM. See patch #12. SEC phase calls the MemEncryptSevSnpValidateRam() to
validate the output buffer used for the decompression. This was
sufficient to boot into the PEI phase, see patch #13. The PEI detects
all the available system RAM. After the memory detection is completed
the PlatformPei calls the AmdSevSnpInitialize(). The initialization
routine iterate through the HOB and calls the
MemEncryptSevSnpValidateRam() to validate all the system RAM. Is it
possible the more system ram can be detected after the PlatformPei is
completed ?

One of the important thing is we should *never* validate the pages
twice. The MemEncryptSevSnpValidateRam() uses a interval search tree to
keep the record of what has been validated. Before validating the range,
it lookup in its tree and if it finds that range is already validated
then do nothing. If it detects an overlap then it will validate only non
overlapping regions -- see patch #14.

The patch #18 extend the MemEncrypt{Set,Clear}PageEncMask() to call the
SNP page state change during the C-bit toggle.

Please let me know if you have any questions. We can hash out the design
here before you taking a closure look at the code.


If there is a particular patch whose commit message is closely related
to my question, can you point it out? Patch#15 perhaps? (Doesn't seem
like a big patch; for some reason I'd expect something more complex, but
perhaps that's only because it builds upon the many earlier patches.)

Thanks,
Laszlo

This series does not implements the following SEV-SNP features yet:

* CPUID filtering
* AP bring up using the new SEV-SNP NAE
* Lazy validation
* Interrupt security

The series is based on commit:
e542e05d4f UefiCpuPkg/SmmCpuFeaturesLib: Abstract PcdCpuMaxLogicalProcessorNumber

Additional resources
---------------------
SEV-SNP whitepaper
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2FSEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb8ee3abf81aa4e5b1e6008d8fa74dbbc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637534728122143364%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=uUqlVHhWQ6geGDaNHwxGMpoSpIamB%2F1vHH69h%2FEGUro%3D&amp;reserved=0

APM 2: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F24593.pdf&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb8ee3abf81aa4e5b1e6008d8fa74dbbc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637534728122143364%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=R2kU42jvCDZat8kGZ5gDDz2nFXIawHfXdRW1aovhNK8%3D&amp;reserved=0 (section 15.36)

The complete source is available at
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Ftree%2Fsev-snp-rfc-1&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb8ee3abf81aa4e5b1e6008d8fa74dbbc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637534728122143364%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=yreFg2hr%2F82WYEjxqCmb7pXUdtrRCJRYPrPHgfrWjM8%3D&amp;reserved=0

GHCB spec v2:
The draft specification is posted on AMD-SEV-SNP mailing list:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.suse.com%2Fmailman%2Fprivate%2Famd-sev-snp%2F&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb8ee3abf81aa4e5b1e6008d8fa74dbbc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637534728122143364%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=PFV2mA7T%2Fbl2zP5j52kNdT%2FavDMRLWDEDqz6JGusEFg%3D&amp;reserved=0

Copy of the spec is also available at
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2FAMDSEV%2Fblob%2Fsev-snp-devel%2Fdocs%2F56421-Guest_Hypervisor_Communication_Block_Standardization.pdf&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb8ee3abf81aa4e5b1e6008d8fa74dbbc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637534728122143364%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=U3oKRe5m0NxI0xqv1gBoyh%2BEEX1LVeCWR42rvPh6XZ8%3D&amp;reserved=0

GHCB spec v1:
SEV-SNP firmware specification:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeveloper.amd.com%2Fsev%2F&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb8ee3abf81aa4e5b1e6008d8fa74dbbc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637534728122143364%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=G%2BttkORsTJchJ1Fy1iNlD%2B%2BqiQZuwI8md5vhJjEb%2Fn4%3D&amp;reserved=0

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>

Brijesh Singh (19):
OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest
OvmfPkg: validate the data pages used in the SEC phase
MdePkg: Expand the SEV MSR to include the SNP definition
OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled()
MdePkg: Define the GHCB GPA structure
UefiCpuPkg/MpLib: add support to register GHCB GPA when SEV-SNP is
enabled
OvmfPkg: Add a library to support registering GHCB GPA
OvmfPkg: register GHCB gpa for the SEV-SNP guest
MdePkg: Add AsmPvalidate() support
OvmfPkg: Define the Page State Change VMGEXIT structures
OvmfPkg/ResetVector: Invalidate the GHCB page
OvmfPkg/MemEncryptSevLib: Add support to validate system RAM
OvmfPkg/SecMain: Validate the data/code pages used for the PEI phase
OvmfPkg/MemEncryptSevLib: Add support to validate RAM in PEI phase
OvmfPkg/PlatformPei: Validate the system RAM when SNP is active
OvmfPkg/MemEncryptSevLib: Add support to validate > 4GB memory in PEI
phase
OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase
OvmfPkg/MemEncryptSevLib: Validate the memory during set or clear enc
attribute
OvmfPkg/MemEncryptSevLib: Skip page state change for non RAM region

MdePkg/Include/Library/BaseLib.h | 37 +++
MdePkg/Include/Register/Amd/Fam17Msr.h | 31 ++-
MdePkg/Include/Register/Amd/Ghcb.h | 39 ++-
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 43 +++
OvmfPkg/Include/Library/GhcbRegisterLib.h | 27 ++
OvmfPkg/Include/Library/MemEncryptSevLib.h | 30 +++
.../DxeMemEncryptSevLib.inf | 7 +
.../DxeMemEncryptSevLibInternal.c | 27 ++
.../Ia32/SnpPageStateChange.c | 17 ++
.../PeiMemEncryptSevLib.inf | 9 +
.../PeiMemEncryptSevLibInternal.c | 47 ++++
.../SecMemEncryptSevLib.inf | 4 +
.../SecMemEncryptSevLibInternal.c | 39 +++
.../BaseMemEncryptSevLib/SnpPageStateChange.h | 37 +++
.../X64/PeiDxeSnpSetPageState.c | 63 +++++
.../X64/PeiDxeVirtualMemory.c | 151 ++++++++++-
.../X64/PeiSnpSystemRamValidate.c | 129 +++++++++
.../X64/SecSnpSystemRamValidate.c | 23 ++
.../X64/SnpPageStateChangeInternal.c | 254 ++++++++++++++++++
.../X64/SnpPageStateTrack.c | 119 ++++++++
.../X64/SnpPageStateTrack.h | 36 +++
.../X64/SnpSetPageState.h | 27 ++
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 19 ++
.../Library/GhcbRegisterLib/GhcbRegisterLib.c | 97 +++++++
.../GhcbRegisterLib/GhcbRegisterLib.inf | 33 +++
OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf | 4 +
OvmfPkg/Library/VmgExitLib/VmgExitLib.inf | 7 +
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 45 ++++
OvmfPkg/OvmfPkg.dec | 12 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfPkgX64.fdf | 33 ++-
OvmfPkg/PlatformPei/AmdSev.c | 52 ++++
OvmfPkg/PlatformPei/PlatformPei.inf | 2 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 24 ++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 106 ++++++++
OvmfPkg/ResetVector/ResetVector.inf | 5 +
OvmfPkg/ResetVector/ResetVector.nasmb | 4 +
OvmfPkg/Sec/SecMain.c | 102 +++++++
OvmfPkg/Sec/SecMain.inf | 2 +
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 1 +
UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 +
UefiCpuPkg/Library/MpInitLib/MpLib.h | 2 +
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 51 ++++
UefiCpuPkg/UefiCpuPkg.dec | 6 +
47 files changed, 1790 insertions(+), 19 deletions(-)
create mode 100644 MdePkg/Library/BaseLib/X64/Pvalidate.nasm
create mode 100644 OvmfPkg/Include/Library/GhcbRegisterLib.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/SnpPageStateChange.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/SnpPageStateChange.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeSnpSetPageState.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateTrack.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateTrack.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpSetPageState.h
create mode 100644 OvmfPkg/Library/GhcbRegisterLib/GhcbRegisterLib.c
create mode 100644 OvmfPkg/Library/GhcbRegisterLib/GhcbRegisterLib.inf


Re: [edk2-platforms][PATCH v2 1/1] Platform/RaspberryPi: Fix mini UART clock divisor calculation

Pete Batard
 

On 2021.04.08 12:06, Ard Biesheuvel wrote:
On Thu, 8 Apr 2021 at 11:47, Pete Batard <pete@akeo.ie> wrote:

On 2021.04.06 15:04, Mario Bălănică wrote:
The VPU clock divisor has changed in this commit:
https://github.com/raspberrypi/firmware/commit/1e5456a,
thus breaking the mini UART clock divisor calculation on the Pi 4.

Fix this by reading the core clock from the mailbox instead.

Signed-off-by: Mario Bălănică <mariobalanica02@gmail.com>
---
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 15 +++------------
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 4 ----
Platform/RaspberryPi/RPi4/RPi4.dsc | 6 ++++--
3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
index 5e83bbf022eb..d2f983bf0a9f 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
@@ -34,25 +34,16 @@ SerialPortGetDivisor (
UINT32 SerialBaudRate

)

{

- UINT64 BaseClockRate;

+ UINT32 BaseClockRate;

UINT32 Divisor;



- //

- // On the Raspberry Pi, the clock to use for the 16650-compatible UART

- // is the base clock divided by the 12.12 fixed point VPU clock divisor.

- //

- BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate);

-#if (RPI_MODEL == 4)

- Divisor = MmioRead32(BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;

- if (Divisor != 0)

- BaseClockRate = (BaseClockRate << 12) / Divisor;

-#endif

+ BaseClockRate = PcdGet32 (PcdSerialClockRate);



//

// As per the BCM2xxx datasheets:

// baudrate = system_clock_freq / (8 * (divisor + 1)).

//

- Divisor = (UINT32)BaseClockRate / (SerialBaudRate * 8);

+ Divisor = BaseClockRate / (SerialBaudRate * 8);

if (Divisor != 0) {

Divisor--;

}

diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
index 58351e4fb8cc..7008aaf8aa4c 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
+++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
@@ -85,13 +85,11 @@ ASM_FUNC (ArmPlatformPeiBootAction)
adr x2, mBoardRevision

str w0, [x2]



-#if (RPI_MODEL == 3)

run .Lclkinfo_buffer



ldr w0, .Lfrequency

adr x2, _gPcd_BinaryPatch_PcdSerialClockRate

str w0, [x2]

-#endif



ret



@@ -135,7 +133,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // end tag

.set .Lrevinfo_size, . - .Lrevinfo_buffer



-#if (RPI_MODEL == 3)

.align 4

.Lclkinfo_buffer:

.long .Lclkinfo_size

@@ -148,7 +145,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // frequency

.long 0 // end tag

.set .Lclkinfo_size, . - .Lclkinfo_buffer

-#endif



//UINTN

//ArmPlatformGetPrimaryCoreMpId (

diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 2c05c31118d2..ff802d8347ea 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -429,7 +429,6 @@ [PcdsFixedAtBuild.common]
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4

- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8

gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200

@@ -465,6 +464,9 @@ [PcdsFixedAtBuild.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"

gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE



+[PcdsPatchableInModule]

+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000

+

[PcdsDynamicHii.common.DEFAULT]



#

@@ -621,7 +623,7 @@ [Components.common]
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf

MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {

<LibraryClasses>

- SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf

+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf

}

Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf

EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf
Reviewed-by: Pete Batard <pete@akeo.ie>
Tested-by: Pete Batard <pete@akeo.ie>
Thanks Pete.
Could anyone get me a version of this patch that is not whitespace
mangled? This one does not apply with 'git am'
Done.

The version I sent is the one I applied & tested after I ran it through my unmangling script to remove the extra lines.

Not sure if it'll still not be re-mangled by the list processing though.

Regards,

/Pete


[edk2-platforms][PATCH v2 1/1] Platform/RaspberryPi: Fix mini UART clock divisor calculation

Pete Batard
 

From: Mario B=C4=83l=C4=83nic=C4=83 <mariobalanica02@gmail.com>=0D

The VPU clock divisor has changed in this commit: https://github.com/raspbe=
rrypi/firmware/commit/1e5456a,=0D
thus breaking the mini UART clock divisor calculation on the Pi 4.=0D
=0D
Fix this by reading the core clock from the mailbox instead.=0D
=0D
Signed-off-by: Mario B=C4=83l=C4=83nic=C4=83 <mariobalanica02@gmail.com>=0D
---=0D
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 15 =
+++------------=0D
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 4 =
----=0D
Platform/RaspberryPi/RPi4/RPi4.dsc | 6 =
++++--=0D
3 files changed, 7 insertions(+), 18 deletions(-)=0D
=0D
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortL=
ib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c=0D
index 5e83bbf022eb..d2f983bf0a9f 100644=0D
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c=0D
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c=0D
@@ -34,25 +34,16 @@ SerialPortGetDivisor (=0D
UINT32 SerialBaudRate=0D
)=0D
{=0D
- UINT64 BaseClockRate;=0D
+ UINT32 BaseClockRate;=0D
UINT32 Divisor;=0D
=0D
- //=0D
- // On the Raspberry Pi, the clock to use for the 16650-compatible UART=0D
- // is the base clock divided by the 12.12 fixed point VPU clock divisor.=
=0D
- //=0D
- BaseClockRate =3D (UINT64)PcdGet32 (PcdSerialClockRate);=0D
-#if (RPI_MODEL =3D=3D 4)=0D
- Divisor =3D MmioRead32(BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) &=
0xFFFFFF;=0D
- if (Divisor !=3D 0)=0D
- BaseClockRate =3D (BaseClockRate << 12) / Divisor;=0D
-#endif=0D
+ BaseClockRate =3D PcdGet32 (PcdSerialClockRate);=0D
=0D
//=0D
// As per the BCM2xxx datasheets:=0D
// baudrate =3D system_clock_freq / (8 * (divisor + 1)).=0D
//=0D
- Divisor =3D (UINT32)BaseClockRate / (SerialBaudRate * 8);=0D
+ Divisor =3D BaseClockRate / (SerialBaudRate * 8);=0D
if (Divisor !=3D 0) {=0D
Divisor--;=0D
}=0D
diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHe=
lper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper=
.S=0D
index 58351e4fb8cc..7008aaf8aa4c 100644=0D
--- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S=
=0D
+++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S=
=0D
@@ -85,13 +85,11 @@ ASM_FUNC (ArmPlatformPeiBootAction)=0D
adr x2, mBoardRevision=0D
str w0, [x2]=0D
=0D
-#if (RPI_MODEL =3D=3D 3)=0D
run .Lclkinfo_buffer=0D
=0D
ldr w0, .Lfrequency=0D
adr x2, _gPcd_BinaryPatch_PcdSerialClockRate=0D
str w0, [x2]=0D
-#endif=0D
=0D
ret=0D
=0D
@@ -135,7 +133,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)=0D
.long 0 // end tag=0D
.set .Lrevinfo_size, . - .Lrevinfo_buffer=0D
=0D
-#if (RPI_MODEL =3D=3D 3)=0D
.align 4=0D
.Lclkinfo_buffer:=0D
.long .Lclkinfo_size=0D
@@ -148,7 +145,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)=0D
.long 0 // frequency=0D
.long 0 // end tag=0D
.set .Lclkinfo_size, . - .Lclkinfo_buffer=0D
-#endif=0D
=0D
//UINTN=0D
//ArmPlatformGetPrimaryCoreMpId (=0D
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4=
/RPi4.dsc=0D
index 2c05c31118d2..ff802d8347ea 100644=0D
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc=0D
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc=0D
@@ -429,7 +429,6 @@ [PcdsFixedAtBuild.common]=0D
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4=0D
- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8=0D
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200=0D
@@ -465,6 +464,9 @@ [PcdsFixedAtBuild.common]=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE=0D
=0D
+[PcdsPatchableInModule]=0D
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000=0D
+=0D
[PcdsDynamicHii.common.DEFAULT]=0D
=0D
#=0D
@@ -621,7 +623,7 @@ [Components.common]=0D
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf=0D
MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {=0D
<LibraryClasses>=0D
- SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSer=
ialPortLib.inf=0D
+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSer=
ialPortDxeLib.inf=0D
}=0D
Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf=0D
EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf=0D
-- 2.29.2.windows.2=0D


Re: [GSoC proposal] Secure Image Loader

Laszlo Ersek
 

On 04/06/21 12:06, Marvin Häuser wrote:
Good day Nate,

Comments are inline.

Best regards,
Marvin

On 06.04.21 11:41, Nate DeSimone wrote:
Hi Marvin,

Great to meet you and welcome back! Glad you hear you are interested!
Completing a formal verification of a PE/COFF loader is certainly
impressive. Was this done with some sort of automated theorem proving?
It would seem a rather arduous task doing an inductive proof for an
algorithm like that by hand!
I would call it "semi-automated", a great deal of intermediate goals
(preconditions, postconditions, invariants, assertions, ...) were
required to show all interesting properties. But yes, the actual proof
steps are automated by common SMT solvers. It was done using the
AstraVer Toolset and ACSL, latter basically a language to express logic
statements with C-like syntax.

I completely agree with you that getting a formally verified PE/COFF
loader into mainline is undoubtably valuable and would pay security
dividends for years to come.
I'm glad to hear that. :)

Admittedly, this is an area of computer science that I don't have a
great deal of experience with. The furthest I have gone on this topic
is writing out proofs for simple algorithms on exams in my Algorithms
class in college. Regardless you have a much better idea of what the
current status is of the work that you and Vitaly have done. I guess
my only question is do you think there is sufficient work remaining to
fill the 10 week GSoC development window?
Please don't get me wrong, but I would be surprised if the UEFI
specification changes I'd like to discuss alone would be completed
within 10 weeks, let alone implementation throughout the codebase. While
I think the plain amount of code may be a bit less than say a
MinPlatform port, the changes are much deeper and require much more
caution to avoid regressions (e.g. by invalidating undocumented
assertions). This sadly is not a matter of just replacing the underlying
library implementation or "plug-in and play" at all. It furthermore
affects many parts of the stack, the core dispatchers used for all
platforms, image emulation (EBC), UEFI userland emulation (EmuPkg), and
so on. I was rather worried the scope is too broad time-wise, but it can
be narrowed/widened as you see fit really. This is one of *the* core
components used on millions of device, and many package maintainers need
to review and validate the changes, this must really be done right the
first try. :)

Certainly we can use some of that time to perform the code reviews you
mention and write up formal ECRs for the UEFI spec changes that you
believe are needed.
I believed that was part of the workload, yes, but even without it I
think there is plenty to do.

Thank you for sending the application and alerting us to the great
work you and Vitaly have done! I'll read your paper more closely and
come back with any questions I still have.
Thank you, I will gladly explain anything unclear. Just try to not give
Laszlo too many flashbacks. :)
I haven't commented yet in this thread, as I thought my stance on this
undertaking was (or should be) obvious.

I very much welcome a replacement for the PE/COFF parser (as I consider
its security issues unfixable in an incremental manner). From my reading
of Marvin's and Vitaly's paper (draft), they have my full trust, and I'm
ready to put their upcoming code to use in ArmVirtPkg and OvmfPkg with
minimal actual code review. If fixing the pervasive security problems
around this area cannot avoid spiraling out to other core code in edk2,
such as dispatchers, and even to the PI / UEFI specs, so be it.

Regarding GSoC itself: as I stated elsewhere previously, I support
edk2's participation in GSoC, while at the same time I'm not
volunteering for mentorship at all. I'm uncertain if GSoC is the best
framework for upstreaming such a large undertaking, but if it can help,
we should use it as much as possible.

Thanks
Laszlo







With Best Regards,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser
Sent: Sunday, April 4, 2021 4:02 PM
To: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Andrew Fish
<afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [edk2-devel] [GSoC proposal] Secure Image Loader

Good day everyone,

I'll keep the introduction brief because I've been around for a while
now. :) I'm
Marvin Häuser, a third-year Computer Science student from TU
Kaiserslautern,
Germany. Late last year, my colleague Vitaly from ISP RAS and me
introduced a
formally verified Image Loader for UEFI usage at ISP RAS Open[1] due
to various
defects we outlined in the corresponding paper. Thank you once again
Laszlo
for your *incredible* review work on the publication part.

I now want to make an effort to mainline it, preferably as part of
the current
Google Summer of Code event. To be clear, my internship at ISP RAS has
concluded, and while Vitaly will be available for design discussion,
he has other
priorities at the moment and the practical part will be on me. I have
previously
submitted a proposal via the GSoC website for your review.

There are many things to consider:
1. The Image Loader is a core component, and there needs to be a
significant
level of quality and security assurance.
2. Being consumed by many packages, the proposed patch set will take
a lot of
time to review and integrate.
3. During my initial exploration, I discovered defective PPIs and
protocols (e.g.
returning data with no corresponding size) originating from the UEFI
PI and
UEFI specifications. Changes need to be discussed, settled on, and
submitted to
the UEFI Forum.
4. Some UEFI APIs like the Security Architecture protocols are
inconveniently
abstract, see 5.
5. Some of the current code does not use the existing context, or
accesses it
outside of the exposed APIs. The control flow of the dispatchers may
need to be
adapted to make the context available to appropriate APIs.

But obviously there are not only unpleasant considerations:
A. The Image Loader is mostly formally verified, and only very few
changes will
be required from the last proven state. This gives a lot of trust in
its correctness
and safety.
B. All outlined defects that are of critical nature have been fixed
successfully.
C. The Image Loader has been tested with real-world code loading
real-world
OSes on thousands of machines in the past few months, including
rejecting
malformed images (configurable by PCD).
D. The new APIs will centralise everything PE, reducing code
duplication and
potentially unsafe operations.
E. Centralising and reduced parse duplication may improve overall boot
performance.
F. The code has been coverage-tested to not contain dead code.
G. The code has been fuzz-tested including sanitizers to not invoke
undefined
behaviour.
H. I already managed to identify a malformed image in OVMF with its help
(incorrectly reported section alignment of an Intel IPXE driver). A
fix will be
submitted shortly.
I. I plan to support PE section permissions, allowing for read-only data
segments when enabled.

There are likely more points for both lists, but I hope this gives a
decent
starting point for discussion. What are your thoughts on the matter?
I strongly
encourage everyone to read the section regarding defects of our
publication[2]
to better understand the motivation. The vague points above can of
course be
elaborated in due time, as you see fit.

Thank you for your time!

Best regards,
Marvin


[1] https://github.com/mhaeuser/ISPRASOpen-SecurePE
[2] https://arxiv.org/pdf/2012.05471.pdf







Re: [edk2-platforms][PATCH v2 1/1] Platform/RaspberryPi: Fix mini UART clock divisor calculation

Ard Biesheuvel
 

On Thu, 8 Apr 2021 at 11:47, Pete Batard <pete@akeo.ie> wrote:

On 2021.04.06 15:04, Mario Bălănică wrote:
The VPU clock divisor has changed in this commit:
https://github.com/raspberrypi/firmware/commit/1e5456a,
thus breaking the mini UART clock divisor calculation on the Pi 4.

Fix this by reading the core clock from the mailbox instead.

Signed-off-by: Mario Bălănică <mariobalanica02@gmail.com>
---
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 15 +++------------
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 4 ----
Platform/RaspberryPi/RPi4/RPi4.dsc | 6 ++++--
3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
index 5e83bbf022eb..d2f983bf0a9f 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
@@ -34,25 +34,16 @@ SerialPortGetDivisor (
UINT32 SerialBaudRate

)

{

- UINT64 BaseClockRate;

+ UINT32 BaseClockRate;

UINT32 Divisor;



- //

- // On the Raspberry Pi, the clock to use for the 16650-compatible UART

- // is the base clock divided by the 12.12 fixed point VPU clock divisor.

- //

- BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate);

-#if (RPI_MODEL == 4)

- Divisor = MmioRead32(BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;

- if (Divisor != 0)

- BaseClockRate = (BaseClockRate << 12) / Divisor;

-#endif

+ BaseClockRate = PcdGet32 (PcdSerialClockRate);



//

// As per the BCM2xxx datasheets:

// baudrate = system_clock_freq / (8 * (divisor + 1)).

//

- Divisor = (UINT32)BaseClockRate / (SerialBaudRate * 8);

+ Divisor = BaseClockRate / (SerialBaudRate * 8);

if (Divisor != 0) {

Divisor--;

}

diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
index 58351e4fb8cc..7008aaf8aa4c 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
+++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
@@ -85,13 +85,11 @@ ASM_FUNC (ArmPlatformPeiBootAction)
adr x2, mBoardRevision

str w0, [x2]



-#if (RPI_MODEL == 3)

run .Lclkinfo_buffer



ldr w0, .Lfrequency

adr x2, _gPcd_BinaryPatch_PcdSerialClockRate

str w0, [x2]

-#endif



ret



@@ -135,7 +133,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // end tag

.set .Lrevinfo_size, . - .Lrevinfo_buffer



-#if (RPI_MODEL == 3)

.align 4

.Lclkinfo_buffer:

.long .Lclkinfo_size

@@ -148,7 +145,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // frequency

.long 0 // end tag

.set .Lclkinfo_size, . - .Lclkinfo_buffer

-#endif



//UINTN

//ArmPlatformGetPrimaryCoreMpId (

diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 2c05c31118d2..ff802d8347ea 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -429,7 +429,6 @@ [PcdsFixedAtBuild.common]
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4

- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8

gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200

@@ -465,6 +464,9 @@ [PcdsFixedAtBuild.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"

gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE



+[PcdsPatchableInModule]

+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000

+

[PcdsDynamicHii.common.DEFAULT]



#

@@ -621,7 +623,7 @@ [Components.common]
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf

MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {

<LibraryClasses>

- SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf

+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf

}

Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf

EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf
Reviewed-by: Pete Batard <pete@akeo.ie>
Tested-by: Pete Batard <pete@akeo.ie>
Thanks Pete.

Could anyone get me a version of this patch that is not whitespace
mangled? This one does not apply with 'git am'


Re: [GSoC proposal] Secure Image Loader

Michael Brown
 

On 08/04/2021 11:13, Marvin Häuser wrote:
Very much less perfect.  The mere existence of such an option immediately reimposes the burden on external code to support both, because it opens up the possibility of running on systems where the option is set to false.
One more time, I do not know why any non-platform code would call those APIs. Preferably, they would be private implementation details to me.
If you are talking about private APIs that are not even exposed at the UEFI specification level (i.e. do not have an EFI_XXX_PROTOCOL name and well-known GUID) then there's no issue.

If they are defined as public APIs (i.e. things that do have an EFI_XXX_PROTOCOL name and well-known GUID) then you must assume that some external code, somewhere, will use them.

Which is the case?

As a practical consideration: unless there is a security reason to do otherwise, you should almost certainly relax the constraints on images that your loader will accept, to avoid causing unnecessary end-user disruption.  What is the *security* reason behind your alignment requirements (which clearly are not required by any other toolchain, including those used for signing Secure Boot binaries)?
Sorry if that was not clear from the PR, I hoped it was. It's the fact that memory permissions can only be enforced page-wise.
Perfect; thank you. So: the security requirement is that memory permissions must change only at page boundaries.

I furthermore hope that, at some point, both iPXE and shim switch to EDK II for PE generation to ensure consistency of the binary generation.
There is zero chance of ever pulling the EDK2 build system into iPXE. It's too large, too painful to use, and doesn't support the full range of target platforms required (UEFI, BIOS, and Linux userspace).

If EDK2 publishes a tool for converting ELF to PE, and that tool becomes generally available in Linux distros, then I'd be happy to drop elf2efi.c and switch to the EDK2 tool about five years from now once it's safe to assume its existence on any viable build platform.

This may not be quite the answer you were hoping for, but it's the only practical one.

Thanks,

Michael


Re: [GSoC proposal] Secure Image Loader

Marvin Häuser
 

On 08.04.21 11:55, Michael Brown wrote:
On 08/04/2021 10:41, Marvin Häuser wrote:
No, backwards-compatibility will not be broken in the sense that the old API is absent or malfunctioning.
Perfect. :)

As I *have* said, I imagine there to be an option (default true) to expose both variants.
Very much less perfect.  The mere existence of such an option immediately reimposes the burden on external code to support both, because it opens up the possibility of running on systems where the option is set to false.
One more time, I do not know why any non-platform code would call those APIs. Preferably, they would be private implementation details to me. I understand that you are displeased with your maintenance burden in iPXE, and I can assure you, you are not alone. We want to support hotswap with one of our UEFI applications, and I am currently contemplating overriding Uninstall(Multiple)ProtocolInterface(s) to try to ensure security. I hear you, totally. :)

With default settings, I want the loader to be at the very least mostly plug-'n'-play with existing platform drivers and OS loaders from the real world. "Mostly" can be clarified further once we have a detailed plan on the changes (and responses to e.g. malformed binary issues with iPXE and GNU-EFI).
Yes; thank you for https://github.com/ipxe/ipxe/pull/313.  It will take some time to review.

As a practical consideration: unless there is a security reason to do otherwise, you should almost certainly relax the constraints on images that your loader will accept, to avoid causing unnecessary end-user disruption.  What is the *security* reason behind your alignment requirements (which clearly are not required by any other toolchain, including those used for signing Secure Boot binaries)?
Sorry if that was not clear from the PR, I hoped it was. It's the fact that memory permissions can only be enforced page-wise. So, when two sections with different permissions share a page, at the very least this page must be applied with relaxed permissions. I do not like relaxing permissions. :)

There already is a PCD to relax this, and both iPXE and GNU-EFI images load correctly and securely with it. Just the more relaxed loading is, the more awkward cases need to be considered when applying memory permission attributes. Logically, as the original ELF was correctly aligned segment-wise, the case I described above will not really happen. Yet it is now the firmware's burden to check all sections with overlapping pages for their attributes and adjust. As, please do not take this offensively, it is "only" iPXE images and the GNU shim loader affected so far, I hope that in due time all images can be updated (the shim can be used for older releases of any distribution as well!) and the constraints be tightened. Yet, of course, this is up to the EDK II maintainers to decide.

I furthermore hope that, at some point, both iPXE and shim switch to EDK II for PE generation to ensure consistency of the binary generation.

Best regards,
Marvin


Thanks,

Michael




Re: [RFC PATCH 00/19] Add AMD Secure Nested Paging (SEV-SNP) support

Laszlo Ersek
 

Hi Brijesh,

On 03/24/21 16:31, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

SEV-SNP builds upon existing SEV and SEV-ES functionality while adding
new hardware-based memory protections. SEV-SNP adds strong memory integrity
protection to help prevent malicious hypervisor-based attacks like data
replay, memory re-mapping and more in order to create an isolated memory
encryption environment.

This series provides the basic building blocks to support booting the SEV-SNP
VMs, it does not cover all the security enhancement introduced by the SEV-SNP
such as interrupt protection.

Many of the integrity guarantees of SEV-SNP are enforced through a new
structure called the Reverse Map Table (RMP). Adding a new page to SEV-SNP
VM requires a 2-step process. First, the hypervisor assigns a page to the
guest using the new RMPUPDATE instruction. This transitions the page to
guest-invalid. Second, the guest validates the page using the new PVALIDATE
instruction. The SEV-SNP VMs can use the new "Page State Change Request NAE"
defined in the GHCB specification to ask hypervisor to add or remove page
from the RMP table.

Each page assigned to the SEV-SNP VM can either be validated or unvalidated,
as indicated by the Validated flag in the page's RMP entry. There are two
approaches that can be taken for the page validation: Pre-validation and
Lazy Validation.

Under pre-validation, the pages are validated prior to first use. And under
lazy validation, pages are validated when first accessed. An access to a
unvalidated page results in a #VC exception, at which time the exception
handler may validate the page. Lazy validation requires careful tracking of
the validated pages to avoid validating the same GPA more than once. The
recently introduced "Unaccepted" memory type can be used to communicate the
unvalidated memory ranges to the Guest OS.

At this time we only support the pre-validation. OVMF detects all the available
system RAM in the PEI phase. When SEV-SNP is enabled, the memory is validated
before it is made available to the EDK2 core.
Can you describe this in a bit more detail, before I look at the
individual patches? Specifically, what existing logic in the PEI phase
was taken, and extended, and how?

If there is a particular patch whose commit message is closely related
to my question, can you point it out? Patch#15 perhaps? (Doesn't seem
like a big patch; for some reason I'd expect something more complex, but
perhaps that's only because it builds upon the many earlier patches.)

Thanks,
Laszlo


This series does not implements the following SEV-SNP features yet:

* CPUID filtering
* AP bring up using the new SEV-SNP NAE
* Lazy validation
* Interrupt security

The series is based on commit:
e542e05d4f UefiCpuPkg/SmmCpuFeaturesLib: Abstract PcdCpuMaxLogicalProcessorNumber

Additional resources
---------------------
SEV-SNP whitepaper
https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf

APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)

The complete source is available at
https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-1

GHCB spec v2:
The draft specification is posted on AMD-SEV-SNP mailing list:
https://lists.suse.com/mailman/private/amd-sev-snp/

Copy of the spec is also available at
https://github.com/AMDESE/AMDSEV/blob/sev-snp-devel/docs/56421-Guest_Hypervisor_Communication_Block_Standardization.pdf

GHCB spec v1:
SEV-SNP firmware specification:
https://developer.amd.com/sev/

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>

Brijesh Singh (19):
OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest
OvmfPkg: validate the data pages used in the SEC phase
MdePkg: Expand the SEV MSR to include the SNP definition
OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled()
MdePkg: Define the GHCB GPA structure
UefiCpuPkg/MpLib: add support to register GHCB GPA when SEV-SNP is
enabled
OvmfPkg: Add a library to support registering GHCB GPA
OvmfPkg: register GHCB gpa for the SEV-SNP guest
MdePkg: Add AsmPvalidate() support
OvmfPkg: Define the Page State Change VMGEXIT structures
OvmfPkg/ResetVector: Invalidate the GHCB page
OvmfPkg/MemEncryptSevLib: Add support to validate system RAM
OvmfPkg/SecMain: Validate the data/code pages used for the PEI phase
OvmfPkg/MemEncryptSevLib: Add support to validate RAM in PEI phase
OvmfPkg/PlatformPei: Validate the system RAM when SNP is active
OvmfPkg/MemEncryptSevLib: Add support to validate > 4GB memory in PEI
phase
OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase
OvmfPkg/MemEncryptSevLib: Validate the memory during set or clear enc
attribute
OvmfPkg/MemEncryptSevLib: Skip page state change for non RAM region

MdePkg/Include/Library/BaseLib.h | 37 +++
MdePkg/Include/Register/Amd/Fam17Msr.h | 31 ++-
MdePkg/Include/Register/Amd/Ghcb.h | 39 ++-
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 43 +++
OvmfPkg/Include/Library/GhcbRegisterLib.h | 27 ++
OvmfPkg/Include/Library/MemEncryptSevLib.h | 30 +++
.../DxeMemEncryptSevLib.inf | 7 +
.../DxeMemEncryptSevLibInternal.c | 27 ++
.../Ia32/SnpPageStateChange.c | 17 ++
.../PeiMemEncryptSevLib.inf | 9 +
.../PeiMemEncryptSevLibInternal.c | 47 ++++
.../SecMemEncryptSevLib.inf | 4 +
.../SecMemEncryptSevLibInternal.c | 39 +++
.../BaseMemEncryptSevLib/SnpPageStateChange.h | 37 +++
.../X64/PeiDxeSnpSetPageState.c | 63 +++++
.../X64/PeiDxeVirtualMemory.c | 151 ++++++++++-
.../X64/PeiSnpSystemRamValidate.c | 129 +++++++++
.../X64/SecSnpSystemRamValidate.c | 23 ++
.../X64/SnpPageStateChangeInternal.c | 254 ++++++++++++++++++
.../X64/SnpPageStateTrack.c | 119 ++++++++
.../X64/SnpPageStateTrack.h | 36 +++
.../X64/SnpSetPageState.h | 27 ++
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 19 ++
.../Library/GhcbRegisterLib/GhcbRegisterLib.c | 97 +++++++
.../GhcbRegisterLib/GhcbRegisterLib.inf | 33 +++
OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf | 4 +
OvmfPkg/Library/VmgExitLib/VmgExitLib.inf | 7 +
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 45 ++++
OvmfPkg/OvmfPkg.dec | 12 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfPkgX64.fdf | 33 ++-
OvmfPkg/PlatformPei/AmdSev.c | 52 ++++
OvmfPkg/PlatformPei/PlatformPei.inf | 2 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 24 ++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 106 ++++++++
OvmfPkg/ResetVector/ResetVector.inf | 5 +
OvmfPkg/ResetVector/ResetVector.nasmb | 4 +
OvmfPkg/Sec/SecMain.c | 102 +++++++
OvmfPkg/Sec/SecMain.inf | 2 +
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 1 +
UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 +
UefiCpuPkg/Library/MpInitLib/MpLib.h | 2 +
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 51 ++++
UefiCpuPkg/UefiCpuPkg.dec | 6 +
47 files changed, 1790 insertions(+), 19 deletions(-)
create mode 100644 MdePkg/Library/BaseLib/X64/Pvalidate.nasm
create mode 100644 OvmfPkg/Include/Library/GhcbRegisterLib.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/SnpPageStateChange.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/SnpPageStateChange.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeSnpSetPageState.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateTrack.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateTrack.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpSetPageState.h
create mode 100644 OvmfPkg/Library/GhcbRegisterLib/GhcbRegisterLib.c
create mode 100644 OvmfPkg/Library/GhcbRegisterLib/GhcbRegisterLib.inf


Re: [GSoC proposal] Secure Image Loader

Michael Brown
 

On 08/04/2021 10:41, Marvin Häuser wrote:
No, backwards-compatibility will not be broken in the sense that the old API is absent or malfunctioning.
Perfect. :)

As I *have* said, I imagine there to be an option (default true) to expose both variants.
Very much less perfect. The mere existence of such an option immediately reimposes the burden on external code to support both, because it opens up the possibility of running on systems where the option is set to false.

With default settings, I want the loader to be at the very least mostly plug-'n'-play with existing platform drivers and OS loaders from the real world. "Mostly" can be clarified further once we have a detailed plan on the changes (and responses to e.g. malformed binary issues with iPXE and GNU-EFI).
Yes; thank you for https://github.com/ipxe/ipxe/pull/313. It will take some time to review.

As a practical consideration: unless there is a security reason to do otherwise, you should almost certainly relax the constraints on images that your loader will accept, to avoid causing unnecessary end-user disruption. What is the *security* reason behind your alignment requirements (which clearly are not required by any other toolchain, including those used for signing Secure Boot binaries)?

Thanks,

Michael


Re: [GSoC proposal] Secure Image Loader

Marvin Häuser
 

Sorry, I accidentally removed an inline comment when sending.

Best regards,
Marvin

On 08.04.21 11:41, Marvin Häuser wrote:
Well, I assume this is a misunderstanding. I understood your usage of "workaround" to be supporting both *_PROTOCOL and *2_PROTOCOL instances. Yes, backwards-compatibility will be broken in the sense that the new interface will not be compatible with the old interface. No, backwards-compatibility will not be broken in the sense that the old API is absent or malfunctioning. As I *have* said, I imagine there to be an option (default true) to expose both variants. With default settings, I want the loader to be at the very least mostly plug-'n'-play with existing platform drivers and OS loaders from the real world. "Mostly" can be clarified further once we have a detailed plan on the changes (and responses to e.g. malformed binary issues with iPXE and GNU-EFI).

Best regards,
Marvin

On 08.04.21 11:26, Michael Brown wrote:
On 08/04/2021 09:53, Marvin Häuser wrote:
On 07.04.21 23:50, Michael Brown wrote:
InstallMultipleProtocolInterfaces() is not a breaking change: the existence of InstallMultipleProtocolInterfaces() does not require any change to the way that InstallProtocolInterface() is implemented or consumed.

Code written before the invention of InstallMultipleProtocolInterfaces() will still work now, with no modifications required.
The same is true for the *2_PROTOCOL instances, I do not see how you distinct between them. Could you elaborate, please?
The distinction is very straightforward.  If you plan to *remove* support for the older protocols, then you by definition place a burden on all externally maintained code to support both protocols.  If the older protocol will continue to be supported then no such burden is created.

This is why I am asking you if your proposed changes require *breaking* backwards compatibility.  You still haven't answered this key question.

You have to remember that UEFI is not a monolithic codebase with a single maintaining organisation. Implementing a *2_PROTOCOL and deprecating the original just causes pain for all the code in the world that is maintained outside of the EDK2 repository, since that code now has to support *both* the old and new protocols.
I am aware, but actually it's not far from it nowadays. In contrast to the days of Aptio IV, I believe all big vendors maintain forks of EDK II. I know the fork nature taints the issue, but also see last quote comment.
This is empirically not true.  Buy a selection of devices in the wild, and you'll find a huge amount of non-EDK2 code on them.
It is not about "how much" is EDK II code, but that it is the core. We are talking about things like the dispatcher, I have not ever seen it modified "lately" (Gigabyte modded AMI CORE_PEI and CORE_DXE with their SIO code in Z77, but you get the idea...). I am very well aware of issues with "user-facing" things such as input.


I would be extremely happy if every vendor just used the EDK2 code: it would make my life a lot easier.  But it's not what happens in the real world.

I see that there is no EFI_USB_IO2_PROTOCOL instance to argue by. Yet there are EFI_BLOCK_IO2_PROTOCOL and EFI_LOAD_FILE2_PROTOCOL. Former, in my opinion, close in nature to your your example, and latter close to the nature on what I plan to propose. Sorry, but I do not see how what I suggest has not been done multiple times in the past already.

Please also look at it from an angle of trust. How can I trust the "secure" advertisements of UEFI / EDK II when the specification *dictates* the usage of intrinsically insecure APIs?
The consequence for security-critical situations would be to necessarily abandon UEFI and come up with a new design. In who's interest is this?
Again, we return to the question that you have not yet answered: do your proposed changes require breaking backwards compatibility?

Please do answer this question: if you're *not* proposing to break the platform in a way that would prevent older binaries from working without modification, then we have no disagreement! :)

Don't get me wrong: I *am* in favour of improving the security of EDK2, and a formally verified loader is potentially useful for that. But I could only consider it a good idea if it can be done without making breaking API changes for which I know I will personally have to maintain workarounds for the next ten years.
Sorry, but you seem to have missed my points regarding these concerns, at least you did not address them I believe. I don't know why you need to (actively) maintain workarounds for APIs external code has no reason to use, especially when the legacy implementation would quite literally be a wrapper function.
<same comment as above>

Thanks,

Michael


Re: [PATCH 3/3] Platform/RaspberryPi/AcpiTables: Correct _DMA consumer

Pete Batard
 

On 2021.04.08 06:58, Jeremy Linton wrote:
Bridge devices should be marked as producers so that their
children can consume the resources. In linux if this isn't
true then the translation gets ignored and the DMA values
are incorrect. This fixes DMA on all the devices that
need a translation.
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
Platform/RaspberryPi/AcpiTables/Dsdt.asl | 2 +-
Platform/RaspberryPi/AcpiTables/Emmc.asl | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Platform/RaspberryPi/AcpiTables/Dsdt.asl b/Platform/RaspberryPi/AcpiTables/Dsdt.asl
index d116f965e1..32cd5fc9f9 100644
--- a/Platform/RaspberryPi/AcpiTables/Dsdt.asl
+++ b/Platform/RaspberryPi/AcpiTables/Dsdt.asl
@@ -205,7 +205,7 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5, "RPIFDN", "RPI", 2)
// Only the first GB is available.
// Bus 0xC0000000 -> CPU 0x00000000.
//
- QWordMemory (ResourceConsumer,
+ QWordMemory (ResourceProducer,
,
MinFixed,
MaxFixed,
diff --git a/Platform/RaspberryPi/AcpiTables/Emmc.asl b/Platform/RaspberryPi/AcpiTables/Emmc.asl
index 179dd3ecdb..0fbc2a79ea 100644
--- a/Platform/RaspberryPi/AcpiTables/Emmc.asl
+++ b/Platform/RaspberryPi/AcpiTables/Emmc.asl
@@ -32,7 +32,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4EMMC", 2)
}
Name (_DMA, ResourceTemplate() {
- QWordMemory (ResourceConsumer,
+ QWordMemory (ResourceProducer,
,
MinFixed,
MaxFixed,
Reviewed-by: Pete Batard <pete@akeo.ie>


Re: [PATCH 2/3] Platform/RaspberryPi/AcpiTables: Add further named components

Pete Batard
 

On 2021.04.08 06:58, Jeremy Linton wrote:
Add some additional IORT nodes for the USB & EMMC devices, realistically
we probably only need to have a single node with the lowest AddressSizeLimit
but this is conceptually "cleaner" should anyone actually try and use these
values rather than the _DMA provided ones.
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
Platform/RaspberryPi/AcpiTables/Iort.aslc | 44 ++++++++++++++++++++++++++++++-
1 file changed, 43 insertions(+), 1 deletion(-)
diff --git a/Platform/RaspberryPi/AcpiTables/Iort.aslc b/Platform/RaspberryPi/AcpiTables/Iort.aslc
index 00720194bb..810307ae37 100644
--- a/Platform/RaspberryPi/AcpiTables/Iort.aslc
+++ b/Platform/RaspberryPi/AcpiTables/Iort.aslc
@@ -20,6 +20,8 @@ typedef struct {
typedef struct {
EFI_ACPI_6_0_IO_REMAPPING_TABLE Iort;
RPI4_NC_NODE NamedCompNode;
+ RPI4_NC_NODE NamedCompNode2;
+ RPI4_NC_NODE NamedCompNode3;
} RPI4_IO_REMAPPING_STRUCTURE;
STATIC RPI4_IO_REMAPPING_STRUCTURE Iort = {
@@ -27,7 +29,7 @@ STATIC RPI4_IO_REMAPPING_STRUCTURE Iort = {
ACPI_HEADER (EFI_ACPI_6_0_IO_REMAPPING_TABLE_SIGNATURE,
RPI4_IO_REMAPPING_STRUCTURE,
EFI_ACPI_IO_REMAPPING_TABLE_REVISION),
- 1, // NumNodes
+ 3, // NumNodes
sizeof (EFI_ACPI_6_0_IO_REMAPPING_TABLE), // NodeOffset
0 // Reserved
}, {
@@ -50,6 +52,46 @@ STATIC RPI4_IO_REMAPPING_STRUCTURE Iort = {
}, {
"\\_SB_.SCB0.XHC0" // ObjectName
}
+ }, {
+ // gpu/dwc usb named component node
+ {
+ {
+ EFI_ACPI_IORT_TYPE_NAMED_COMP, // Type
+ sizeof (RPI4_NC_NODE), // Length
+ 0x0, // Revision
+ 0x0, // Reserved
+ 0x0, // NumIdMappings
+ 0x0, // IdReference
+ },
+ 0x0, // Flags
+ 0x0, // CacheCoherent
+ 0x0, // AllocationHints
+ 0x0, // Reserved
+ 0x0, // MemoryAccessFlags
+ 30, // AddressSizeLimit
+ }, {
+ "\\_SB_.GDV0.USB0" // ObjectName
+ }
+ }, {
+ // emmc2 named component node
+ {
+ {
+ EFI_ACPI_IORT_TYPE_NAMED_COMP, // Type
+ sizeof (RPI4_NC_NODE), // Length
+ 0x0, // Revision
+ 0x0, // Reserved
+ 0x0, // NumIdMappings
+ 0x0, // IdReference
+ },
+ 0x0, // Flags
+ 0x0, // CacheCoherent
+ 0x0, // AllocationHints
+ 0x0, // Reserved
+ 0x0, // MemoryAccessFlags
+ 30, // AddressSizeLimit
+ }, {
+ "\\_SB_.GDV1.SDC3" // ObjectName
+ }
}
};
Reviewed-by: Pete Batard <pete@akeo.ie>


Re: [PATCH 1/3] Platform/RaspberryPi/Acpitables: Enable Arasan hispeed mode

Pete Batard
 

On 2021.04.08 06:58, Jeremy Linton wrote:
The arasan caps registers are no longer being overridden by
the brcm iproc driver, so we should be assuring that the
"High Speed Support" bit 21 is set in the capability
register. This significantly improves the wifi perf
using linux.
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
Platform/RaspberryPi/AcpiTables/Sdhc.asl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Platform/RaspberryPi/AcpiTables/Sdhc.asl b/Platform/RaspberryPi/AcpiTables/Sdhc.asl
index 0430ab7d2d..42776e33bb 100644
--- a/Platform/RaspberryPi/AcpiTables/Sdhc.asl
+++ b/Platform/RaspberryPi/AcpiTables/Sdhc.asl
@@ -52,7 +52,7 @@ Device (SDC1)
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
- Package () { "sdhci-caps", 0x0100fa81 },
+ Package () { "sdhci-caps", 0x0120fa81 },
}
})
Reviewed-by: Pete Batard <pete@akeo.ie>


Re: [edk2-platforms][PATCH v2 1/1] Platform/RaspberryPi: Fix mini UART clock divisor calculation

Pete Batard
 

On 2021.04.06 15:04, Mario Bălănică wrote:
The VPU clock divisor has changed in this commit:
https://github.com/raspberrypi/firmware/commit/1e5456a,
thus breaking the mini UART clock divisor calculation on the Pi 4.
Fix this by reading the core clock from the mailbox instead.
Signed-off-by: Mario Bălănică <mariobalanica02@gmail.com>
---
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 15 +++------------
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 4 ----
Platform/RaspberryPi/RPi4/RPi4.dsc | 6 ++++--
3 files changed, 7 insertions(+), 18 deletions(-)
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
index 5e83bbf022eb..d2f983bf0a9f 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
@@ -34,25 +34,16 @@ SerialPortGetDivisor (
UINT32 SerialBaudRate
)
{
- UINT64 BaseClockRate;
+ UINT32 BaseClockRate;
UINT32 Divisor;
- //
- // On the Raspberry Pi, the clock to use for the 16650-compatible UART
- // is the base clock divided by the 12.12 fixed point VPU clock divisor.
- //
- BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate);
-#if (RPI_MODEL == 4)
- Divisor = MmioRead32(BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
- if (Divisor != 0)
- BaseClockRate = (BaseClockRate << 12) / Divisor;
-#endif
+ BaseClockRate = PcdGet32 (PcdSerialClockRate);
//
// As per the BCM2xxx datasheets:
// baudrate = system_clock_freq / (8 * (divisor + 1)).
//
- Divisor = (UINT32)BaseClockRate / (SerialBaudRate * 8);
+ Divisor = BaseClockRate / (SerialBaudRate * 8);
if (Divisor != 0) {
Divisor--;
}
diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
index 58351e4fb8cc..7008aaf8aa4c 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
+++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
@@ -85,13 +85,11 @@ ASM_FUNC (ArmPlatformPeiBootAction)
adr x2, mBoardRevision
str w0, [x2]
-#if (RPI_MODEL == 3)
run .Lclkinfo_buffer
ldr w0, .Lfrequency
adr x2, _gPcd_BinaryPatch_PcdSerialClockRate
str w0, [x2]
-#endif
ret
@@ -135,7 +133,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // end tag
.set .Lrevinfo_size, . - .Lrevinfo_buffer
-#if (RPI_MODEL == 3)
.align 4
.Lclkinfo_buffer:
.long .Lclkinfo_size
@@ -148,7 +145,6 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // frequency
.long 0 // end tag
.set .Lclkinfo_size, . - .Lclkinfo_buffer
-#endif
//UINTN
//ArmPlatformGetPrimaryCoreMpId (
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 2c05c31118d2..ff802d8347ea 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -429,7 +429,6 @@ [PcdsFixedAtBuild.common]
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4
- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
@@ -465,6 +464,9 @@ [PcdsFixedAtBuild.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
+[PcdsPatchableInModule]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
+
[PcdsDynamicHii.common.DEFAULT]
#
@@ -621,7 +623,7 @@ [Components.common]
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
<LibraryClasses>
- SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf
}
Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf
EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf
Reviewed-by: Pete Batard <pete@akeo.ie>
Tested-by: Pete Batard <pete@akeo.ie>


Re: [GSoC proposal] Secure Image Loader

Marvin Häuser
 

Well, I assume this is a misunderstanding. I understood your usage of "workaround" to be supporting both *_PROTOCOL and *2_PROTOCOL instances. Yes, backwards-compatibility will be broken in the sense that the new interface will not be compatible with the old interface. No, backwards-compatibility will not be broken in the sense that the old API is absent or malfunctioning. As I *have* said, I imagine there to be an option (default true) to expose both variants. With default settings, I want the loader to be at the very least mostly plug-'n'-play with existing platform drivers and OS loaders from the real world. "Mostly" can be clarified further once we have a detailed plan on the changes (and responses to e.g. malformed binary issues with iPXE and GNU-EFI).

Best regards,
Marvin

On 08.04.21 11:26, Michael Brown wrote:
On 08/04/2021 09:53, Marvin Häuser wrote:
On 07.04.21 23:50, Michael Brown wrote:
InstallMultipleProtocolInterfaces() is not a breaking change: the existence of InstallMultipleProtocolInterfaces() does not require any change to the way that InstallProtocolInterface() is implemented or consumed.

Code written before the invention of InstallMultipleProtocolInterfaces() will still work now, with no modifications required.
The same is true for the *2_PROTOCOL instances, I do not see how you distinct between them. Could you elaborate, please?
The distinction is very straightforward.  If you plan to *remove* support for the older protocols, then you by definition place a burden on all externally maintained code to support both protocols.  If the older protocol will continue to be supported then no such burden is created.

This is why I am asking you if your proposed changes require *breaking* backwards compatibility.  You still haven't answered this key question.

You have to remember that UEFI is not a monolithic codebase with a single maintaining organisation. Implementing a *2_PROTOCOL and deprecating the original just causes pain for all the code in the world that is maintained outside of the EDK2 repository, since that code now has to support *both* the old and new protocols.
I am aware, but actually it's not far from it nowadays. In contrast to the days of Aptio IV, I believe all big vendors maintain forks of EDK II. I know the fork nature taints the issue, but also see last quote comment.
This is empirically not true.  Buy a selection of devices in the wild, and you'll find a huge amount of non-EDK2 code on them.

I would be extremely happy if every vendor just used the EDK2 code: it would make my life a lot easier.  But it's not what happens in the real world.

I see that there is no EFI_USB_IO2_PROTOCOL instance to argue by. Yet there are EFI_BLOCK_IO2_PROTOCOL and EFI_LOAD_FILE2_PROTOCOL. Former, in my opinion, close in nature to your your example, and latter close to the nature on what I plan to propose. Sorry, but I do not see how what I suggest has not been done multiple times in the past already.

Please also look at it from an angle of trust. How can I trust the "secure" advertisements of UEFI / EDK II when the specification *dictates* the usage of intrinsically insecure APIs?
The consequence for security-critical situations would be to necessarily abandon UEFI and come up with a new design. In who's interest is this?
Again, we return to the question that you have not yet answered: do your proposed changes require breaking backwards compatibility?

Please do answer this question: if you're *not* proposing to break the platform in a way that would prevent older binaries from working without modification, then we have no disagreement! :)

Don't get me wrong: I *am* in favour of improving the security of EDK2, and a formally verified loader is potentially useful for that. But I could only consider it a good idea if it can be done without making breaking API changes for which I know I will personally have to maintain workarounds for the next ten years.
Sorry, but you seem to have missed my points regarding these concerns, at least you did not address them I believe. I don't know why you need to (actively) maintain workarounds for APIs external code has no reason to use, especially when the legacy implementation would quite literally be a wrapper function.
<same comment as above>

Thanks,

Michael


Re: [GSoC proposal] Secure Image Loader

Michael Brown
 

On 08/04/2021 10:04, Marvin Häuser wrote:
Thank you a lot. One thing I noticed is that part of the quote I did not see on the list before, so I marked it below.
On 08.04.21 00:10, Andrew Fish wrote:
The general UEFI (and UEFI PI) is we mostly add new things, and don’t depreciated things to maintain compatibility. So for example you can add a new Protocol to a handle so you have V1 and V2 of a protocol on the same handle. An example of this is SimpleTextIn and SimpleTextInEx. SimpleTextIn was modeled on the LCD of a serial terminal (our server roots) so it did not expose modifier keys, or have an easy way to implement snag keys so that is why SimpleTextInEx got added for the new functional.
^ Here; +1
The addition of EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL was really not a good example of how to handle this issue gracefully.

Here is the kind of workaround that external code has to implement: it's a perfect example of how this "add a V2 protocol" approach ends up imposing a permanent maintenance burden on external code:

https://github.com/ipxe/ipxe/commit/a08244ecc

Note that there was absolutely no reason for the specification to require a V2 protocol in order to support Ctrl-<key>, and the EDK2 codebase will indeed do the sensible thing and return the ASCII values for Ctrl-<key> via the original EFI_SIMPLE_TEXT_INPUT_PROTOCOL. It would be amazing if, as you suggested, everyone uses the EDK2 codebase and so all public implementations of EFI_SIMPLE_TEXT_INPUT_PROTOCOL would do this sensible thing.

Unfortunately, this is not the case. Very large numbers of vendors use some other non-EDK2 implementation that does not do the sensible thing. I have no idea why.

It's also worth noting that the addition of EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL opened up a gaping potential security hole related to pointer lifetimes, as documented in the above-linked commit message.


TL;DR: please assume that creating a V2 protocol has a very significant cost, and needs to come with benefits that outweigh that very significant cost. If you can achieve what you need without breaking backwards compatibility, then that represents a massive increase in value.

Thanks,

Michael


Re: [GSoC proposal] Secure Image Loader

Michael Brown
 

On 08/04/2021 09:53, Marvin Häuser wrote:
On 07.04.21 23:50, Michael Brown wrote:
InstallMultipleProtocolInterfaces() is not a breaking change: the existence of InstallMultipleProtocolInterfaces() does not require any change to the way that InstallProtocolInterface() is implemented or consumed.

Code written before the invention of InstallMultipleProtocolInterfaces() will still work now, with no modifications required.
The same is true for the *2_PROTOCOL instances, I do not see how you distinct between them. Could you elaborate, please?
The distinction is very straightforward. If you plan to *remove* support for the older protocols, then you by definition place a burden on all externally maintained code to support both protocols. If the older protocol will continue to be supported then no such burden is created.

This is why I am asking you if your proposed changes require *breaking* backwards compatibility. You still haven't answered this key question.

You have to remember that UEFI is not a monolithic codebase with a single maintaining organisation.  Implementing a *2_PROTOCOL and deprecating the original just causes pain for all the code in the world that is maintained outside of the EDK2 repository, since that code now has to support *both* the old and new protocols.
I am aware, but actually it's not far from it nowadays. In contrast to the days of Aptio IV, I believe all big vendors maintain forks of EDK II. I know the fork nature taints the issue, but also see last quote comment.
This is empirically not true. Buy a selection of devices in the wild, and you'll find a huge amount of non-EDK2 code on them.

I would be extremely happy if every vendor just used the EDK2 code: it would make my life a lot easier. But it's not what happens in the real world.

I see that there is no EFI_USB_IO2_PROTOCOL instance to argue by. Yet there are EFI_BLOCK_IO2_PROTOCOL and EFI_LOAD_FILE2_PROTOCOL. Former, in my opinion, close in nature to your your example, and latter close to the nature on what I plan to propose. Sorry, but I do not see how what I suggest has not been done multiple times in the past already.
Please also look at it from an angle of trust. How can I trust the "secure" advertisements of UEFI / EDK II when the specification *dictates* the usage of intrinsically insecure APIs?
The consequence for security-critical situations would be to necessarily abandon UEFI and come up with a new design. In who's interest is this?
Again, we return to the question that you have not yet answered: do your proposed changes require breaking backwards compatibility?

Please do answer this question: if you're *not* proposing to break the platform in a way that would prevent older binaries from working without modification, then we have no disagreement! :)

Don't get me wrong: I *am* in favour of improving the security of EDK2, and a formally verified loader is potentially useful for that. But I could only consider it a good idea if it can be done without making breaking API changes for which I know I will personally have to maintain workarounds for the next ten years.
Sorry, but you seem to have missed my points regarding these concerns, at least you did not address them I believe. I don't know why you need to (actively) maintain workarounds for APIs external code has no reason to use, especially when the legacy implementation would quite literally be a wrapper function.
<same comment as above>

Thanks,

Michael

10261 - 10280 of 84035