Date   

[edk2-platforms][PATCH V3 3/6] Platform/Sgi: fix CPU acpi-id for RD-V1-MC platform

Pranav Madhu
 

Fix the incorrect ACPI _UID (Unique ID) object for CPU devices listed
for the RD-V1-MC platform.

Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
---
Platform/ARM/SgiPkg/AcpiTables/RdV1Mc/Madt.aslc | 24 ++++++++++---------=
-
1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdV1Mc/Madt.aslc b/Platform/A=
RM/SgiPkg/AcpiTables/RdV1Mc/Madt.aslc
index 0da56ed2a39b..9b3e4f2be370 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdV1Mc/Madt.aslc
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdV1Mc/Madt.aslc
@@ -69,55 +69,55 @@ STATIC EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE M=
adt =3D {
=20
// Chip 1
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core0
- 0, 0, GET_MPID(0x01000000ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 4, GET_MPID(0x01000000ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core1
- 0, 1, GET_MPID(0x01000100ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 5, GET_MPID(0x01000100ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core2
- 0, 2, GET_MPID(0x01000200ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 6, GET_MPID(0x01000200ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core3
- 0, 3, GET_MPID(0x01000300ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 7, GET_MPID(0x01000300ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
=20
// Chip 2
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core0
- 0, 0, GET_MPID(0x02000000ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 8, GET_MPID(0x02000000ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core1
- 0, 1, GET_MPID(0x02000100ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 9, GET_MPID(0x02000100ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core2
- 0, 2, GET_MPID(0x02000200ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 10, GET_MPID(0x02000200ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core3
- 0, 3, GET_MPID(0x02000300ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 11, GET_MPID(0x02000300ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
=20
// Chip 3
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core0
- 0, 0, GET_MPID(0x03000000ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 12, GET_MPID(0x03000000ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core1
- 0, 1, GET_MPID(0x03000100ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 13, GET_MPID(0x03000100ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core2
- 0, 2, GET_MPID(0x03000200ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 14, GET_MPID(0x03000200ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
EFI_ACPI_6_2_GICC_STRUCTURE_INIT( // Zeus core3
- 0, 3, GET_MPID(0x03000300ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
+ 0, 15, GET_MPID(0x03000300ULL, 0x0), EFI_ACPI_6_2_GIC_ENABLED, 23,
FixedPcdGet32 (PcdGicDistributorBase),
0x2c020000, 0x2c010000, 25, 0 /* GicRBase */, 0 /* Efficiency */),
},
--=20
2.17.1


[edk2-platforms][PATCH V3 2/6] Platform/Sgi: fix the list of CPU devices on RD-V1-MC platform

Pranav Madhu
 

RD-V1-MC platform has four CPUs in each of its four coherently connected
chips. So remove a incorrect CPU device entry in DSDT table that lists a
additional non-existent CPU.

Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
---
Platform/ARM/SgiPkg/AcpiTables/RdV1Mc/Dsdt.asl | 6 ------
1 file changed, 6 deletions(-)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdV1Mc/Dsdt.asl b/Platform/AR=
M/SgiPkg/AcpiTables/RdV1Mc/Dsdt.asl
index aef6473857b0..9bf57d05a646 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdV1Mc/Dsdt.asl
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdV1Mc/Dsdt.asl
@@ -109,11 +109,5 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 1, "ARMLTD=
", "ARMSGI",
Name (_UID, 15)
Name (_STA, 0xF)
}
-
- Device (CP16) { // Zeus core 16
- Name (_HID, "ACPI0007")
- Name (_UID, 16)
- Name (_STA, 0xF)
- }
} // Scope(_SB)
}
--=20
2.17.1


[edk2-platforms][PATCH V3 1/6] Platform/Sgi: include SSDT table for RD-V1 platform

Pranav Madhu
 

Ssdt ACPI table in SgiPkg describes the PCIe controller and the root
complex resources. Include this table for RD-V1 and RD-V1-MC platforms.

Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
---
Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf | 1 +
Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf | 1 +
2 files changed, 2 insertions(+)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf b/Platform=
/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf
index a5f936b8a758..583ffac70b71 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf
@@ -24,6 +24,7 @@
RdV1/Dsdt.asl
RdV1/Madt.aslc
Spcr.aslc
+ Ssdt.asl
=20
[Packages]
ArmPkg/ArmPkg.dec
diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf b/Platfo=
rm/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf
index 04edfc487738..d0d9473057a9 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf
@@ -26,6 +26,7 @@
RdV1Mc/Madt.aslc
RdV1Mc/Srat.aslc
Spcr.aslc
+ Ssdt.asl
=20
[Packages]
ArmPkg/ArmPkg.dec
--=20
2.17.1


[edk2-platforms][PATCH V3 0/6] Platform/Sgi: improvements in RD platform support

Pranav Madhu
 

Changes since V2:
- Rebase the patches on top of latest master branch
- Addressed comments from Sami

Changes since V1:
- Rebase the patches on top of latest master branch
- Add three more patches to the series
- Picked up Sami's R-b tags on applicable patches

This patch series contains assorted cleanups for the RD platforms. The
first patch in this series includes the SSDT table for both the RD-V1
single chip and multi-chip platform. The second and third patches in
this series cleanup the ACPI processor ID and MPIDR values for RD-V1 in
multichip configuration. The fourth patch in this series updates the
DSDT/SSDT table revision as per ACPI specification 6.3. The fifth patch
adds PCDs for timer interrupts as interrupt mapping is different RD-N2
platform. And the last patch in this series adds SMMU and timer entries
into memory description table to support Arm SystemReady SR SBSA tests.

Link to github branch with the patches in this series -
https://github.com/Pranav-Madhu/edk2-platforms/tree/topics/rd/rd-cleanup

Pranav Madhu (6):
Platform/Sgi: include SSDT table for RD-V1 platform
Platform/Sgi: fix the list of CPU devices on RD-V1-MC platform
Platform/Sgi: fix CPU acpi-id for RD-V1-MC platform
Platform/Sgi: update ACPI table revision
Platform/Sgi: define PCD for timer interrupt numbers
Platform/Sgi: add SMMU and timer entries to memory description table

Platform/ARM/SgiPkg/SgiPlatform.dec | 18 +++++++++++
Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc | 10 ++++++
Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc | 10 ++++++
Platform/ARM/SgiPkg/SgiPlatform.dsc.inc | 15 ++++++++-
.../SgiPkg/AcpiTables/RdE1EdgeAcpiTables.inf | 4 +++
.../SgiPkg/AcpiTables/RdN1EdgeAcpiTables.inf | 4 +++
.../AcpiTables/RdN1EdgeX2AcpiTables.inf | 4 +++
.../ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf | 4 +++
.../ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf | 5 +++
.../SgiPkg/AcpiTables/RdV1McAcpiTables.inf | 5 +++
.../SgiPkg/AcpiTables/Sgi575AcpiTables.inf | 4 +++
.../Library/PlatformLib/PlatformLib.inf | 11 +++++++
Platform/ARM/SgiPkg/Include/SgiPlatform.h | 4 ---
.../Library/PlatformLib/PlatformLibMem.c | 32 ++++++++++++++++---
Platform/ARM/SgiPkg/AcpiTables/Gtdt.aslc | 8 ++---
.../ARM/SgiPkg/AcpiTables/RdE1Edge/Dsdt.asl | 4 +--
.../ARM/SgiPkg/AcpiTables/RdN1Edge/Dsdt.asl | 4 +--
Platform/ARM/SgiPkg/AcpiTables/RdN2/Dsdt.asl | 4 +--
Platform/ARM/SgiPkg/AcpiTables/RdV1/Dsdt.asl | 4 +--
.../ARM/SgiPkg/AcpiTables/RdV1Mc/Dsdt.asl | 10 ++----
.../ARM/SgiPkg/AcpiTables/RdV1Mc/Madt.aslc | 24 +++++++-------
.../ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl | 4 +--
Platform/ARM/SgiPkg/AcpiTables/Ssdt.asl | 4 +--
Platform/ARM/SgiPkg/AcpiTables/SsdtRos.asl | 4 +--
24 files changed, 153 insertions(+), 47 deletions(-)

--=20
2.17.1


Re: [EXTERNAL] Re: [edk2-devel] [GSoC proposal] Secure Image Loader

Bret Barkelew
 

As always, we volunteer the UEFI Talkbox Discord for conversations of this nature. 😉

 

https://discord.gg/cuqjER3Juw

 

- Bret

 

From: Marvin Häuser via groups.io
Sent: Monday, April 12, 2021 10:24 AM
To: devel@edk2.groups.io; Desimone, Nathaniel L; Laszlo Ersek; Andrew Fish; Kinney, Michael D
Subject: [EXTERNAL] Re: [edk2-devel] [GSoC proposal] Secure Image Loader

 

Good day Nate,

As you seem to be mostly in charge of the GSoC side of things, I direct
this at you, but of course everyone is welcome to comment.

I think I finished my first round of investigations just in time for the
deadline. You can find a list of BZs attached[1]. Please note that 1)
some are declared security issues and may not be publicly accessible,
and 2) that this list is far from complete. I only added items that are
a) not implicitly fixed by "simply" deploying the new Image Loader
(*some* important prerequisites are listed), and b) I am confident are
actual issues (or things to consider) I believe to know how to approach.

I have taken notes about more things, e.g. the existence of the security
architectural protocols, which I could not find a rationale for. I can
prepare something for this matter, but it really needs an active
discussion with some of the core people. I'm not sure delayed e-mail
discussion is going to be enough, but there is an official IRC I
suppose. :) I hope we can work something out for this.

I also hope this makes it clearer why I don't believe that we need to
"fill" 10 weeks, but rather the opposite. This is not a matter of
replacing a library instance, but the whole surrounding ecosystem needs
to follow for the changes to make sense. And as I tried to make clear in
my discussion with Michael Brown, I am not keen on preserving
backwards-compatibility with platform code (i.e. PEI, DXE, things we
consider "internal"), as most of it should be controlled by EDK II
already. This of course does *not* include user code (OROMs,
bootloaders, ...), for which I want to provide the *option* to lock some
of them out for security, but with sane defaults that will ensure good
compatibility.

I'd like to thank Michael Brown for his cooperation and support, because
we recently landed changes in iPXE to allow for the strictest image
format and permission constraints currently possible[2].

I will have to rework the submitted proposal to reflect the new
knowledge. To be honest, seeing how the BZs kept rolling in, I am not
convinced an amazing amount of mainlining can be accomplished during the
10 weeks. It may have to suffice to have a publicly accessible prototype
(e.g. OVMF) and a subset of the planned patches on the list. I hope you
can manage to provide some feedback before the deadline passes tomorrow.

Thank you in advance!

Best regards,
Marvin

[1]
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3315&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446516854%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=xkAIi7yRQfRtw3pKELUzpb1oo9EN4kyroCdadjEzPLQ%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3316&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446516854%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=cFAacXfo69cDMpxSggXjnVpoRqYdIj21QYg%2Ffo5jp9Y%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3317&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446516854%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Ssf4h8yn3zee1IaK5%2BwI5WwvOvUW4gAtjikil0pS3Fw%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3318&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446516854%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=2KKVA6qqHIP2skLSLXo56av1%2FS9pL1MMJbt%2FPI9BBPM%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3319&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446516854%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ERrMCAigrcP0pkORLcNzFRPw74YxmHlZFohsvmpeXpE%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3320&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=4yKviWdeKCojn7cVTsHE5xQbflvX7TdnML07D67NdYc%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3321&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=KSt0vUz1VioMOEfcagubQjh0YjVIljpyxJHiKWVklXc%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3322&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=FFVnp5Np6xQHhKt8dpKGPlU9dV5BEMQBQ8qb1Sd6g5Q%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3323&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3PRwMITWczBZ5ioQC6MP%2BnLbywvU2hTMlEQA%2F5u9%2FlI%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=werYWbqTbEQfVc7C1ujWYNZlDbG%2BEykDppDi5TL8fg0%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3326&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=F9f%2FGvEvqBPpTWewflQ2OGlTSpE5CGl5yA%2B705X%2BV78%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3327&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eTbDLsN3UymQHBZqyzBkyBDHerHF1RbigQwy6r50PaE%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3328&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=DSHRyWvAoxTFN2xaLbonm1yh2eOAvf9GvLezG2kOQfs%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3329&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=DEvvfOQsVQiQIbHL919g0QKNU%2Fr2%2FXi64zfESo2EOn8%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3330&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=lIcbok5gDUg5kcmqUQvmQLKIkGHQhg05MPn07ayQAXk%3D&amp;reserved=0
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3331&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=b6mWPdfFR53ZXKkUWiWrwWGyRFllWe7gfpQz0zRy%2F9w%3D&amp;reserved=0

[2] https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fipxe%2Fipxe%2Fpull%2F313&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Kyz14TkQ1YUexm9Xm7eSQEXSaDCioZk9xSdmEZ17Qfs%3D&amp;reserved=0

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 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.
>
> 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? 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.
>
> 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.
>
> 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@...>; Andrew Fish
>> <afish@...>; Kinney, Michael D <michael.d.kinney@...>
>> 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmhaeuser%2FISPRASOpen-SecurePE&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=BlF8x5mleNL5mFSYGBmKT8aVIt1Jl0pjUlzTI%2Fu49AM%3D&amp;reserved=0
>> [2] https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Farxiv.org%2Fpdf%2F2012.05471.pdf&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7C045c38141f1e443d1e4b08d8fdd78e23%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637538450446526809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=flmu2cOW7z1A2c%2FpM3Hc1N3tvVLlx44kMKDw8U%2BAmC8%3D&amp;reserved=0
>>
>>
>>
>>
>
>
>
>
>





 


Re: [PATCH v2 1/1] SecurityPkg/Tcg2Smm: Initialize local Status variable

Michael Kubacki
 

Hi Laszlo and SecurityPkg maintainers,

This is a relatively straightforward patch. Please let me know if anything else is needed for you to submit it.

Thanks,
Michael

On 4/7/2021 9:06 AM, Laszlo Ersek wrote:
On 04/06/21 20:12, mikuback@linux.microsoft.com wrote:
From: Michael Kubacki <michael.kubacki@microsoft.com>

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

Initializes the Status variable in TcgMmReadyToLock().

Fixes a Clang build failure:
Tcg2Smm.c - SecurityPkg\Tcg\Tcg2Smm\Tcg2Smm.c:254:7: error:
variable 'Status' is used uninitialized whenever 'if'
condition is false [-Werror,-Wsometimes-uninitialized]

Initializing this variable is required to address a practical
scenario in which the return value of TcgMmReadyToLock() is
undefined based on conditional evaluation in the function.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Qi Zhang <qi1.zhang@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Kun Qin <kun.q@outlook.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
---

Notes:
V2 change:
Clarify in commit message that the issue reported by Clang is not
solely a false positive.

SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c b/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
index 589c08794bcf..f49eccb0bdf4 100644
--- a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
+++ b/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
@@ -253,6 +253,8 @@ TcgMmReadyToLock (
{
EFI_STATUS Status;
+ Status = EFI_SUCCESS;
+
if (mReadyToLockHandle != NULL) {
Status = gMmst->MmiHandlerUnRegister (mReadyToLockHandle);
mReadyToLockHandle = NULL;
Awesome, thanks!
I've also managed to look at the code now.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo


Re: [EXTERNAL] [edk2-devel] Build Failed for QEMU35Pkg

Pandya, Shivanshi <Shivanshi.Pandya@...>
 

Hey All,

 

Thank you for your thoughts and responses. I have figured it out.

The branch has been updated with fix which I missed! Got the successful build done.

 

Thank you,

Shivanshi

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Monday, April 12, 2021 11:32 AM
To: Andrew Fish; edk2-devel-groups-io
Cc: Pandya, Shivanshi
Subject: RE: [EXTERNAL] [edk2-devel] Build Failed for QEMU35Pkg

 

[EXTERNAL EMAIL]

To my knowledge, we do not currently have a required version; the current state is it either works or it doesn’t, and if it doesn’t it’s on you to figure that out from the failures.

 

I’m not opposed to adding one – if there’s interest. I’d have to think about the best place to add it. The idea of making it its own test is interesting. It wouldn’t prevent you from getting other test data, but it would let you know you aren’t on the ideal version and there may be unpredictable results. Ponder ponder…

 

- Bret

 

From: Andrew Fish
Sent: Monday, April 12, 2021 9:27 AM
To: edk2-devel-groups-io; Bret Barkelew
Cc: Shivanshi.Pandya@...
Subject: Re: [EXTERNAL] [edk2-devel] Build Failed for QEMU35Pkg

 

 

 

On Apr 9, 2021, at 6:33 PM, Bret Barkelew via groups.io [nam06.safelinks.protection.outlook.com] <bret.barkelew@...> wrote:

 

Andrew,

Not a scheme that I would consider a “good” scheme. You can see what we’re running CI against (in Mu and EDK, both), by checking for the  “UsePythonVersion” command in the .azurepipelines/pr-gate-steps.yml file.

 

 

Bret,

 

If I understand correctly the CI list is the recommended versions, but not the required versions. Do we have any concept of the required versions? I guess we could put a python assert to enforce min Python version? I guess we could be more aggressive on the required Python version (forces people to install a custom Python version for the edk2 build), or build a CI test that tests the min Python version for the tools...

 

Sorry mostly thinking out loud…. 

 

Thanks,

 

Andrew Fish

 

 

Shivanshi,

I just ran a build on my system with that exact version of Python (3.9.0). Did your build produce a “BUILD_TOOLS_REPORT.json” file? If so, can you send it?

Can you also send the exact command that you’re running when you see this issue?

 

Thanks!

 

- Bret 

 

From: Andrew Fish
Sent: Friday, April 9, 2021 4:54 PM
To: edk2-devel-groups-io; Bret Barkelew
Cc: Shivanshi.Pandya@...
Subject: [EXTERNAL] Re: [edk2-devel] Build Failed for QEMU35Pkg

 

 



On Apr 9, 2021, at 1:55 PM, Bret Barkelew via groups.io [nam06.safelinks.protection.outlook.com] <bret.barkelew@...> wrote:

 

It looks like a Python 3.8.x vs 3.9.x issue.

 

 

Do we have a scheme to require a min Python version?

 

Thanks,

 

Andrew Fish



It looks as if you’re using Mu Q35 as your platform. Can you tell me what branch you’re on?

 

- Bret 

 

From: Pandya, Shivanshi via groups.io
Sent: Friday, April 9, 2021 1:52 PM
To: devel@edk2.groups.io
Subject: [EXTERNAL] [edk2-devel] Build Failed for QEMU35Pkg

 

Hello,

 

Build failed with following call trace

 

build.py...

INFO -  : error C0DE: Unknown fatal error when processing [c:\bea\dfci\mu_tiano_platforms\Common\PRM\PrmPkg\Library\DxePrmModuleDiscoveryLib\DxePrmModuleDiscoveryLib.inf [X64, VS2017, DEBUG]]

INFO -

INFO - (Please send email to devel@edk2.groups.io for help, attaching following call stack trace!)

INFO -

INFO - (Python 3.9.0 on win32) Traceback (most recent call last):

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2635, in Main

INFO -     MyBuild.Launch()

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2430, in Launch

INFO -     self._MultiThreadBuildPlatform()

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2238, in _MultiThreadBuildPlatform

INFO -     Wa, self.BuildModules = self.PerformAutoGen(BuildTarget,ToolChain)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2112, in PerformAutoGen

INFO -     CmdListDict = self._GenFfsCmd(Wa.ArchList)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2009, in _GenFfsCmd

INFO -     GenFfsDict = GenFds.GenFfsMakefile('', GlobalData.gFdfParser, self, ArchList, GlobalData)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\GenFds.py", line 541, in GenFfsMakefile

INFO -     FdObj.GenFd(Flag=True)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\Fd.py", line 131, in GenFd

INFO -     RegionObj.AddToBuffer (FdBuffer, self.BaseAddress, self.BlockSizeList, self.ErasePolarity, GenFdsGlobalVariable.ImageBinDict, self.DefineVarDict, Flag=Flag)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\Region.py", line 134, in AddToBuffer

INFO -     FvObj.AddToBuffer(FvBuffer, FvBaseAddress, BlockSize, BlockNum, ErasePolarity, Flag=Flag)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\Fv.py", line 127, in AddToBuffer

INFO -     FileName = FfsFile.GenFfs(MacroDict, FvParentAddr=BaseAddress, IsMakefile=Flag, FvName=self.UiFvName)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\FfsInfStatement.py", line 518, in GenFfs

INFO -     InputSectList, InputSectAlignments = self.__GenComplexFileSection__(Rule, FvChildAddr, FvParentAddr, IsMakefile=IsMakefile)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\FfsInfStatement.py", line 969, in __GenComplexFileSection__

INFO -     SectList, Align = Sect.GenSection(self.OutputPath, self.ModuleGuid, SecIndex, self.KeyStringList, self, IsMakefile = IsMakefile)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\EfiSection.py", line 218, in GenSection

INFO -     GenFdsGlobalVariable.GenerateSection(OutputFile, [], 'EFI_SECTION_USER_INTERFACE',

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\GenFdsGlobalVariable.py", line 466, in GenerateSection

INFO -     SectionData.fromstring(Ui.encode("utf_16_le"))

INFO - AttributeError: 'array.array' object has no attribute 'fromstring'

 

Kind Regards,

Shivanshi

 

<79C90400E51C4EC6A197393CD98D0F7A.png>

 

 

<CC6C706EF927441BBD49E964355D5AA6.png>

 

 


Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing

Kun Qin
 

Hi Laszlo,

Thanks for the help.

Regards,
Kun

On 04/12/2021 10:36, Laszlo Ersek wrote:
On 04/07/21 18:08, Laszlo Ersek wrote:
On 04/06/21 21:52, Kun Qin wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3283

Current SMM Save State routine does not check the number of bytes to be
read, when it comse to read IO_INFO, before casting the incoming buffer
to EFI_SMM_SAVE_STATE_IO_INFO. This could potentially cause memory
corruption due to extra bytes are written out of buffer boundary.

This change adds a width check before copying IoInfo into output buffer.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
v2:
- Update return code description [Laszlo]

UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 9 ++++++++-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 2 +-
2 files changed, 9 insertions(+), 2 deletions(-)
Thanks, looks OK. I'll let Ray or Eric merge the patch.
:/
Merged as commit a7d8e28b29f2, via
<https://github.com/tianocore/edk2/pull/1554>.
Laszlo


Re: [PATCH v1 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Check buffer size before accessing

Laszlo Ersek
 

On 04/07/21 18:08, Laszlo Ersek wrote:
On 04/06/21 21:52, Kun Qin wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3283

Current SMM Save State routine does not check the number of bytes to be
read, when it comse to read IO_INFO, before casting the incoming buffer
to EFI_SMM_SAVE_STATE_IO_INFO. This could potentially cause memory
corruption due to extra bytes are written out of buffer boundary.

This change adds a width check before copying IoInfo into output buffer.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
v2:
- Update return code description [Laszlo]

UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 9 ++++++++-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 2 +-
2 files changed, 9 insertions(+), 2 deletions(-)
Thanks, looks OK. I'll let Ray or Eric merge the patch.
:/

Merged as commit a7d8e28b29f2, via
<https://github.com/tianocore/edk2/pull/1554>.

Laszlo


Re: [PATCH 1/1] Platform/RaspberryPi: Setup option for disabling Fast Boot

Ard Biesheuvel
 

On Mon, 12 Apr 2021 at 15:03, Pete Batard <pete@akeo.ie> wrote:

Hi Sunny,

Functionaly, I see no issues with this patch, and it's indeed a good
solution to the problem of trying to satisfy everyone while fixing the
issues we are seeing.

There are however a few minor typos and whitespace issues, that I'm
detailing below. So could you please send a v2 to address these?
Agree with Pete. Thanks for providing a fix for this, and please
incorporate the changes suggested by Pete.

Then, could we *please* figure out a way to get this patch on the
mailing list without Windows whitespace damage? Not sure if anything
changed recently, but not a single EDK2 patch has arrived in my
mailbox recently in a form that I could apply with 'git am'.

Thanks,
Ard.


On 2021.04.12 10:05, Sunny Wang wrote:
This is a fix for https://github.com/pftf/RPi4/issues/114.

Changes:
1. Add a setup option called Boot Policy and consume the setting
during boot to whether perform or skip ConnectAll.
2. The Default setting is set to Full discovery because it is not
worth enabling Fast boot by default on RaspberryPi systems.
Enabling it just saves boot time about 1 second, but caused a
lot of issues.

Testing Done:
- Booted to Standalone UEFI shell on SD card and use drivers
command to check the result with Fast Boot and Full discovery
settings. Then, child/device handles are created as expected.

Note and to-do items:
- The root cause looks like that boot loaders and some tools like
grub and iPXE haven't supported selective connect/Fast boot.
However, system firmware should still provide a setup option for
user to enable Fast boot with old version boot loaders and tools
, which is why we proposed this change. We will also report this
issue to boot loader and tool vendors/open source GitHubs.
- We Will add more options for connecting specific type devices so
that we can still have the shortest boot time for all use cases.

Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
Cc: Jeremy Linton <jeremy.linton@arm.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Signed-off-by: Sunny Wang <sunny.wang@arm.com>
---
.../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 11 ++++++++++-
.../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf | 3 ++-
.../Drivers/ConfigDxe/ConfigDxeHii.uni | 10 +++++++++-
.../Drivers/ConfigDxe/ConfigDxeHii.vfr | 15 ++++++++++++++-
Platform/RaspberryPi/Include/ConfigVars.h | 12 +++++++++++-
.../Library/PlatformBootManagerLib/PlatformBm.c | 16 ++++++++++++++--
.../PlatformBootManagerLib.inf | 1 +
Platform/RaspberryPi/RPi3/RPi3.dsc | 10 +++++++++-
Platform/RaspberryPi/RPi4/RPi4.dsc | 9 ++++++++-
Platform/RaspberryPi/RaspberryPi.dec | 2 ++
10 files changed, 80 insertions(+), 9 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index 22f86d4d44..d3c5869949 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -1,6 +1,6 @@
/** @file

*

- * Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.

+ * Copyright (c) 2019 - 2021, ARM Limited. All rights reserved.

* Copyright (c) 2018 - 2020, Andrei Warkentin <andrey.warkentin@gmail.com>

*

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

@@ -281,6 +281,15 @@ SetupVariables (
);

}



+ Size = sizeof (UINT32);

+ Status = gRT->GetVariable (L"BootPolicy",

+ &gConfigDxeFormSetGuid,

+ NULL, &Size, &Var32);

+ if (EFI_ERROR (Status)) {

+ Status = PcdSet32S (PcdBootPolicy, PcdGet32 (PcdBootPolicy));

+ ASSERT_EFI_ERROR (Status);

+ }

+

Size = sizeof (UINT32);

Status = gRT->GetVariable (L"SdIsArasan",

&gConfigDxeFormSetGuid,

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
index d51e54e010..032e40b0c3 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
@@ -2,7 +2,7 @@
#

# Component description file for the RasbperryPi DXE platform config driver.

#

-# Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.

+# Copyright (c) 2019 - 2021, ARM Limited. All rights reserved.

# Copyright (c) 2018 - 2020, Andrei Warkentin <andrey.warkentin@gmail.com>

#

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

@@ -93,6 +93,7 @@
gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB

gRaspberryPiTokenSpaceGuid.PcdFanOnGpio

gRaspberryPiTokenSpaceGuid.PcdFanTemp

+ gRaspberryPiTokenSpaceGuid.PcdBootPolicy



[Depex]

gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
index 466fa852cb..7b14fdf39f 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
@@ -1,7 +1,7 @@
/** @file

*

* Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>

- * Copyright (c) 2020, ARM Limited. All rights reserved.

+ * Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.

*

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

*

@@ -60,6 +60,14 @@
#string STR_ADVANCED_ASSET_TAG_PROMPT #language en-US "Asset Tag"

#string STR_ADVANCED_ASSET_TAG_HELP #language en-US "Set the system Asset Tag"

Personally, I would move the Boot Policy section between "System Table
Selection" and "ACPI Fan Control" if we try to keep the advanced
settings in the order we think they will be most used.


+#string STR_BOOT_POLICY_PROMPT #language en-US "Boot Policy"

+#string STR_BOOT_POLICY_HELP #language en-US "When Fast Boot is selected, only required devices will be discoverd for reducing "
"discoverd" -> "discovered"


+ "the boot time."
Missing a space after "the boot time.". It should be "the boot time. ",
else the help text will display as "(...) the boot time.When Full
Discovery (...)"


+ "When Full Discovery is selected, all the devices will be discoverd for some "
"discoverd" -> "discovered"


+ "scenarios such as system deployement and diagnostic tests"
"deployement" -> "deployment"


+#string STR_FAST_BOOT #language en-US "Fast Boot"

+#string STR_FULL_DISCOVERY #language en-US "Full Discovery"

+

/*

* MMC/SD configuration.

*/

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
index cc7a09cfb7..5dc558ec08 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
@@ -1,7 +1,7 @@
/** @file

*

* Copyright (c) 2018 Andrei Warkentin <andrey.warkentin@gmail.com>

- * Copyright (c) 2020, ARM Limited. All rights reserved.

+ * Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.

*

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

*

@@ -116,6 +116,11 @@ formset
name = DisplayEnableSShot,

guid = CONFIGDXE_FORM_SET_GUID;



+ efivarstore BOOT_POLICY_VARSTORE_DATA,

+ attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,

+ name = BootPolicy,

+ guid = CONFIGDXE_FORM_SET_GUID;

+

form formid = 1,

title = STRING_TOKEN(STR_FORM_SET_TITLE);

subtitle text = STRING_TOKEN(STR_NULL_STRING);

@@ -220,6 +225,14 @@ formset
minsize = 0,

maxsize = ASSET_TAG_STR_MAX_LEN,

endstring;

+

+ oneof varid = BootPolicy.BootPolicy,

+ prompt = STRING_TOKEN(STR_BOOT_POLICY_PROMPT),

+ help = STRING_TOKEN(STR_BOOT_POLICY_HELP),

+ flags = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,

+ option text = STRING_TOKEN(STR_FAST_BOOT ), value = FAST_BOOT , flags = 0;
"STRING_TOKEN(STR_FAST_BOOT )" -> "STRING_TOKEN(STR_FAST_BOOT)"


+ option text = STRING_TOKEN(STR_FULL_DISCOVERY), value = FULL_DISCOVERY, flags = DEFAULT;

+ endoneof;

endform;



form formid = 0x1003,

diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
index 142317985a..3347f899df 100644
--- a/Platform/RaspberryPi/Include/ConfigVars.h
+++ b/Platform/RaspberryPi/Include/ConfigVars.h
@@ -1,7 +1,7 @@
/** @file

*

* Copyright (c) 2020, Andrei Warkentin <andrey.warkentin@gmail.com>

- * Copyright (c) 2020, ARM Limited. All rights reserved.

+ * Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.

*

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

*

@@ -143,4 +143,14 @@ typedef struct {
UINT32 EnableDma;

} MMC_EMMC_DMA_VARSTORE_DATA;



+#define FAST_BOOT 0

+#define FULL_DISCOVERY 1

+typedef struct {

+ /*

+ * 0 - Fast Boot

+ * 1 - Full Discovrey (Connect All)
"Discovrey" -> "Discovery"


+ */

+ UINT32 BootPolicy;

+} BOOT_POLICY_VARSTORE_DATA;

+

#endif /* CONFIG_VARS_H */

diff --git a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
index c2fc40b8ea..abbe4fb3d0 100644
--- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
@@ -4,7 +4,7 @@
* Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>

* Copyright (c) 2016, Linaro Ltd. All rights reserved.

* Copyright (c) 2015-2016, Red Hat, Inc.

- * Copyright (c) 2014-2020, ARM Ltd. All rights reserved.

+ * Copyright (c) 2014-2021, ARM Ltd. All rights reserved.

* Copyright (c) 2004-2016, Intel Corporation. All rights reserved.

*

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

@@ -26,9 +26,11 @@
#include <Guid/EventGroup.h>

#include <Guid/TtyTerm.h>



+#include <ConfigVars.h>
I'd remove the extra line between <Guid/TtyTerm.h> and <ConfigVars.h>


+

#include "PlatformBm.h"



-#define BOOT_PROMPT L"ESC (setup), F1 (shell), ENTER (boot)"

+#define BOOT_PROMPT L"ESC (setup), F1 (shell), ENTER (boot)\n"
Why are you adding the carriage return? Was there an issue with the
display of this prompt?




#define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }



@@ -633,6 +635,16 @@ PlatformBootManagerAfterConsole (
Print (BOOT_PROMPT);

}



+ //

+ // Connect the rest of the devices if the boot polcy is set to Full discovery

+ //

+ if (PcdGet32 (PcdBootPolicy) == FULL_DISCOVERY) {

+ DEBUG ((DEBUG_INFO, "Boot Policy is Full Discovery. Connect All devices\n"));
"All" -> "all"


+ EfiBootManagerConnectAll ();

+ } else if (PcdGet32 (PcdBootPolicy) == FAST_BOOT) {

+ DEBUG ((DEBUG_INFO, "Boot Policy is Fast Boot. Skip connecting all devices\n"));

+ }

+

Status = gBS->LocateProtocol (&gEsrtManagementProtocolGuid, NULL, (VOID**)&EsrtManagement);

if (!EFI_ERROR (Status)) {

EsrtManagement->SyncEsrtFmp ();

diff --git a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 88f6f8fe09..fbf510ab96 100644
--- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -63,6 +63,7 @@
[Pcd]

gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut

gRaspberryPiTokenSpaceGuid.PcdSdIsArasan

+ gRaspberryPiTokenSpaceGuid.PcdBootPolicy



[Guids]

gEfiFileInfoGuid

diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index 0961133ae9..ddb03e405f 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -1,6 +1,6 @@
# @file

#

-# Copyright (c) 2011 - 2020, ARM Limited. All rights reserved.

+# Copyright (c) 2011 - 2021, ARM Limited. All rights reserved.

# Copyright (c) 2014, Linaro Limited. All rights reserved.

# Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.

# Copyright (c) 2017 - 2018, Andrei Warkentin <andrey.warkentin@gmail.com>

@@ -512,6 +512,14 @@
gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0

gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|0



+ #

+ # Boot Policy

+ # 0 - Fast Boot

+ # 1 - Full Discovrey (Connect All)
"Discovrey" -> "Discovery"


+ #

+ gRaspberryPiTokenSpaceGuid.PcdBootPolicy|L"BootPolicy"|gConfigDxeFormSetGuid|0x0|1

+

+
Please remove the extra line. There should be just one blank like
between the sections.


#

# Reset-related.

#

diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index ff802d8347..8ee1922a44 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -1,6 +1,6 @@
# @file

#

-# Copyright (c) 2011 - 2020, ARM Limited. All rights reserved.

+# Copyright (c) 2011 - 2021, ARM Limited. All rights reserved.

# Copyright (c) 2017 - 2018, Andrei Warkentin <andrey.warkentin@gmail.com>

# Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.

# Copyright (c) 2014, Linaro Limited. All rights reserved.

@@ -528,6 +528,13 @@
gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0

gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|60



+ #

+ # Boot Policy

+ # 0 - Fast Boot

+ # 1 - Full Discovrey (Connect All)
"Discovrey" -> "Discovery"

Regards,

/Pete


+ #

+ gRaspberryPiTokenSpaceGuid.PcdBootPolicy|L"BootPolicy"|gConfigDxeFormSetGuid|0x0|1

+

#

# Reset-related.

#

diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
index 08135717ed..8eb1c2bac7 100644
--- a/Platform/RaspberryPi/RaspberryPi.dec
+++ b/Platform/RaspberryPi/RaspberryPi.dec
@@ -2,6 +2,7 @@
#

# Copyright (c) 2016, Linaro, Ltd. All rights reserved.

# Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>

+# Copyright (c) 2021, ARM Limited. All rights reserved.

#

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

#

@@ -70,3 +71,4 @@
gRaspberryPiTokenSpaceGuid.PcdFanTemp|0|UINT32|0x0000001D

gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|0|UINT32|0x0000001E

gRaspberryPiTokenSpaceGuid.PcdMmcEnableDma|0|UINT32|0x0000001F

+ gRaspberryPiTokenSpaceGuid.PcdBootPolicy|0|UINT32|0x00000020


Re: [GSoC proposal] Secure Image Loader

Marvin Häuser
 

Good day Nate,

As you seem to be mostly in charge of the GSoC side of things, I direct this at you, but of course everyone is welcome to comment.

I think I finished my first round of investigations just in time for the deadline. You can find a list of BZs attached[1]. Please note that 1) some are declared security issues and may not be publicly accessible, and 2) that this list is far from complete. I only added items that are a) not implicitly fixed by "simply" deploying the new Image Loader (*some* important prerequisites are listed), and b) I am confident are actual issues (or things to consider) I believe to know how to approach.

I have taken notes about more things, e.g. the existence of the security architectural protocols, which I could not find a rationale for. I can prepare something for this matter, but it really needs an active discussion with some of the core people. I'm not sure delayed e-mail discussion is going to be enough, but there is an official IRC I suppose. :) I hope we can work something out for this.

I also hope this makes it clearer why I don't believe that we need to "fill" 10 weeks, but rather the opposite. This is not a matter of replacing a library instance, but the whole surrounding ecosystem needs to follow for the changes to make sense. And as I tried to make clear in my discussion with Michael Brown, I am not keen on preserving backwards-compatibility with platform code (i.e. PEI, DXE, things we consider "internal"), as most of it should be controlled by EDK II already. This of course does *not* include user code (OROMs, bootloaders, ...), for which I want to provide the *option* to lock some of them out for security, but with sane defaults that will ensure good compatibility.

I'd like to thank Michael Brown for his cooperation and support, because we recently landed changes in iPXE to allow for the strictest image format and permission constraints currently possible[2].

I will have to rework the submitted proposal to reflect the new knowledge. To be honest, seeing how the BZs kept rolling in, I am not convinced an amazing amount of mainlining can be accomplished during the 10 weeks. It may have to suffice to have a publicly accessible prototype (e.g. OVMF) and a subset of the planned patches on the list. I hope you can manage to provide some feedback before the deadline passes tomorrow.

Thank you in advance!

Best regards,
Marvin

[1]
https://bugzilla.tianocore.org/show_bug.cgi?id=3315
https://bugzilla.tianocore.org/show_bug.cgi?id=3316
https://bugzilla.tianocore.org/show_bug.cgi?id=3317
https://bugzilla.tianocore.org/show_bug.cgi?id=3318
https://bugzilla.tianocore.org/show_bug.cgi?id=3319
https://bugzilla.tianocore.org/show_bug.cgi?id=3320
https://bugzilla.tianocore.org/show_bug.cgi?id=3321
https://bugzilla.tianocore.org/show_bug.cgi?id=3322
https://bugzilla.tianocore.org/show_bug.cgi?id=3323
https://bugzilla.tianocore.org/show_bug.cgi?id=3324
https://bugzilla.tianocore.org/show_bug.cgi?id=3326
https://bugzilla.tianocore.org/show_bug.cgi?id=3327
https://bugzilla.tianocore.org/show_bug.cgi?id=3328
https://bugzilla.tianocore.org/show_bug.cgi?id=3329
https://bugzilla.tianocore.org/show_bug.cgi?id=3330
https://bugzilla.tianocore.org/show_bug.cgi?id=3331

[2] https://github.com/ipxe/ipxe/pull/313

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 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.

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? 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.

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.

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: [EXTERNAL] [edk2-devel] Build Failed for QEMU35Pkg

Bret Barkelew
 

To my knowledge, we do not currently have a required version; the current state is it either works or it doesn’t, and if it doesn’t it’s on you to figure that out from the failures.

 

I’m not opposed to adding one – if there’s interest. I’d have to think about the best place to add it. The idea of making it its own test is interesting. It wouldn’t prevent you from getting other test data, but it would let you know you aren’t on the ideal version and there may be unpredictable results. Ponder ponder…

 

- Bret

 

From: Andrew Fish
Sent: Monday, April 12, 2021 9:27 AM
To: edk2-devel-groups-io; Bret Barkelew
Cc: Shivanshi.Pandya@...
Subject: Re: [EXTERNAL] [edk2-devel] Build Failed for QEMU35Pkg

 

 



On Apr 9, 2021, at 6:33 PM, Bret Barkelew via groups.io <bret.barkelew@...> wrote:

 

Andrew,

Not a scheme that I would consider a “good” scheme. You can see what we’re running CI against (in Mu and EDK, both), by checking for the  “UsePythonVersion” command in the .azurepipelines/pr-gate-steps.yml file.

 

 

Bret,

 

If I understand correctly the CI list is the recommended versions, but not the required versions. Do we have any concept of the required versions? I guess we could put a python assert to enforce min Python version? I guess we could be more aggressive on the required Python version (forces people to install a custom Python version for the edk2 build), or build a CI test that tests the min Python version for the tools...

 

Sorry mostly thinking out loud…. 

 

Thanks,

 

Andrew Fish

 



Shivanshi,

I just ran a build on my system with that exact version of Python (3.9.0). Did your build produce a “BUILD_TOOLS_REPORT.json” file? If so, can you send it?

Can you also send the exact command that you’re running when you see this issue?

 

Thanks!

 

- Bret 

 

From: Andrew Fish
Sent: Friday, April 9, 2021 4:54 PM
To: edk2-devel-groups-io; Bret Barkelew
Cc: Shivanshi.Pandya@...
Subject: [EXTERNAL] Re: [edk2-devel] Build Failed for QEMU35Pkg

 

 




On Apr 9, 2021, at 1:55 PM, Bret Barkelew via groups.io <bret.barkelew@...> wrote:

 

It looks like a Python 3.8.x vs 3.9.x issue.

 

 

Do we have a scheme to require a min Python version?

 

Thanks,

 

Andrew Fish




It looks as if you’re using Mu Q35 as your platform. Can you tell me what branch you’re on?

 

- Bret 

 

From: Pandya, Shivanshi via groups.io
Sent: Friday, April 9, 2021 1:52 PM
To: devel@edk2.groups.io
Subject: [EXTERNAL] [edk2-devel] Build Failed for QEMU35Pkg

 

Hello,

 

Build failed with following call trace

 

build.py...

INFO -  : error C0DE: Unknown fatal error when processing [c:\bea\dfci\mu_tiano_platforms\Common\PRM\PrmPkg\Library\DxePrmModuleDiscoveryLib\DxePrmModuleDiscoveryLib.inf [X64, VS2017, DEBUG]]

INFO -

INFO - (Please send email to devel@edk2.groups.io for help, attaching following call stack trace!)

INFO -

INFO - (Python 3.9.0 on win32) Traceback (most recent call last):

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2635, in Main

INFO -     MyBuild.Launch()

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2430, in Launch

INFO -     self._MultiThreadBuildPlatform()

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2238, in _MultiThreadBuildPlatform

INFO -     Wa, self.BuildModules = self.PerformAutoGen(BuildTarget,ToolChain)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2112, in PerformAutoGen

INFO -     CmdListDict = self._GenFfsCmd(Wa.ArchList)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2009, in _GenFfsCmd

INFO -     GenFfsDict = GenFds.GenFfsMakefile('', GlobalData.gFdfParser, self, ArchList, GlobalData)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\GenFds.py", line 541, in GenFfsMakefile

INFO -     FdObj.GenFd(Flag=True)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\Fd.py", line 131, in GenFd

INFO -     RegionObj.AddToBuffer (FdBuffer, self.BaseAddress, self.BlockSizeList, self.ErasePolarity, GenFdsGlobalVariable.ImageBinDict, self.DefineVarDict, Flag=Flag)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\Region.py", line 134, in AddToBuffer

INFO -     FvObj.AddToBuffer(FvBuffer, FvBaseAddress, BlockSize, BlockNum, ErasePolarity, Flag=Flag)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\Fv.py", line 127, in AddToBuffer

INFO -     FileName = FfsFile.GenFfs(MacroDict, FvParentAddr=BaseAddress, IsMakefile=Flag, FvName=self.UiFvName)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\FfsInfStatement.py", line 518, in GenFfs

INFO -     InputSectList, InputSectAlignments = self.__GenComplexFileSection__(Rule, FvChildAddr, FvParentAddr, IsMakefile=IsMakefile)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\FfsInfStatement.py", line 969, in __GenComplexFileSection__

INFO -     SectList, Align = Sect.GenSection(self.OutputPath, self.ModuleGuid, SecIndex, self.KeyStringList, self, IsMakefile = IsMakefile)

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\EfiSection.py", line 218, in GenSection

INFO -     GenFdsGlobalVariable.GenerateSection(OutputFile, [], 'EFI_SECTION_USER_INTERFACE',

INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\GenFdsGlobalVariable.py", line 466, in GenerateSection

INFO -     SectionData.fromstring(Ui.encode("utf_16_le"))

INFO - AttributeError: 'array.array' object has no attribute 'fromstring'

 

Kind Regards,

Shivanshi

 

<79C90400E51C4EC6A197393CD98D0F7A.png>

 

 

<CC6C706EF927441BBD49E964355D5AA6.png>

 

 


Re: [EXTERNAL] [edk2-devel] Build Failed for QEMU35Pkg

Andrew Fish
 



On Apr 9, 2021, at 6:33 PM, Bret Barkelew via groups.io <bret.barkelew@...> wrote:

Andrew,
Not a scheme that I would consider a “good” scheme. You can see what we’re running CI against (in Mu and EDK, both), by checking for the  “UsePythonVersion” command in the .azurepipelines/pr-gate-steps.yml file.
 

Bret,

If I understand correctly the CI list is the recommended versions, but not the required versions. Do we have any concept of the required versions? I guess we could put a python assert to enforce min Python version? I guess we could be more aggressive on the required Python version (forces people to install a custom Python version for the edk2 build), or build a CI test that tests the min Python version for the tools...

Sorry mostly thinking out loud…. 

Thanks,

Andrew Fish


Shivanshi,
I just ran a build on my system with that exact version of Python (3.9.0). Did your build produce a “BUILD_TOOLS_REPORT.json” file? If so, can you send it?
Can you also send the exact command that you’re running when you see this issue?
 
Thanks!
 
- Bret 
 
From: Andrew Fish
Sent: Friday, April 9, 2021 4:54 PM
To: edk2-devel-groups-io; Bret Barkelew
Cc: Shivanshi.Pandya@...
Subject: [EXTERNAL] Re: [edk2-devel] Build Failed for QEMU35Pkg
 
 


On Apr 9, 2021, at 1:55 PM, Bret Barkelew via groups.io <bret.barkelew@...> wrote:
 
It looks like a Python 3.8.x vs 3.9.x issue.
 
 
Do we have a scheme to require a min Python version?
 
Thanks,
 
Andrew Fish


It looks as if you’re using Mu Q35 as your platform. Can you tell me what branch you’re on?
 
- Bret 
 
From: Pandya, Shivanshi via groups.io
Sent: Friday, April 9, 2021 1:52 PM
To: devel@edk2.groups.io
Subject: [EXTERNAL] [edk2-devel] Build Failed for QEMU35Pkg
 
Hello,
 
Build failed with following call trace
 
build.py...
INFO -  : error C0DE: Unknown fatal error when processing [c:\bea\dfci\mu_tiano_platforms\Common\PRM\PrmPkg\Library\DxePrmModuleDiscoveryLib\DxePrmModuleDiscoveryLib.inf [X64, VS2017, DEBUG]]
INFO -
INFO - (Please send email to devel@edk2.groups.io for help, attaching following call stack trace!)
INFO -
INFO - (Python 3.9.0 on win32) Traceback (most recent call last):
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2635, in Main
INFO -     MyBuild.Launch()
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2430, in Launch
INFO -     self._MultiThreadBuildPlatform()
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2238, in _MultiThreadBuildPlatform
INFO -     Wa, self.BuildModules = self.PerformAutoGen(BuildTarget,ToolChain)
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2112, in PerformAutoGen
INFO -     CmdListDict = self._GenFfsCmd(Wa.ArchList)
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\build\build.py", line 2009, in _GenFfsCmd
INFO -     GenFfsDict = GenFds.GenFfsMakefile('', GlobalData.gFdfParser, self, ArchList, GlobalData)
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\GenFds.py", line 541, in GenFfsMakefile
INFO -     FdObj.GenFd(Flag=True)
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\Fd.py", line 131, in GenFd
INFO -     RegionObj.AddToBuffer (FdBuffer, self.BaseAddress, self.BlockSizeList, self.ErasePolarity, GenFdsGlobalVariable.ImageBinDict, self.DefineVarDict, Flag=Flag)
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\Region.py", line 134, in AddToBuffer
INFO -     FvObj.AddToBuffer(FvBuffer, FvBaseAddress, BlockSize, BlockNum, ErasePolarity, Flag=Flag)
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\Fv.py", line 127, in AddToBuffer
INFO -     FileName = FfsFile.GenFfs(MacroDict, FvParentAddr=BaseAddress, IsMakefile=Flag, FvName=self.UiFvName)
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\FfsInfStatement.py", line 518, in GenFfs
INFO -     InputSectList, InputSectAlignments = self.__GenComplexFileSection__(Rule, FvChildAddr, FvParentAddr, IsMakefile=IsMakefile)
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\FfsInfStatement.py", line 969, in __GenComplexFileSection__
INFO -     SectList, Align = Sect.GenSection(self.OutputPath, self.ModuleGuid, SecIndex, self.KeyStringList, self, IsMakefile = IsMakefile)
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\EfiSection.py", line 218, in GenSection
INFO -     GenFdsGlobalVariable.GenerateSection(OutputFile, [], 'EFI_SECTION_USER_INTERFACE',
INFO -   File "C:\BEA\DFCI\mu_tiano_platforms\MU_BASECORE\BaseTools\Source\Python\GenFds\GenFdsGlobalVariable.py", line 466, in GenerateSection
INFO -     SectionData.fromstring(Ui.encode("utf_16_le"))
INFO - AttributeError: 'array.array' object has no attribute 'fromstring'
 
Kind Regards,
Shivanshi
 

<79C90400E51C4EC6A197393CD98D0F7A.png>
 
 
<CC6C706EF927441BBD49E964355D5AA6.png>


Re: [PATCH v1 1/1] MdeModulePkg: Initialize temp variable in VarCheckPolicyLib

Bret Barkelew
 

Looks like we have the requisite approvals. Shall I create a PR?

 

- Bret

 

From: Wu, Hao A via groups.io
Sent: Sunday, April 11, 2021 7:23 PM
To: Bret Barkelew; devel@edk2.groups.io
Cc: Wang, Jian J
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg: Initialize temp variable in VarCheckPolicyLib

 

> -----Original Message-----
> From: Bret Barkelew <bret@...>
> Sent: Saturday, April 10, 2021 2:25 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>
> Subject: [PATCH v1 1/1] MdeModulePkg: Initialize temp variable in
> VarCheckPolicyLib
>
> DumpVariablePolicy() will return EFI_INVALID_PARAMETER if the Buffer
> pointer is NULL and the indirect Size is anything but 0. Since this TempSize
> was not being initialized it is very likely that this sequence would not return
> the total buffer size as expected.
>
> Bugzilla: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3310&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cdd597013a0874fa676f708d8fd5a02e2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637537910324146700%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=qxCQ6vccJFzG8gH9qDBHD9xnl%2FCs3DdVUuMtHspanfY%3D&amp;reserved=0
>
> Cc: Jian J Wang <jian.j.wang@...>
> Cc: Hao A Wu <hao.a.wu@...>
> Signed-off-by: Bret Barkelew <bret.barkelew@...>
> ---
>  MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> index 14e1904e96d3..e50edb4ffc5a 100644
> --- a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> +++ b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> @@ -216,6 +216,7 @@ VarCheckPolicyLibMmiHandler (
>          DumpParamsOut->TotalSize = 0;
>          DumpParamsOut->PageSize = 0;
>          DumpParamsOut->HasMore = FALSE;
> +        TempSize = 0;
>          SubCommandStatus = DumpVariablePolicy (NULL, &TempSize);


Reviewed-by: Hao A Wu <hao.a.wu@...>

Best Regards,
Hao Wu


>          if (SubCommandStatus == EFI_BUFFER_TOO_SMALL && TempSize > 0) {
>            mCurrentPaginationCommand =
> VAR_CHECK_POLICY_COMMAND_DUMP;
> --
> 2.28.0.windows.1





 


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

Laszlo Ersek
 

On 04/10/21 00:43, Brijesh Singh wrote:

On 4/9/21 7:24 AM, Laszlo Ersek wrote:
On 04/08/21 13:59, Brijesh Singh wrote:
On 4/8/21 4:58 AM, Laszlo Ersek wrote:
On 03/24/21 16:31, Brijesh Singh wrote:
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).
What is the primary threat (in simple terms please) that validation
is supposed to prevent?

To protect against the memory re-mapping attack the guest pages must
be validated. The idea is that every guest page can map only to a
single physical memory page at one time.
I don't understand. How can a given guest page frame map to multiple
host page frames?

Do you mean that the situation to prevent is when multiple guest page
frames (GPAs) map to the same host page frame, as set up by the
hypervisor?


And, against that particular threat, does pre-validation offer some
protection, or will that only come with lazy validation?
For the hardware it does not matter how the memory was validated --
lazy vs prevalidate. Both approaches use the PVALIDATE instruction to
validate the page.

In the case of pre-validation, the memory is validated before the
access. Whereas in the lazy validation, the access will cause a fault
and fault handler should validate the page and retry the access. Its
similar to a page fault handler, OS can populate the page table or
back the pages on demand.

The only downside of pre-validation is, we will take a hit on the boot
time. The GHCB spec provides method by which we can batch multiple
requests at once to minimize the context switches.
If pre-validation is simpler, and the only drawback is a one-time hit
during boot, then wouldn't it be better to stick with pre-validation
forever? I expect that would be a lot simpler for (a) the #VC handlers,
(b) the tracking of the "already validated" information.


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.
This means the two pflash chips, right?

This does not need to be two pflash chips. The SNP firmware command
does not know anything about the ROM or pflash chip. The command
accepts the system physical address that need to be validated by the
firmware. In patch #2, OVMF provides a range of data pages that need
to be validated by the SNP firmware before booting the guest.
I guess my question related to the guest code that executes from pflash,
and/or accesses data from pflash, before the guest itself could ask for
any validation. Such as, the reset vector (which runs from pflash).
Then, some non-volatile UEFI variable data that resides in the other
pflash chip, and is accessed (read-only) during the PEI phase (the
memory type information variable namely). I understood your comments as
QEMU pre-validating those areas before guest launch. Is that correct?

Anyway, I guess at this point I should just start reviewing the series.

Some more comments below.


- We need more areas made accessible in SEC than just the
decompression buffer; for example the temporary SEC/PEI heap and
stack, and (IIRC) various special SEV-ES areas laid out via MEMFD.
Where are all of those handled / enumerated?

Sorry, I simplified my response by saying just decompression. You are
right that its more than the decompression buffer. In my current
patch, the SEC phase validates all the memory up to
PcdOvmfDecompressionScratchEnd (which includes more than just output
buffer). See patch #13.
That sounds good (will check the patch out of course).


One of the important thing is we should *never* validate the pages
twice.
What are the symptoms / consequences of:

- the guest accessing an unvalidated page (I understand it causes a
#VC, but what is the direct result of that, when this series is
applied?),

After the series it applied, an access to an unvalidated page will
result in #VC. The series does not extend the VC handler to handle the
page-not-validated error code. The current VC handler
[InternalVmgExitHandleVc()] will inject a #GP to terminate the guest
if #VC is raised for unvalidated page.
OK.


- doubly-validating a page?

The first question is relevant because we should crash as soon as we
touch a page we forgot to validate (we shouldn't let any corruption
or similar spread out until we finally realize there's a problem).

The second question is relevant for security I guess. What attacks
become possible, and/or what symptoms are produced, if we
doubly-validate a page?
Assuming that guest validates its memory, this guarantees the
injective mapping between GPAs and SPAs.

As I noted that the page validation process consist of two steps #1
RMPUPDATE and 2#PVALIDATE. The RMPUPDATE is a hypervisor instruction
and PVALIDATE is a guest instruction. A malicious hypervisor can remap
gpa to different physical address in RMP table using the RMPUPDATE
just before the second PVALIDATE instruction is executed. The guest
need to ensure that it does not leave the security vulnerability that
can be exploited by a malicious hypervisor. The SNP white paper
recommends that a guest OS should keep track of what is has validate
so that it can avoid getting into these situation.
Hm, OK. I think this answers my top-most question.


Furthermore, IIRC, we have separate #VC handlers for the different
firmware phases; do they behave consistently / identicall when a
#VC(page-not-validated) occurs, when this patch set is applied?

My first question is basically asking whether we can *exclusively*
rely on #VC(page-not-validated) faults to tell us that we missed
validating a particular page. If we can do that, then the job is a
bit easier, because from the GPA, we can more or less also derive
*when and where* we should pre-validate the page (at least until
validation is done completely lazily).
Yes, we should be able to rely #VC to tell us that we missed
validating the page. In my this series so far I have not seen a #VC
due to page-not-validated because we have carefully validated the
pages before they are accessed. I am thinking that
#VC(page-not-validated) should be treated as an error in the first
phase. Incrementally we will add the lazy validation, in which the
#VC(page-not-validated) will provide a hint as to what has not been
validated.
I'm OK with the current handling of #VC(page-not-validated) -- i.e.,
fatal error.

I'm not yet convinced that lazy validation will be useful at all, but
that could be just my lack of understanding.


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. hat data
structure is used for implementing the interval tree?
I'm not necessarily looking for a data structure with "nice"
asymptotic time complexity. With pre-validation especially, I think
simplicity (ease of review) is more important for the data structure
than performance. If it's not an actual "tree", we shouldn't call it
a "tree". (An "interval tree" is usually an extension of a Red-Black
Tree, and that's not the simplest data structure in existence;
although edk2 does offer an rbtree library.)

I am using binary search tree. Currently, we don't have many nodes to
track. Most of the guest memory is private and are validated before we
exit from the PEI phase.
My preliminary comments about MemEncryptSevSnpValidateSystemRam():

(1) The function prototype (patch#12) promises that the function can
never fail. In patch#14 however, I see some AddRangeToIntervalTree()
calls whose return values are not checked. I think error checking should
be strict, and if an error is detected (such as memory allocation
failure), a plain ASSERT (FALSE) is not sufficient. Both ASSERT() and
CpuDeadLoop() should be used explicitly, for the sake of RELEASE builds.
And the function prototype (the leading comment) should highlight that
the function either succeeds, or doesn't return.


(2) I don't like the recursive nature of the functions. That's always
convenient first, and almost always a mess to get out of later. However,
this is not a request to complicate the tree's implementation,
because...


(3) ... you mention we don't have many nodes to track. Is a tree
structure, with dynamically allocated nodes, justified at all?


(4) Regarding repeating dynamic allocation -- that's never a great thing
to do in the SEC / PEI phases. Can we use a fixed size, once-allocated,
or even static, array somewhere?

In particular, patch#14 uses AllocatePool() for SNP_VALIDATED_RANGE, in
the PEI phase, that results in EFI_HOB_TYPE_MEMORY_POOL HOBs; see:

AllocatePool() [MdePkg/Library/PeiMemoryAllocationLib/MemoryAllocationLib.c]
PeiServicesAllocatePool() [MdePkg/Library/PeiServicesLib/PeiServicesLib.c]
PeiAllocatePool() [MdeModulePkg/Core/Pei/Memory/MemoryServices.c]

Now, dependent on where you call this AllocatePool() from, it could
occur still on the temporary SEC/PEI RAM -- i.e., before permanent PEI
RAM is installed. That means that such HOBs would be subject to the HOB
list migration, after permanent PEI RAM is installed. The PI spec writes
about EFI_HOB_MEMORY_POOL: "The HOB consumer phase should be able to
ignore these HOBs". The PI spec writes about AllocatePool(): "The early
allocations from temporary memory will be migrated to permanent memory
when permanent main memory is installed; this migration shall occur when
the HOB list is migrated to permanent memory".

So, whenever we use AllocatePool() in the PEI phase, we need an "address
stability proof", namely that the AllocatePool() call, and any later
access to the allocated area (the HOB), are never separated by the
installation of "gEfiPeiMemoryDiscoveredPpiGuid" in the PPI database, by
the PEI Core. If we can't guarantee that, then an access later on could
occur to the original allocation address, which has by then been
invalidated by the temporary RAM migration (= the allocation HOB has
been moved).

AllocatePages() is "stable" however, in this sense; no proof like the
one described above is necessary.


(5) The "mRootNode" variable is defined in the file
"OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c"
(patch #14). That means that every single module (i.e., PEIM) which
consumes this library instance will own a separate "mRootNode" variable,
with a validation-tracking tree that's correspondingly private to that
PEIM.

But the validation state is global to the entire firmware. I think we
should somehow clarify, or even enforce, that the tracking data
structure is firmware-global.

Perhaps this can be solved by (a) moving the ownership of the data
structure to a particular firmware module, with sole responsibility for
pre-validation, and (b) updating the validation APIs to take this data
structure explicitly.

Alternatively, if multiple modules are expected to inter-operate on the
tracker data structure (by which I mean inter-operation *in sequence*,
not concurrently in a multiprocessing sense), then we might need a
dynamic PCD for inter-driver understanding.


(NB: both points (4) and (5) seem to conflict with *existent* code in
BaseMemEncryptSevLib. However:

- concerning (4), the current allocations are all performed with
AllocateAlignedPages(), which are unaffected by temporary-to-permanent
RAM migration,

- and concerning (5), namely the existent "mPageTablePool" global
variable, is a *known bug* -- see commit b721aa749b86a and
TianoCore#847. This issue (namely, multiple modules messing with the
page tables independently) remains unsolved, and I'd like to avoid
introducing *more* paging-related allocations that are module-owned,
and not firmware-level.)


All in all, here's what I'd prefer (and please correct me if it's
unrealistic):

- make the SNP validation functions take a simple, *unresizeable*
tracker structure for input/output,

- allocate that data structure with a single AllocatePages() call in the
proper PEIM,

- if the SNP validation functions run out of space in the structure,
hang hard,

- avoid recursion, and use a simple linear search through the array,

- evaluate whether graceful handling of overlaps (= range splitting,
merging) is actually a requirement. See more on this below.


(6) Can you please confirm that the order in which
MemEncryptSevSnpValidateSystemRam()'s declaration, call sites, and
implementations are introduced, is sensible? I can't tell immediately,
but I'd like to be sure that the tree at least builds at every stage (no
linkage errors at any stage).


Furthermore, what you describe above is called idempotency. No matter
how many times we attempt to validate a range, it may (or may not
even) cause an actual change in the first action only. Is this
property (=idempotency) an inherent requirement of the technology, or
is it a simplification of the implementation? Put differently: if you
called CpuDeadLoop() in the validation function any time an
overlapping validation request were received, would that hugely
complicate the call sites?
From the technology point of view you can validate the same page again
and again. Hardware does not force any restriction. To protect against
time-based remap attack a guest also need to ensure that it does not
blindly validates the pages otherwise guest is taking risk. Avoiding a
double validation is strong recommendation. In current implementation,
the validation burden is not put on the caller. Caller calls standard
APIs to toggle the encryption attribute. Under the hood, if we see
that page is already validated then no need to issue the PVALIDATE. If
the page is getting changed from C=1 -> 0 then unvalidate it.
There remains a huge gap in my understanding here. (The rest of my
earlier questions did concern the relationship between the C bit and
validation, but I might as well comment right at this spot.)

"Invalidation in connection to C-bit toggling" means that there's going
to be significant "validation churn" in the DXE phase. For example when
fw_cfg is used (which relies on the edk2 IOMMU protocol), or when virtio
transfers are performed (which also relies on the edk2 IOMMU protocol,
although less directly -- through PciIo, namely).

That, however, brings up several problems for me:

- Is the tracker data structure scalable? I don't see any balancing in
it. (And I wouldn't want a new, complex data structure for it anyway.)

- In fact, I don't see any code related to invalidation -- more
precisely, I don't see where and how the C-bit changes consult the
tracker structure *at all*.

- Given issue (5), I don't see how the tracker structure would be
inherited from the PEI phase to the DXE phase.

In particular, in patch#18, the flipping of the C-bit results in calls
to SetPageStateInternal(). But SetPageStateInternal() does not go
*through* the tracker structure. Even in patch#14,
SevSnpValidateSystemRam() calls SetPageStateInternal() *in addition to*
AddRangeToIntervalTree().

In other words, I don't see where validity is tracked in the DXE phase,
in response to the C-bit toggling.


I'm kind of "obsessing" about idempotency because you say we must
*never* doubly-validate a page, so the *difference* between:
- explicitly crashing on such an attempt,
- and silently ignoring such an attempt,
may be meaningful.

I said we should never doubly validate to avoid creating a
vulnerability that can later be exploited by the malicious hypervisor.
Currently we lookup address in our tree to ensure that we don't do a
double validation. The PVALIDATE instruction also can be used to
determine if we are attempting to validate an already validated page.
In current implementation after the PVALIDATE completion, I check the
rflags.CF to ensure that its not a double validation condition. If its
double validation then abort the guest. Its bug in the guest.

It's kind of the difference between "oops this is a call site *bug*,
but we patched it up", vs. "this is expected of call sites, we should
just handle it internally".
Our attempt should be to patch to avoid the double validation. I would
still check the status of PVALIDATE, if instruction detects it as a
double validation then we have a bug in our tracking and should abort
the guest.
Let me rephrase. My question concerns the *abstraction level* at which
we should catch and handle double-validation attempts.

If we silently split and merge ranges in the tracker structure, then the
guest (as a whole) will not double-validate, which is good. However,
double-validation attempts will potentially still *exist*, in
higher-level modules in the firmware. My question is whether it is
*right* to paper over such overlaps, coming from the high-level code, in
the tracker structure. I suspect that it's not right.

In other words, the spot where we should abort, due to
"double-validation detected", is not when the PVALIDATE instruction
reports a problem in the tracker structure. Instead, the spot where we
should abort is when we detect an overlapping request *before* issuing
PVALIDATE. I'd like the tracker to be as simple and dumb as possible,
and I'd like the actual sites that request validation to blow up
immediately, if they issue an overlapping request. Is my expectation
wrong?


The patch #18 extend the MemEncrypt{Set,Clear}PageEncMask() to call
the SNP page state change during the C-bit toggle.
- When exactly do we invalidate?
Basically before the page is transition from C=1 -> 0 in the page
table.


- Does the time and place of invalidation depend on whether we
perform pre-validation, or lazy validation?
It does not matter. Both type of validation update the Validated bit
in the RMP table. The important thing is that invalidation can happen
only for a already validated private page. If the page is shared then
invalidation will result in #GP.

- Is page invalidation idempotent too?
We should treat this as idempotent too. As I said, tacking the page
state is strongly recommended for the security reason. The SNP white
paper has a section dedicated for the validation and may able to
clarify things which is not covered in my response.

- What is the consequence (security impact) if we forget
invalidation?
I can't think of a security implication of it but if we forget to
invalidate then it can lead issues in the tracking data structure when
it comes to validation next time. e.g a page was mapped as C=1 and we
changed it to C=0 without invalidating it. Now if the same page is
being changed from C=0 -> 1 then tracking data structure may think
that page is already validated and will skip the validation process
and thus access will result in #VC (page-not-validated).
As I said above, I don't see where the validation tracking and the C-bit
flipping "meet". Especially across the PEI and DXE phases.

Basically I'm missing the core concept for maintaining a firmware-global
structure, for tracking the validation status of GPA intervals.

Up to and *excluding* the DXE phase, the validation tracking should be
trivial, naive, and unforgiving. This is because, in the PEI phase,
validation is one-shot. I would expect the tracker structure to live in
the PlatformPei module.

Starting with the DXE phase, where we need (in)validation to closely
accompany C-bit toggling, we need a different, performant data structure
for tracking validation status. This data structure should be primed
from the status last set up in the PEI phase. The tracking should still
be unforgiving. I believe *this* tracker structure may have to live in
the IOMMU driver.

BaseMemEncryptSevLib may offer helper functions, but even if it does, it
should never attempt to *own* tracking information, via an internal
global variable. Furthermore, those helper functions that only make
sense for a particular firmware phase, should have such implementations
that hang hard, in library instances that match the *other* firmware
phases. For example, SevSnpValidateSystemRam() should have a
DXE-specific implementation that hangs hard.

It's entirely possible that my points above are complete garbage. I
don't have a grasp on the core concept, for the firmware-wide tracking
of the validation status.

... Ugh, wait. I've just noticed something. In patch#19,
"SEC_SEV_ES_WORK_AREA.SnpSystemRamValidatedRootAddress" is introduced,
which seems (?) to be implementing the kind of global (inter-driver,
inter-phase) tracking that I've been missing. However, even if my guess
(?) is correct about that, this idea definitely doesn't belong in the
last patch, presented as a "bugfix". Even if it were a bugfix, it should
be incorporated into one of the earlier patches (so that we preferably
not introduce the bug in the first place). However, my argument is that
it's not a bugfix -- to me that seems to be the core concept. That's
what the whole series needs to be built upon. (In all of my thinking,
before this particular paragraph, I *never* looked at patch#19. There
was no reason to.)


- There are four page states I can imagine:
- encrypted for host access, valid for guest access
- encrypted for host access, invalid for guest access
- decrypted for host access, valid for guest access
- decrypted for host access, invalid for guest access

Does a state exist, from these four, that's never used? (Potentially
caught by the hardware?)
I would not mix the page state with the host access. Basically there
is two page state exist

- Guest-Valid

- Guest-Invalid

By default all the guest memory is in Guest-Invalid state on boot. The
PVALIDATE instruction is used to transition from Valid->Invalid.
Do you mean "Invalid->Valid" transition?


Guest can use the PVALIDATE to change the state from Valid to Invalid.

A guest private page (i.e page mapped C=1) must be in Guest Valid
state. If hardware see that a private access page is not in the
Guest-Valid state then it will trigger #VC.



Do the patches highlight / explain the validity transitions, in
comments? My understanding is that the C-bit toggle and the guest
access valid/invalid toggle are separate actions, so "middle" states
do exist, but perhaps only temporarily.
I have tried to document the steps in the patch description and I can
add more comments as required. When SEV-SNP is active then we need to
invalidate the page before toggling the C-bit from C=1 -> 0 and
similarly after the page is toggled from C=0 -> 1 we must validate it.


I'm curious how it works, for example, with variousvirtio transfers
(bus master read, bus master write, bus master common buffer). In the
IoMmuDxe driver minimally, we access memory both through PTEs with
the C-bit set and through PTEs with the C-bit clear, meaning that
"encrypted, valid", and "decrypted, valid" are both required states.
But that seems to conflict with the notion that "C-bit toggle" be
directly coupled with a "validity toggle".
That should not be an issue. Each page have entry in the RMP table.
So, you can have one page as guest invalid and another page as a guest
valid.

Put differently, I'm happy that modifying IoMmuDxe appears
unnecessary, but then that tells me I'm missing something about the
state transitions between the above *four* states.

Only thing which I would propose changing in IoMmuDxe driver is to use
a pre-allocated buffer and avoid toggling the C-bit on every
read/write. In past the C-bit toggle was all contained inside the
guest. But when SNP is enabled, each C-bit toggle causes two
additional steps #1 RMPUPDATE and #2 PVALIDATE. The RMPUPDATE is
requested using the VMEXIT so it will cause a VM context switch. I
would prefer to avoid it.

My current patch set does not have this optimization yet.
Please let's stick with the absolute minimum in this series, yes.

I realize my email is complete chaos. That's because my understanding of
this work is complete chaos.

Can we approach this feature as follows:

- Make "validation tracking" the core tenet (central principle) of this
feature.

- Design the validation *patterns* that are required in each firmware
phase.

- Identify the *sole* owner module, for validation tracking, in each
firmware phase. If we can't readily do this, we might have to
introduce new drivers (PEIMs, DXE drivers?) and new interfaces (PPIs,
protocols?)

- Design the hand-off between the owner modules (on firmware phase
boundaries).

- Add only helper functions to BaseMemEncryptSevLib, without any
ownership of tracking info. The helpers should be as restricted as
possible.

I'd like to avoid "smearing" page validation responsibility over a bunch
of modules. (See again: TianoCore#847.) I'd like responsibilities
strictly centralized.

If you feel that my points make no sense, I can start going over the
individual patches. However, even that way, I'd have found patch#19 only
in the end, and that doesn't seem right. And I wouldn't like to go
"numb" on the actual code, being distracted by style issues and so on,
before we have some agreement on the core approach.

Another tip: if you can add content to MdePkg that is governed by AMD
specifications, such as patches #3, #5, #9, I would recommend splitting
that off to a separate (pre-requisite) series. That content seems
unaffected by whatever we do in OvmfPkg, and it would help decrease the
size of the OvmfPkg series. It's not like we're going to abandon the
SEV-SNP feature, so I don't think there's a risk in merging the MdePkg
series first, while the OvmfPkg series is not ready. The MdePkg stuff
will not remain unused indefinitely.

Thanks,
Laszlo


Re: [PATCH] IntelSiliconPkg/ShadowMicrocode: Fix build failure

Chaganty, Rangasai V
 

Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Monday, April 12, 2021 1:45 AM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
Subject: [PATCH] IntelSiliconPkg/ShadowMicrocode: Fix build failure

The commit 7e4c6f982a0accd5aa86337b46d20199db989aeb
updated ShadowMicrocode module to consume MicrocodeLib.

But the change caused the build failure.

The patch fixed the build failure and also verified the change in real platform.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
---
.../Feature/ShadowMicrocode/ShadowMicrocodePei.c | 3 +--
Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc | 1 +
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.c b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.c
index 4e4b69a0ca..7f4a3f8fbd 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicroc
+++ odePei.c
@@ -291,7 +291,6 @@ ShadowMicrocode (
UINTN MaxPatchNumber; CPU_MICROCODE_HEADER *MicrocodeEntryPoint; UINTN PatchCount;- UINTN DataSize; UINTN TotalSize; UINTN TotalLoadSize; @@ -342,7 +341,7 @@ ShadowMicrocode (
if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE) { MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) FitEntry[Index].Address; TotalSize = GetMicrocodeLength (MicrocodeEntryPoint);- if (IsValidMicrocode (MicrocodeEntryPoint, TotalSize, MicrocodeCpuId, CpuIdCount, FALSE)) {+ if (IsValidMicrocode (MicrocodeEntryPoint, TotalSize, 0, MicrocodeCpuId, CpuIdCount, FALSE)) { PatchInfoBuffer[PatchCount].Address = (UINTN) MicrocodeEntryPoint; PatchInfoBuffer[PatchCount].Size = TotalSize; TotalLoadSize += TotalSize;diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
index edc79c9b9c..5e0de7e19a 100644
--- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
+++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
@@ -39,6 +39,7 @@
MicrocodeFlashAccessLib|IntelSiliconPkg/Feature/Capsule/Library/MicrocodeFlashAccessLibNull/MicrocodeFlashAccessLibNull.inf PeiGetVtdPmrAlignmentLib|IntelSiliconPkg/Library/PeiGetVtdPmrAlignmentLib/PeiGetVtdPmrAlignmentLib.inf TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf+ MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf [LibraryClasses.common.PEIM] PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf--
2.27.0.windows.1


GCC49 DEBUG AARCH64 and ARM builds use -O0

Rebecca Cran
 

I noticed the GCC49 (and GCC48) AARCH64 and ARM DEBUG builds use -O0, unlike IA32 and X64 platforms which build with -Os.

e.g. from https://github.com/tianocore/edk2/blob/master/BaseTools/Conf/tools_def.template :

DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0

Is that deliberate, or should it be like X64 where DEBUG builds are optimized and NOOPT is used when unoptimized binaries are needed?

--
Rebecca Cran


[PATCH v1 1/1] Fix AsmReadMsr64() and AsmWriteMsr64() with GCC toolchain

 

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

1. AsmReadMsr64() in X64/GccInlinePriv.c
AsmReadMsr64 can return uninitialized value if FilterBeforeMsrRead
returns False. This causes build error with the CLANG toolchain.

2. AsmWriteMsr64() in X64/GccInlinePriv.c
In the case that FilterBeforeMsrWrite changes Value and returns True,
The original Value, not the changed Value, is written to the MSR.
This behavior is different from the one of AsmWriteMsr64() in
X64/WriteMsr64.c for the MSFT toolchain.

Signed-off-by: Takuto Naito <naitaku@gmail.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdePkg/Library/BaseLib/X64/GccInlinePriv.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/MdePkg/Library/BaseLib/X64/GccInlinePriv.c b/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
index e4920f2116..244bd62ee6 100644
--- a/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
+++ b/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
@@ -80,7 +80,7 @@ AsmReadMsr64 (
}
FilterAfterMsrRead (Index, &Value);

- return (((UINT64)HighData) << 32) | LowData;
+ return Value;
}

/**
@@ -111,11 +111,10 @@ AsmWriteMsr64 (
UINT32 HighData;
BOOLEAN Flag;

- LowData = (UINT32)(Value);
- HighData = (UINT32)(Value >> 32);
-
Flag = FilterBeforeMsrWrite (Index, &Value);
if (Flag) {
+ LowData = (UINT32)(Value);
+ HighData = (UINT32)(Value >> 32);
__asm__ __volatile__ (
"wrmsr"
:
--
2.31.1


[PATCH v1 0/1] Fix AsmReadMsr64() and AsmWriteMsr64() with GCC toolchain

 

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

This patch fixes the problems of AsmReadMsr64/AsmWriteMsr64 for the GCC toolchain
introduced when RegisterFilterLib support was added.

Patch v1 branch: https://github.com/naitaku/edk2/tree/patch/fix_read_msr_with_gcc

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>

Takuto Naito (1):
Fix AsmReadMsr64() and AsmWriteMsr64() with GCC toolchain

MdePkg/Library/BaseLib/X64/GccInlinePriv.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

--
2.31.1


Re: [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest

Brijesh Singh
 

Hi James and Laszlo,

I was planning to work to add the support to reserve the Secrets and
CPUID page in E820 map and then create the EFI configuration table entry
for it so that guest OS can reach to it. We have two packages
"SecretDxe" and "SecretPei" in OvmfPkg/AmdSev. Any issues if I use them
in the OvmfPkg.dsc ? Here is what I was thinking:

1) Rename the PcdSevLaunchSecretBase -> PcdSevSecretsBase

2) When SNP is enabled then VMM use this page as secrets page for the SNP

3) When SEV or SEV-ES is enabled then VMM uses this page as a launch
secret page

This will allow me to drop PcdOvmfSnpSecretsBase. This will not just
save 4-bytes but also minimize the code duplication.

Thoughts ?

-Brijesh

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

During the SEV-SNP guest launch sequence, two special pages need to
be inserted, the secrets page and cpuid page. The secrets page,
contain the VM platform communication keys. The guest BIOS and OS
can use this key to communicate with the SEV firmware to get the
attestation report. The Cpuid page, contain the CPUIDs entries
filtered through the AMD-SEV firmware.

The VMM will locate the secrets and cpuid page addresses through a
fixed GUID and pass them to SEV firmware to populate further.
For more information about the page content, see the SEV-SNP spec.

To simplify the pre-validation range calculation in the next patch,
the CPUID and Secrets pages are moved to the start of the
MEMFD_BASE_ADDRESS.

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>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/OvmfPkg.dec | 8 +++++++
OvmfPkg/OvmfPkgX64.fdf | 24 ++++++++++++--------
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 19 ++++++++++++++++
OvmfPkg/ResetVector/ResetVector.inf | 4 ++++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 ++
5 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4348bb45c6..062926772d 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -317,6 +317,14 @@
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43

+ ## The base address of the CPUID page used by SEV-SNP
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase|0|UINT32|0x48
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize|0|UINT32|0x49
+
+ ## The base address of the Secrets page used by SEV-SNP
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|0|UINT32|0x50
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize|0|UINT32|0x51
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index d519f85328..ea214600be 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -67,27 +67,33 @@ ErasePolarity = 1
BlockSize = 0x10000
NumBlocks = 0xD0

-0x000000|0x006000
+0x000000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize
+
+0x001000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
+
+0x002000|0x006000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize

-0x006000|0x001000
+0x008000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize

-0x007000|0x001000
+0x009000|0x001000
gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize

-0x008000|0x001000
+0x00A000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize

-0x009000|0x002000
+0x00B000|0x002000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize

-0x00B000|0x001000
-gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
-
-0x00C000|0x001000
+0x00D000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize

+0x00F000|0x001000
+gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
+
0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 9c0b5853a4..5456f02924 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -47,6 +47,25 @@ TIMES (15 - ((guidedStructureEnd - guidedStructureStart + 15) % 16)) DB 0
;
guidedStructureStart:

+;
+; SEV-SNP boot support
+;
+; sevSnpBlock:
+; For the initial boot of SEV-SNP guest, a Secrets and CPUID page must be
+; reserved by the BIOS at a RAM area defined by SEV_SNP_SECRETS_PAGE
+; and SEV_SNP_CPUID_PAGE. A VMM will locate this information using the
+; SEV-SNP boot block.
+;
+; GUID (SEV-SNP boot block): bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9
+;
+sevSnpBootBlockStart:
+ DD SEV_SNP_SECRETS_PAGE
+ DD SEV_SNP_CPUID_PAGE
+ DW sevSnpBootBlockEnd - sevSnpBootBlockStart
+ DB 0xC2, 0xC0, 0x39, 0xBD, 0x8e, 0x2F, 0x43, 0x42
+ DB 0x83, 0xE8, 0x1B, 0x74, 0xCE, 0xBC, 0xB7, 0xD9
+sevSnpBootBlockEnd:
+
;
; SEV Secret block
;
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index dc38f68919..d890bb6b29 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -37,6 +37,10 @@
gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 5fbacaed5f..2c194958f4 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -75,6 +75,8 @@
%define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
%define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
%define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
+ %define SEV_SNP_SECRETS_PAGE FixedPcdGet32 (PcdOvmfSnpSecretsBase)
+ %define SEV_SNP_CPUID_PAGE FixedPcdGet32 (PcdOvmfSnpCpuidBase)
%define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
%include "Ia32/Flat32ToFlat64.asm"
%include "Ia32/PageTables64.asm"

6881 - 6900 of 80786