Date   

Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

Jeff Brasen
 

We actually can't open the BlockIo protocol as BY_DRIVER as it is already owned by the DiskIoDxe driver. Will upload a v3 with the other feedback though.

Thanks,

Jeff


From: Marvin Häuser <mhaeuser@...>
Sent: Friday, September 10, 2021 12:09 PM
To: Pedro Falcato <pedro.falcato@...>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Jeff Brasen <jbrasen@...>
Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
 
External email: Use caution opening links or attachments


On 10/09/2021 19:08, Pedro Falcato wrote:
> Ah yes, thanks! Although I didn't find the part where they say that
> you need to use the same attributes,
> after re-reading the spec it seems they recommend using BY_DRIVER.

UEFI 2.9, 11.1, "Device Driver", 2.: "It must use the same Attribute
value that was used in Supported()."

> So, change it to BY_DRIVER. The performance should be the same and
> it's more consistent.

Should be better because all handles opened by BY_DRIVER already will
immediately fail.

Best regards,
Marvin

>
> On Fri, Sep 10, 2021 at 5:56 PM Marvin Häuser <mhaeuser@...> wrote:
>> On 10/09/2021 18:52, Pedro Falcato wrote:
>>> Like Marvin raised in v1, it's a good idea to change the first
>>> OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER
>>> into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake.
>>> Since we're not keeping these protocols open, using BY_DRIVER makes no
>>> difference.
>> No, the UEFI spec demands Supported() to use the same Attributes as
>> Start() (likely for these very performance reasons), actually I thought
>> both need BY_DRIVER.
>>
>> Best regards,
>> Marvin
>>
>>> You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c
>>> when you changed it in the header.
>>> Just a nitpick, but if you could quickly align the
>>> Ext4SuperblockCheckMagic's parameters' descriptions like
>>>
>>>      @param[in] DiskIo      Pointer to the DiskIo.
>>>      @param[in] BlockIo     Pointer to the BlockIo.
>>>
>>> it would be excellent.
>>>
>>> Apart from that, looks great to me and the patch series will get my RB
>>> after that quick change.
>>>
>>> Best regards,
>>>
>>> Pedro
>>>
>>> On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen <jbrasen@...> wrote:
>>>> A couple of improvements to improve performance.
>>>> Add check to return ACCESS_DENIED if already connected
>>>> Add check to verify superblock magic during supported to reduce start calls
>>>>
>>>> Signed-off-by: Jeff Brasen <jbrasen@...>
>>>> ---
>>>>    Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>>>>    Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 58 +++++++++++++++++++++------
>>>>    Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++
>>>>    3 files changed, 95 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>>>> index 64eab455db..5c60860894 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>>>> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
>>>>      OUT UINTN          *VolNameLen
>>>>      );
>>>>
>>>> +/**
>>>> +   Checks the superblock's magic value.
>>>> +
>>>> +   @param[in] DiskIo     Pointer to the DiskIo.
>>>> +   @param[in] BlockIo     Pointer to the BlockIo.
>>>> +
>>>> +   @return TRUE if a valid ext4 superblock, else FALSE.
>>>> +**/
>>>> +BOOLEAN
>>>> +Ext4SuperblockCheckMagic (
>>>> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
>>>> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
>>>> +  );
>>>> +
>>>>    #endif
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>>>> index ea2e048d77..5ae93d0484 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>>>> @@ -631,7 +631,6 @@ Ext4Unload (
>>>>      @retval EFI_ACCESS_DENIED        The device specified by ControllerHandle and
>>>>                                       RemainingDevicePath is already being managed by a different
>>>>                                       driver or an application that requires exclusive access.
>>>> -                                   Currently not implemented.
>>>>      @retval EFI_UNSUPPORTED          The device specified by ControllerHandle and
>>>>                                       RemainingDevicePath is not supported by the driver specified by This.
>>>>    **/
>>>> @@ -643,32 +642,67 @@ Ext4IsBindingSupported (
>>>>      IN EFI_DEVICE_PATH *RemainingDevicePath  OPTIONAL
>>>>      )
>>>>    {
>>>> -  // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
>>>> -  // protocol and ignore the output argument entirely
>>>> +  EFI_STATUS            Status;
>>>> +  EFI_DISK_IO_PROTOCOL  *DiskIo;
>>>> +  EFI_BLOCK_IO_PROTOCOL *BlockIo;
>>>>
>>>> -  EFI_STATUS  Status;
>>>> +  DiskIo = NULL;
>>>> +  BlockIo = NULL;
>>>>
>>>> +  //
>>>> +  // Open the IO Abstraction(s) needed to perform the supported test
>>>> +  //
>>>>      Status = gBS->OpenProtocol (
>>>>                      ControllerHandle,
>>>>                      &gEfiDiskIoProtocolGuid,
>>>> -                  NULL,
>>>> -                  BindingProtocol->ImageHandle,
>>>> +                  (VOID **) &DiskIo,
>>>> +                  BindingProtocol->DriverBindingHandle,
>>>>                      ControllerHandle,
>>>> -                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
>>>> +                  EFI_OPEN_PROTOCOL_BY_DRIVER
>>>>                      );
>>>>
>>>>      if (EFI_ERROR (Status)) {
>>>> -    return Status;
>>>> +    goto Exit;
>>>>      }
>>>> -
>>>> +  //
>>>> +  // Open the IO Abstraction(s) needed to perform the supported test
>>>> +  //
>>>>      Status = gBS->OpenProtocol (
>>>>                      ControllerHandle,
>>>>                      &gEfiBlockIoProtocolGuid,
>>>> -                  NULL,
>>>> -                  BindingProtocol->ImageHandle,
>>>> +                  (VOID **) &BlockIo,
>>>> +                  BindingProtocol->DriverBindingHandle,
>>>>                      ControllerHandle,
>>>> -                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
>>>> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
>>>>                      );
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    goto Exit;
>>>> +  }
>>>> +
>>>> +  if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
>>>> +    Status = EFI_UNSUPPORTED;
>>>> +  }
>>>> +
>>>> +Exit:
>>>> +  //
>>>> +  // Close the I/O Abstraction(s) used to perform the supported test
>>>> +  //
>>>> +  if (DiskIo != NULL) {
>>>> +    gBS->CloseProtocol (
>>>> +          ControllerHandle,
>>>> +          &gEfiDiskIoProtocolGuid,
>>>> +          BindingProtocol->DriverBindingHandle,
>>>> +          ControllerHandle
>>>> +          );
>>>> +  }
>>>> +  if (BlockIo != NULL) {
>>>> +    gBS->CloseProtocol (
>>>> +          ControllerHandle,
>>>> +          &gEfiBlockIoProtocolGuid,
>>>> +          BindingProtocol->DriverBindingHandle,
>>>> +          ControllerHandle
>>>> +          );
>>>> +  }
>>>>      return Status;
>>>>    }
>>>>
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>>>> index c321d8c3d8..1ceb0d5cbb 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>>>> @@ -34,6 +34,41 @@ STATIC CONST UINT32  gSupportedIncompatFeat =
>>>>    // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
>>>>    // references and add some Ext4SignalCorruption function + function call.
>>>>
>>>> +/**
>>>> +   Checks only superblock magic value.
>>>> +
>>>> +   @param[in] DiskIo     Pointer to the DiskIo.
>>>> +   @param[in] BlockIo     Pointer to the BlockIo.
>>>> +
>>>> +   @return TRUE if a valid ext4 superblock, else FALSE.
>>>> +**/
>>>> +BOOLEAN
>>>> +Ext4SuperblockCheckMagic (
>>>> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
>>>> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
>>>> +  )
>>>> +{
>>>> +  UINT16      Magic;
>>>> +  EFI_STATUS  Status;
>>>> +
>>>> +  Status = DiskIo->ReadDisk (
>>>> +                     DiskIo,
>>>> +                     BlockIo->Media->MediaId,
>>>> +                     EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
>>>> +                     sizeof (Magic),
>>>> +                     &Magic
>>>> +                     );
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    return FALSE;
>>>> +  }
>>>> +
>>>> +  if (Magic != EXT4_SIGNATURE) {
>>>> +    return FALSE;
>>>> +  }
>>>> +
>>>> +  return TRUE;
>>>> +}
>>>> +
>>>>    /**
>>>>       Does brief validation of the ext4 superblock.
>>>>
>>>> --
>>>> 2.17.1
>>>>
>


Re: [PATCH v7 0/9] Ovmf: Disable the TPM2 platform hierarchy

Stefan Berger
 

On 9/10/21 12:15 PM, Stefan Berger wrote:


On 9/10/21 11:32 AM, Yao, Jiewen wrote:
According to the security policy, PP request must be processed before EndOfDxe.

May I know when you trigger PP request?
OVMF has 3 implementations invoking it in PlatformBootManagerAfterConsole():

https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c#L1517

https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformBootManagerLibBhyve/BdsPlatform.c#L1451

https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c#L1316
Before I post yet another series...:

The problem is that PPI may require interaction with the console, so it seems we have to handle it in PlatformBootManagerAfterConsole(). The disablement of the TPM 2 platform hierarchy may only occur after that, so we have to move this part here after TPM-PPI-Handling from BeforeConsole() into AfterConsole() because this is what triggers that new code from edk2-platforms to disable that TPM 2 platform hierarchy:

  Status = gBS->InstallProtocolInterface (&Handle,
                  &gEfiDxeSmmReadyToLockProtocolGuid, EFI_NATIVE_INTERFACE,
                  NULL);

And then we have this part here also in BeforeConsole() that has to be moved as well into AfterConsole().

  //
  // Dispatch deferred images after EndOfDxe event and ReadyToLock
  // installation.
  //
  EfiBootManagerDispatchDeferredImages ();


This then leads to something like this with the sequence (TPM-PPI-handling, gEfiDxeSmmReadyToLockProtocol, EfiBootManagerDispatchDeferredImages) needing to stay in that order. However, I am not sure know whether one can just move these parts around like this.


diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 71f63b2448..266d58dfbe 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -354,7 +354,6 @@ PlatformBootManagerBeforeConsole (
   VOID
   )
 {
-  EFI_HANDLE    Handle;
   EFI_STATUS    Status;
   UINT16        FrontPageTimeout;
   RETURN_STATUS PcdStatus;
@@ -387,8 +386,10 @@ PlatformBootManagerBeforeConsole (
     SaveS3BootScript ();
   }

+#if 0
   //
   // Prevent further changes to LockBoxes or SMRAM.
+  // Any TPM 2 Physical Presence Interface opcode must be handled BEFORE
   //
   Handle = NULL;
   Status = gBS->InstallProtocolInterface (&Handle,
@@ -401,6 +402,7 @@ PlatformBootManagerBeforeConsole (
   // installation.
   //
   EfiBootManagerDispatchDeferredImages ();
+#endif

   PlatformInitializeConsole (
     XenDetected() ? gXenPlatformConsole : gPlatformConsole);
@@ -437,6 +439,7 @@ PlatformBootManagerBeforeConsole (
   //
   VisitAllInstancesOfProtocol (&gEfiPciIoProtocolGuid, ConnectVirtioPciRng,
     NULL);
+
 }


@@ -1474,6 +1477,8 @@ PlatformBootManagerAfterConsole (
   VOID
   )
 {
+  EFI_STATUS                         Status;
+  EFI_HANDLE                         Handle;
   EFI_BOOT_MODE                      BootMode;

   DEBUG ((DEBUG_INFO, "PlatformBootManagerAfterConsole\n"));
@@ -1511,11 +1516,29 @@ PlatformBootManagerAfterConsole (
   //
   PciAcpiInitialization ();

+#if 1
   //
-  // Process TPM PPI request
+  // Process TPM PPI request; this may require interaction via console
   //
   Tcg2PhysicalPresenceLibProcessRequest (NULL);

+  //
+  // Prevent further changes to LockBoxes or SMRAM.
+  // Any TPM 2 Physical Presence Interface opcode must be handled BEFORE
+  //
+  Handle = NULL;
+  Status = gBS->InstallProtocolInterface (&Handle,
+                  &gEfiDxeSmmReadyToLockProtocolGuid, EFI_NATIVE_INTERFACE,
+                  NULL);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Dispatch deferred images after EndOfDxe event and ReadyToLock
+  // installation.
+  //
+  EfiBootManagerDispatchDeferredImages ();
+#endif
+
   //
   // Process QEMU's -kernel command line option
   //


   Stefan


Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

Marvin Häuser <mhaeuser@...>
 

On 10/09/2021 19:08, Pedro Falcato wrote:
Ah yes, thanks! Although I didn't find the part where they say that
you need to use the same attributes,
after re-reading the spec it seems they recommend using BY_DRIVER.
UEFI 2.9, 11.1, "Device Driver", 2.: "It must use the same Attribute value that was used in Supported()."

So, change it to BY_DRIVER. The performance should be the same and
it's more consistent.
Should be better because all handles opened by BY_DRIVER already will immediately fail.

Best regards,
Marvin


On Fri, Sep 10, 2021 at 5:56 PM Marvin Häuser <mhaeuser@...> wrote:
On 10/09/2021 18:52, Pedro Falcato wrote:
Like Marvin raised in v1, it's a good idea to change the first
OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER
into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake.
Since we're not keeping these protocols open, using BY_DRIVER makes no
difference.
No, the UEFI spec demands Supported() to use the same Attributes as
Start() (likely for these very performance reasons), actually I thought
both need BY_DRIVER.

Best regards,
Marvin

You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c
when you changed it in the header.
Just a nitpick, but if you could quickly align the
Ext4SuperblockCheckMagic's parameters' descriptions like

@param[in] DiskIo Pointer to the DiskIo.
@param[in] BlockIo Pointer to the BlockIo.

it would be excellent.

Apart from that, looks great to me and the patch series will get my RB
after that quick change.

Best regards,

Pedro

On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen <jbrasen@...> wrote:
A couple of improvements to improve performance.
Add check to return ACCESS_DENIED if already connected
Add check to verify superblock magic during supported to reduce start calls

Signed-off-by: Jeff Brasen <jbrasen@...>
---
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++------
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++
3 files changed, 95 insertions(+), 12 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 64eab455db..5c60860894 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
OUT UINTN *VolNameLen
);

+/**
+ Checks the superblock's magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ );
+
#endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index ea2e048d77..5ae93d0484 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -631,7 +631,6 @@ Ext4Unload (
@retval EFI_ACCESS_DENIED The device specified by ControllerHandle and
RemainingDevicePath is already being managed by a different
driver or an application that requires exclusive access.
- Currently not implemented.
@retval EFI_UNSUPPORTED The device specified by ControllerHandle and
RemainingDevicePath is not supported by the driver specified by This.
**/
@@ -643,32 +642,67 @@ Ext4IsBindingSupported (
IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL
)
{
- // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
- // protocol and ignore the output argument entirely
+ EFI_STATUS Status;
+ EFI_DISK_IO_PROTOCOL *DiskIo;
+ EFI_BLOCK_IO_PROTOCOL *BlockIo;

- EFI_STATUS Status;
+ DiskIo = NULL;
+ BlockIo = NULL;

+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiDiskIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &DiskIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_BY_DRIVER
);

if (EFI_ERROR (Status)) {
- return Status;
+ goto Exit;
}
-
+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiBlockIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &BlockIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
+ if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
+ Status = EFI_UNSUPPORTED;
+ }
+
+Exit:
+ //
+ // Close the I/O Abstraction(s) used to perform the supported test
+ //
+ if (DiskIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiDiskIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
+ if (BlockIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiBlockIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
return Status;
}

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index c321d8c3d8..1ceb0d5cbb 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -34,6 +34,41 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
// this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
// references and add some Ext4SignalCorruption function + function call.

+/**
+ Checks only superblock magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ )
+{
+ UINT16 Magic;
+ EFI_STATUS Status;
+
+ Status = DiskIo->ReadDisk (
+ DiskIo,
+ BlockIo->Media->MediaId,
+ EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
+ sizeof (Magic),
+ &Magic
+ );
+ if (EFI_ERROR (Status)) {
+ return FALSE;
+ }
+
+ if (Magic != EXT4_SIGNATURE) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
/**
Does brief validation of the ext4 superblock.

--
2.17.1


Re: [edk2-libc Patch 1/1] edk2-libc/Readme.md: Updated Readme.md with Python 3.6.8 License details

Rebecca Cran <rebecca@...>
 

Pushed as ce35be387725cf6bb1caefa4d4b0bd48ea72cbfa.


--

Rebecca Cran

On 9/8/21 10:54 AM, Jayaprakash Nevara wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3611

This commit contains updates made to the Readme.md file
of edk2-libc repo to add the Python 3.6.8 license at
appropriate section in the file

Cc: Rebecca Cran <rebecca@...>
Cc: Michael D Kinney <michael.d.kinney@...>
---
Readme.md | 1 +
1 file changed, 1 insertion(+)

diff --git a/Readme.md b/Readme.md
index 341be14..0012cd5 100644
--- a/Readme.md
+++ b/Readme.md
@@ -24,6 +24,7 @@ contains the following components that are covered by additional licenses:
* [AppPkg/Applications/Python/Python-2.7.2/Tools/pybench](AppPkg/Applications/Python/Python-2.7.2/Tools/pybench/LICENSE)
* [AppPkg/Applications/Python/Python-2.7.2](AppPkg/Applications/Python/Python-2.7.2/LICENSE)
* [AppPkg/Applications/Python/Python-2.7.10](AppPkg/Applications/Python/Python-2.7.10/LICENSE)
+* [AppPkg/Applications/Python/Python-3.6.8](AppPkg/Applications/Python/Python-3.6.8/LICENSE)
The EDK II LIBC Project is composed of packages. The maintainers for each
package are listed in [Maintainers.txt](Maintainers.txt).


Re: [edk2-libc Patch 1/1] edk2-libc/Readme.md: Updated Readme.md with Python 3.6.8 License details

Rebecca Cran <rebecca@...>
 

Sorry for the delay -- I've been trying to find time to make sure I can follow the process correctly.

I've just pushed the change as ce35be387725cf6bb1caefa4d4b0bd48ea72cbfa.

For future patches, please add a Signed-off-by line.


--
Rebecca Cran


On 9/9/21 12:12 AM, Jayaprakash, N wrote:
Hello Rebecca,

Would you be able to review and merge this change?

Regards,
JP

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jayaprakash, N
Sent: 08 September 2021 22:24
To: devel@edk2.groups.io
Cc: Rebecca Cran <rebecca@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: [edk2-devel] [edk2-libc Patch 1/1] edk2-libc/Readme.md: Updated Readme.md with Python 3.6.8 License details

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

This commit contains updates made to the Readme.md file of edk2-libc repo to add the Python 3.6.8 license at appropriate section in the file

Cc: Rebecca Cran <rebecca@...>
Cc: Michael D Kinney <michael.d.kinney@...>
---
 Readme.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Readme.md b/Readme.md
index 341be14..0012cd5 100644
--- a/Readme.md
+++ b/Readme.md
@@ -24,6 +24,7 @@ contains the following components that are covered by additional licenses:
 * [AppPkg/Applications/Python/Python-2.7.2/Tools/pybench](AppPkg/Applications/Python/Python-2.7.2/Tools/pybench/LICENSE)
 * [AppPkg/Applications/Python/Python-2.7.2](AppPkg/Applications/Python/Python-2.7.2/LICENSE)
 * [AppPkg/Applications/Python/Python-2.7.10](AppPkg/Applications/Python/Python-2.7.10/LICENSE)
+* 
+[AppPkg/Applications/Python/Python-3.6.8](AppPkg/Applications/Python/Py
+thon-3.6.8/LICENSE)
 
 The EDK II LIBC Project is composed of packages.  The maintainers for each  package are listed in [Maintainers.txt](Maintainers.txt).
--
2.32.0.windows.2








TianoCore Community Meeting Minutes - September 2021

Soumya Guptha
 

TianoCore Community Meeting
September 2, 2021
 
Events:
Google Summer of Code:
  • A very successful summer with students completing the projects and code checked in to EDK2 repo.
  • Those projects that need to be added to the EDK2 master, mentors need to bring those to Liming and Maintainers attention. Maintainers of those projects to discuss with Liming and add those projects that are candidates for the EDK2 master, to the upcoming stable tag releases.
  • Community Action:
  • Mentors to communicate to Liming if any of the projects are a candidate to EDK2 master.
  • Need to advertise successes with an email of the summary/projects, share the content. Soumya to discuss with Nate.
 
UEFI Plugfest update from Dick Wilkins:
  • Concerns around pandemic travel that may impact April ’22 plugfest. Will people be able to travel? Still evaluating.
  • Budget concerns: UEFI forum is trying to keep the dues low. Sponsorships from many companies are impacted which impacts the budget for plugfest. Questions are being raised – do we have enough new technologies to provide test material for people to get together for testing, should we have a plugfest? Can we find the funds to run the plugfest?
  • Can we charge registration fees for the attendees? to cover food, evening event etc. like any other industry events.
  • Community Action: provide input to Dick Wilkins Dick_Wilkins@... on these questions –
  • Is your company interested in sponsoring?
  • Are you willing to pay for the conference?
 
Stable Tag updates:
  • Stable tag 202108 has been released.
  • Next stable tag 202111 is collecting features.
  • Community Action: please send your feature requests ahead of time to Liming.
 
Stewards Download:
  • Not much update for this month.
  • Stable tag 202108 release went smooth. The new process with 2 week gap between hard freeze and release worked well.
 
Opens: none
 
 
 
--------------
 
 
Regards,
Soumya Guptha
 
TianoCore Community Manager
 
 
 

 
 


Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

Pedro Falcato
 

Ah yes, thanks! Although I didn't find the part where they say that
you need to use the same attributes,
after re-reading the spec it seems they recommend using BY_DRIVER.

So, change it to BY_DRIVER. The performance should be the same and
it's more consistent.

On Fri, Sep 10, 2021 at 5:56 PM Marvin Häuser <mhaeuser@...> wrote:

On 10/09/2021 18:52, Pedro Falcato wrote:
Like Marvin raised in v1, it's a good idea to change the first
OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER
into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake.
Since we're not keeping these protocols open, using BY_DRIVER makes no
difference.
No, the UEFI spec demands Supported() to use the same Attributes as
Start() (likely for these very performance reasons), actually I thought
both need BY_DRIVER.

Best regards,
Marvin

You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c
when you changed it in the header.
Just a nitpick, but if you could quickly align the
Ext4SuperblockCheckMagic's parameters' descriptions like

@param[in] DiskIo Pointer to the DiskIo.
@param[in] BlockIo Pointer to the BlockIo.

it would be excellent.

Apart from that, looks great to me and the patch series will get my RB
after that quick change.

Best regards,

Pedro

On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen <jbrasen@...> wrote:
A couple of improvements to improve performance.
Add check to return ACCESS_DENIED if already connected
Add check to verify superblock magic during supported to reduce start calls

Signed-off-by: Jeff Brasen <jbrasen@...>
---
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++------
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++
3 files changed, 95 insertions(+), 12 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 64eab455db..5c60860894 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
OUT UINTN *VolNameLen
);

+/**
+ Checks the superblock's magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ );
+
#endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index ea2e048d77..5ae93d0484 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -631,7 +631,6 @@ Ext4Unload (
@retval EFI_ACCESS_DENIED The device specified by ControllerHandle and
RemainingDevicePath is already being managed by a different
driver or an application that requires exclusive access.
- Currently not implemented.
@retval EFI_UNSUPPORTED The device specified by ControllerHandle and
RemainingDevicePath is not supported by the driver specified by This.
**/
@@ -643,32 +642,67 @@ Ext4IsBindingSupported (
IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL
)
{
- // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
- // protocol and ignore the output argument entirely
+ EFI_STATUS Status;
+ EFI_DISK_IO_PROTOCOL *DiskIo;
+ EFI_BLOCK_IO_PROTOCOL *BlockIo;

- EFI_STATUS Status;
+ DiskIo = NULL;
+ BlockIo = NULL;

+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiDiskIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &DiskIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_BY_DRIVER
);

if (EFI_ERROR (Status)) {
- return Status;
+ goto Exit;
}
-
+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiBlockIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &BlockIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
+ if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
+ Status = EFI_UNSUPPORTED;
+ }
+
+Exit:
+ //
+ // Close the I/O Abstraction(s) used to perform the supported test
+ //
+ if (DiskIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiDiskIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
+ if (BlockIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiBlockIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
return Status;
}

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index c321d8c3d8..1ceb0d5cbb 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -34,6 +34,41 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
// this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
// references and add some Ext4SignalCorruption function + function call.

+/**
+ Checks only superblock magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ )
+{
+ UINT16 Magic;
+ EFI_STATUS Status;
+
+ Status = DiskIo->ReadDisk (
+ DiskIo,
+ BlockIo->Media->MediaId,
+ EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
+ sizeof (Magic),
+ &Magic
+ );
+ if (EFI_ERROR (Status)) {
+ return FALSE;
+ }
+
+ if (Magic != EXT4_SIGNATURE) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
/**
Does brief validation of the ext4 superblock.

--
2.17.1
--
Pedro Falcato


Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

Jeff Brasen
 

Yeah we need by_driver on at least one protocol to prevent re-binding to the controller. It will return ALREADY_STARTED if this driver already has it open (from in Start) or ACCESS_DENIED if another driver is holding this.

I can covert both to by_driver if that is desired.

Will make the other little changes as well once I get a desired direction on how to open BlockIo.

Thanks,

Jeff


From: Marvin Häuser <mhaeuser@...>
Sent: Friday, September 10, 2021 10:56 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; pedro.falcato@... <pedro.falcato@...>; Jeff Brasen <jbrasen@...>
Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
 
External email: Use caution opening links or attachments


On 10/09/2021 18:52, Pedro Falcato wrote:
> Like Marvin raised in v1, it's a good idea to change the first
> OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER
> into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake.
> Since we're not keeping these protocols open, using BY_DRIVER makes no
> difference.

No, the UEFI spec demands Supported() to use the same Attributes as
Start() (likely for these very performance reasons), actually I thought
both need BY_DRIVER.

Best regards,
Marvin

> You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c
> when you changed it in the header.
> Just a nitpick, but if you could quickly align the
> Ext4SuperblockCheckMagic's parameters' descriptions like
>
>     @param[in] DiskIo      Pointer to the DiskIo.
>     @param[in] BlockIo     Pointer to the BlockIo.
>
> it would be excellent.
>
> Apart from that, looks great to me and the patch series will get my RB
> after that quick change.
>
> Best regards,
>
> Pedro
>
> On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen <jbrasen@...> wrote:
>> A couple of improvements to improve performance.
>> Add check to return ACCESS_DENIED if already connected
>> Add check to verify superblock magic during supported to reduce start calls
>>
>> Signed-off-by: Jeff Brasen <jbrasen@...>
>> ---
>>   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>>   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 58 +++++++++++++++++++++------
>>   Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++
>>   3 files changed, 95 insertions(+), 12 deletions(-)
>>
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> index 64eab455db..5c60860894 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
>>     OUT UINTN          *VolNameLen
>>     );
>>
>> +/**
>> +   Checks the superblock's magic value.
>> +
>> +   @param[in] DiskIo     Pointer to the DiskIo.
>> +   @param[in] BlockIo     Pointer to the BlockIo.
>> +
>> +   @return TRUE if a valid ext4 superblock, else FALSE.
>> +**/
>> +BOOLEAN
>> +Ext4SuperblockCheckMagic (
>> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
>> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
>> +  );
>> +
>>   #endif
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>> index ea2e048d77..5ae93d0484 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>> @@ -631,7 +631,6 @@ Ext4Unload (
>>     @retval EFI_ACCESS_DENIED        The device specified by ControllerHandle and
>>                                      RemainingDevicePath is already being managed by a different
>>                                      driver or an application that requires exclusive access.
>> -                                   Currently not implemented.
>>     @retval EFI_UNSUPPORTED          The device specified by ControllerHandle and
>>                                      RemainingDevicePath is not supported by the driver specified by This.
>>   **/
>> @@ -643,32 +642,67 @@ Ext4IsBindingSupported (
>>     IN EFI_DEVICE_PATH *RemainingDevicePath  OPTIONAL
>>     )
>>   {
>> -  // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
>> -  // protocol and ignore the output argument entirely
>> +  EFI_STATUS            Status;
>> +  EFI_DISK_IO_PROTOCOL  *DiskIo;
>> +  EFI_BLOCK_IO_PROTOCOL *BlockIo;
>>
>> -  EFI_STATUS  Status;
>> +  DiskIo = NULL;
>> +  BlockIo = NULL;
>>
>> +  //
>> +  // Open the IO Abstraction(s) needed to perform the supported test
>> +  //
>>     Status = gBS->OpenProtocol (
>>                     ControllerHandle,
>>                     &gEfiDiskIoProtocolGuid,
>> -                  NULL,
>> -                  BindingProtocol->ImageHandle,
>> +                  (VOID **) &DiskIo,
>> +                  BindingProtocol->DriverBindingHandle,
>>                     ControllerHandle,
>> -                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
>> +                  EFI_OPEN_PROTOCOL_BY_DRIVER
>>                     );
>>
>>     if (EFI_ERROR (Status)) {
>> -    return Status;
>> +    goto Exit;
>>     }
>> -
>> +  //
>> +  // Open the IO Abstraction(s) needed to perform the supported test
>> +  //
>>     Status = gBS->OpenProtocol (
>>                     ControllerHandle,
>>                     &gEfiBlockIoProtocolGuid,
>> -                  NULL,
>> -                  BindingProtocol->ImageHandle,
>> +                  (VOID **) &BlockIo,
>> +                  BindingProtocol->DriverBindingHandle,
>>                     ControllerHandle,
>> -                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
>> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
>>                     );
>> +  if (EFI_ERROR (Status)) {
>> +    goto Exit;
>> +  }
>> +
>> +  if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
>> +    Status = EFI_UNSUPPORTED;
>> +  }
>> +
>> +Exit:
>> +  //
>> +  // Close the I/O Abstraction(s) used to perform the supported test
>> +  //
>> +  if (DiskIo != NULL) {
>> +    gBS->CloseProtocol (
>> +          ControllerHandle,
>> +          &gEfiDiskIoProtocolGuid,
>> +          BindingProtocol->DriverBindingHandle,
>> +          ControllerHandle
>> +          );
>> +  }
>> +  if (BlockIo != NULL) {
>> +    gBS->CloseProtocol (
>> +          ControllerHandle,
>> +          &gEfiBlockIoProtocolGuid,
>> +          BindingProtocol->DriverBindingHandle,
>> +          ControllerHandle
>> +          );
>> +  }
>>     return Status;
>>   }
>>
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>> index c321d8c3d8..1ceb0d5cbb 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>> @@ -34,6 +34,41 @@ STATIC CONST UINT32  gSupportedIncompatFeat =
>>   // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
>>   // references and add some Ext4SignalCorruption function + function call.
>>
>> +/**
>> +   Checks only superblock magic value.
>> +
>> +   @param[in] DiskIo     Pointer to the DiskIo.
>> +   @param[in] BlockIo     Pointer to the BlockIo.
>> +
>> +   @return TRUE if a valid ext4 superblock, else FALSE.
>> +**/
>> +BOOLEAN
>> +Ext4SuperblockCheckMagic (
>> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
>> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
>> +  )
>> +{
>> +  UINT16      Magic;
>> +  EFI_STATUS  Status;
>> +
>> +  Status = DiskIo->ReadDisk (
>> +                     DiskIo,
>> +                     BlockIo->Media->MediaId,
>> +                     EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
>> +                     sizeof (Magic),
>> +                     &Magic
>> +                     );
>> +  if (EFI_ERROR (Status)) {
>> +    return FALSE;
>> +  }
>> +
>> +  if (Magic != EXT4_SIGNATURE) {
>> +    return FALSE;
>> +  }
>> +
>> +  return TRUE;
>> +}
>> +
>>   /**
>>      Does brief validation of the ext4 superblock.
>>
>> --
>> 2.17.1
>>
>


Re: [PATCH V5 1/2] OvmfPkg: Introduce Tdx BFV/CFV PCDs and PcdOvmfImageSizeInKb

Erdem Aktas
 

I have few naive questions. Sorry if the answers were obvious.

TDVF also include a configuration firmware volume (CFV) that is separated
from the BFV. The reason is because the CFV is measured in RTMR, while
the BFV is measured in MRTD.
If I understand correctly, this means that the BFV is encrypted and
measured during TD build time. Since CFV is not included in the MRTD,
CFV region is not encrypted with the guest key, is it?

The measurement value of the CFV (provisioned configuration data) is extended to
RTMR registers (similar to TPM PCRs). At the same time it is recorded in the TD Event
log.
Even if it is measured at runtime, the content needs to be copied to
somewhere else, otherwise what stops VMM to change the content after
it is being measured (assuming that it is not encrypted).

As to the spare part in varstore, it is not external input, is it? It's produced and consumed
by code itself. From this perspective it should not be measured. If the spare part
is included in the measurement, then the *good* measurement is not known anymore.
Because no one knows about the content of spare part in advance.
I am confused about how this memory is initialized. If it is
encrypted, then no need to measure it but also it becomes useless as
the key will change in the next boot. If it is not encrypted, VMM can
always modify the content and might cause unexpected behavior at
runtime, right?

I might be missing something here but if this region is not encrypted:
- CFV content needs to be copied into an encrypted buffer after being
measured and should never be used again.
- Allowing variables to be stored in SPARE part seems like opening an
attack surface as no one knows what will be stored in that region.

Is this correct understanding?
-Erdem


On Wed, Sep 1, 2021 at 10:20 PM Andrew Fish <afish@...> wrote:


On Sep 1, 2021, at 9:53 AM, James Bottomley <jejb@...> wrote:

On Wed, 2021-09-01 at 08:59 +0000, Yao, Jiewen wrote:
Hi Min
I agree with Gerd and Ard in this case.

It is NOT so obvious that the FTW is produced then consumed in the
code. What if the attacker prepares some special configuration to
trigger the FTW process at the first boot, the code will do *read*
before *write*? That is a potential attack surface.
It's not just that: even if you can ensure nothing in the host changed
the variables, how do you know *your* code inside the guest is updating
them? In ordinary OVMF we try to ensure that by having the variables
SMM protected so the only update path available to the kernel is via
the setVariable interface, but we can't do that in the confidential
computing case because SMM isn't supported. That means a random kernel
attacker in the guest can potentially write to the var store too.

At least for the first SEV prototype I had to make the var store part
of the first firmware volume firstly so it got measured but secondly so
it couldn't be used as a source of configuration attacks.

I have a nasty feeling that configuration attacks are going to be the
bane of all confidential computing solutions because they give the
untrusted VMM a wide attack surface.
James,

If we take a big step back the requirement for an EFI Runtime Service, like the variable API, is just exclusive access to hardware at OS runtime. The variable store needs to be on a hardware device that has a persistent reliable store. The FTW is really about maintaining the consistency of the store if the power gets yanked at the wrong moment. So the fact that the UEFI Variable Store is in NOR FLASH is a historical artifact more than architecture. Also on physical devices hardware cost money, and you need the NOR FLASH for the firmware so why change it. Thus conceptually the variable store could be backed by a virtual hardware device that was designed with security in mind. Maybe more of message passing interface and the reliability of updates is maintained by the hardware device not the UEFI code. It would also be possible for the hardware device to enforce security policy. You could even have EFI send a one shot message per 1st boot to the hardware to define a security policy. If you wanted the hardware device could even implement the UEFI Secure Boot infrastructure so the UEFI Variable Driver could be untrusted. I guess this hypothetical variable store virtual hardware device could also have hardware access to other security hardware resources (like a TPM) and implement security policies based on that.

FYI on Macs with a T2 (security chip) the UEFI variable store lives on the T2.

Thanks,

Andrew Fish


James







Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

Marvin Häuser <mhaeuser@...>
 

On 10/09/2021 18:52, Pedro Falcato wrote:
Like Marvin raised in v1, it's a good idea to change the first
OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER
into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake.
Since we're not keeping these protocols open, using BY_DRIVER makes no
difference.
No, the UEFI spec demands Supported() to use the same Attributes as Start() (likely for these very performance reasons), actually I thought both need BY_DRIVER.

Best regards,
Marvin

You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c
when you changed it in the header.
Just a nitpick, but if you could quickly align the
Ext4SuperblockCheckMagic's parameters' descriptions like

@param[in] DiskIo Pointer to the DiskIo.
@param[in] BlockIo Pointer to the BlockIo.

it would be excellent.

Apart from that, looks great to me and the patch series will get my RB
after that quick change.

Best regards,

Pedro

On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen <jbrasen@...> wrote:
A couple of improvements to improve performance.
Add check to return ACCESS_DENIED if already connected
Add check to verify superblock magic during supported to reduce start calls

Signed-off-by: Jeff Brasen <jbrasen@...>
---
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++------
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++
3 files changed, 95 insertions(+), 12 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 64eab455db..5c60860894 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
OUT UINTN *VolNameLen
);

+/**
+ Checks the superblock's magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ );
+
#endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index ea2e048d77..5ae93d0484 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -631,7 +631,6 @@ Ext4Unload (
@retval EFI_ACCESS_DENIED The device specified by ControllerHandle and
RemainingDevicePath is already being managed by a different
driver or an application that requires exclusive access.
- Currently not implemented.
@retval EFI_UNSUPPORTED The device specified by ControllerHandle and
RemainingDevicePath is not supported by the driver specified by This.
**/
@@ -643,32 +642,67 @@ Ext4IsBindingSupported (
IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL
)
{
- // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
- // protocol and ignore the output argument entirely
+ EFI_STATUS Status;
+ EFI_DISK_IO_PROTOCOL *DiskIo;
+ EFI_BLOCK_IO_PROTOCOL *BlockIo;

- EFI_STATUS Status;
+ DiskIo = NULL;
+ BlockIo = NULL;

+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiDiskIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &DiskIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_BY_DRIVER
);

if (EFI_ERROR (Status)) {
- return Status;
+ goto Exit;
}
-
+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiBlockIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &BlockIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
+ if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
+ Status = EFI_UNSUPPORTED;
+ }
+
+Exit:
+ //
+ // Close the I/O Abstraction(s) used to perform the supported test
+ //
+ if (DiskIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiDiskIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
+ if (BlockIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiBlockIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
return Status;
}

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index c321d8c3d8..1ceb0d5cbb 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -34,6 +34,41 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
// this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
// references and add some Ext4SignalCorruption function + function call.

+/**
+ Checks only superblock magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ )
+{
+ UINT16 Magic;
+ EFI_STATUS Status;
+
+ Status = DiskIo->ReadDisk (
+ DiskIo,
+ BlockIo->Media->MediaId,
+ EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
+ sizeof (Magic),
+ &Magic
+ );
+ if (EFI_ERROR (Status)) {
+ return FALSE;
+ }
+
+ if (Magic != EXT4_SIGNATURE) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
/**
Does brief validation of the ext4 superblock.

--
2.17.1


Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

Pedro Falcato
 

Like Marvin raised in v1, it's a good idea to change the first
OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER
into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake.
Since we're not keeping these protocols open, using BY_DRIVER makes no
difference.

You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c
when you changed it in the header.
Just a nitpick, but if you could quickly align the
Ext4SuperblockCheckMagic's parameters' descriptions like

@param[in] DiskIo Pointer to the DiskIo.
@param[in] BlockIo Pointer to the BlockIo.

it would be excellent.

Apart from that, looks great to me and the patch series will get my RB
after that quick change.

Best regards,

Pedro

On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen <jbrasen@...> wrote:

A couple of improvements to improve performance.
Add check to return ACCESS_DENIED if already connected
Add check to verify superblock magic during supported to reduce start calls

Signed-off-by: Jeff Brasen <jbrasen@...>
---
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++------
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++
3 files changed, 95 insertions(+), 12 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 64eab455db..5c60860894 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
OUT UINTN *VolNameLen
);

+/**
+ Checks the superblock's magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ );
+
#endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index ea2e048d77..5ae93d0484 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -631,7 +631,6 @@ Ext4Unload (
@retval EFI_ACCESS_DENIED The device specified by ControllerHandle and
RemainingDevicePath is already being managed by a different
driver or an application that requires exclusive access.
- Currently not implemented.
@retval EFI_UNSUPPORTED The device specified by ControllerHandle and
RemainingDevicePath is not supported by the driver specified by This.
**/
@@ -643,32 +642,67 @@ Ext4IsBindingSupported (
IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL
)
{
- // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
- // protocol and ignore the output argument entirely
+ EFI_STATUS Status;
+ EFI_DISK_IO_PROTOCOL *DiskIo;
+ EFI_BLOCK_IO_PROTOCOL *BlockIo;

- EFI_STATUS Status;
+ DiskIo = NULL;
+ BlockIo = NULL;

+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiDiskIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &DiskIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_BY_DRIVER
);

if (EFI_ERROR (Status)) {
- return Status;
+ goto Exit;
}
-
+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiBlockIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &BlockIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
+ if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
+ Status = EFI_UNSUPPORTED;
+ }
+
+Exit:
+ //
+ // Close the I/O Abstraction(s) used to perform the supported test
+ //
+ if (DiskIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiDiskIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
+ if (BlockIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiBlockIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
return Status;
}

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index c321d8c3d8..1ceb0d5cbb 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -34,6 +34,41 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
// this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
// references and add some Ext4SignalCorruption function + function call.

+/**
+ Checks only superblock magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ )
+{
+ UINT16 Magic;
+ EFI_STATUS Status;
+
+ Status = DiskIo->ReadDisk (
+ DiskIo,
+ BlockIo->Media->MediaId,
+ EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
+ sizeof (Magic),
+ &Magic
+ );
+ if (EFI_ERROR (Status)) {
+ return FALSE;
+ }
+
+ if (Magic != EXT4_SIGNATURE) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
/**
Does brief validation of the ext4 superblock.

--
2.17.1
--
Pedro Falcato


Re: [PATCH 1/2] Ext4Pkg: Improve Binding support behavior

Marvin Häuser <mhaeuser@...>
 

Good day,

On 09/09/2021 22:41, Jeff Brasen via groups.io wrote:
A couple improvements to improve performance.
Add check to return ACCESS_DENIED if already connected
This "performance improvement" actually aligns the load behaviour with the UEFI spec, maybe it should be mentioned?

Add check to verify superblock magic during supported to deduce start calls

Signed-off-by: Jeff Brasen <jbrasen@...>
---
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 57 +++++++++++++++++++++------
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 34 ++++++++++++++++
3 files changed, 92 insertions(+), 13 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 64eab455db..9a3938e671 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
OUT UINTN *VolNameLen
);
+/**
+ Checks only superblock magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ );
+
#endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index ea2e048d77..cb1e6d532a 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -631,7 +631,6 @@ Ext4Unload (
@retval EFI_ACCESS_DENIED The device specified by ControllerHandle and
RemainingDevicePath is already being managed by a different
driver or an application that requires exclusive access.
- Currently not implemented.
@retval EFI_UNSUPPORTED The device specified by ControllerHandle and
RemainingDevicePath is not supported by the driver specified by This.
**/
@@ -643,32 +642,64 @@ Ext4IsBindingSupported (
IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL
)
{
- // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
- // protocol and ignore the output argument entirely
-
- EFI_STATUS Status;
+ EFI_STATUS Status;
+ EFI_DISK_IO_PROTOCOL *DiskIo = NULL;
+ EFI_BLOCK_IO_PROTOCOL *BlockIo = NULL;
+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiDiskIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &DiskIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_BY_DRIVER
);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Exit;
This change is not required as nothing was opened on failure; see below.

}
-
+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiBlockIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &BlockIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
Why do the open modes mismatch if both protocols are equally stored in the private partition data? Is the owner of Block I/O a different one than the one of Disk I/O?

);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
+ if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
+ Status = EFI_UNSUPPORTED;
+ }
This can easily be rewritten to not require "goto":

  if (!EFI_ERROR (Status) && !Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
    Status = EFI_UNSUPPORTED;
  }

With the change above this allows to drop the label and any "goto" code.

+
+Exit:
+ //
+ // Close the I/O Abstraction(s) used to perform the supported test
+ //
+ if (DiskIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiDiskIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
+ if (BlockIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiBlockIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
Protocols retrieved by GET_PROTOCOL are not closed.

return Status;
}
diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index c321d8c3d8..1f8cdd3705 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -34,6 +34,40 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
// this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
// references and add some Ext4SignalCorruption function + function call.
+/**
+ Checks only superblock magic value.
Whether it "only" checks the magic is more of a functional detail than an actual description.

+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
Maybe use "@returns  Whether the partition has a valid EXT4 superblock magic" for readability?

Best regards,
Marvin

+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ )
+{
+ UINT16 Magic;
+ EFI_STATUS Status;
+
+ Status = DiskIo->ReadDisk (DiskIo,
+ BlockIo->Media->MediaId,
+ EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
+ sizeof (Magic),
+ &Magic
+ );
+ if (EFI_ERROR (Status)) {
+ return FALSE;
+ }
+
+ if (Magic != EXT4_SIGNATURE) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
/**
Does brief validation of the ext4 superblock.


Re: [PATCH v7 0/9] Ovmf: Disable the TPM2 platform hierarchy

Stefan Berger
 

On 9/10/21 11:32 AM, Yao, Jiewen wrote:
According to the security policy, PP request must be processed before EndOfDxe.

May I know when you trigger PP request?
OVMF has 3 implementations invoking it in PlatformBootManagerAfterConsole():

https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c#L1517

https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformBootManagerLibBhyve/BdsPlatform.c#L1451

https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c#L1316

  Stefan



Thank you
Yao Jiewen

-----Original Message-----
From: Stefan Berger <stefanb@...>
Sent: Friday, September 10, 2021 10:25 PM
To: devel@edk2.groups.io; stefanb@...
Cc: mhaeuser@...; spbrogan@...;
marcandre.lureau@...; kraxel@...; Yao, Jiewen
<jiewen.yao@...>
Subject: Re: [edk2-devel] [PATCH v7 0/9] Ovmf: Disable the TPM2 platform
hierarchy


On 9/9/21 1:35 PM, Stefan Berger wrote:
This series imports code from the edk2-platforms project related to
disabling the TPM2 platform hierarchy in Ovmf. It addresses the Ovmf
aspects of the following bugs:

https://bugzilla.tianocore.org/show_bug.cgi?id=3510
https://bugzilla.tianocore.org/show_bug.cgi?id=3499

I have patched the .dsc files and successfully test-built with most of
them. Some I could not build because they failed for other reasons
unrelated to this series.

I tested the changes with QEMU on x86 following the build of
OvmfPkgX64.dsc.

Neither one of the following commands should work anymore on first
try when run on Linux:

With IBM tss2 tools:
tsshierarchychangeauth -hi p -pwdn newpass

With Intel tss2 tools:
tpm2_changeauth -c platform newpass
While disabling the platform hierarchy works, the unfortunate problem is
now that the signal to disable the TPM 2 platform hierarchy is received
before handling the physical presence interface (PPI) opcodes, which is
bad because some of the opcodes will not go through. The question now is
what is wrong? Are the PPI opcodes handled too late or the signal is
sent to early or is it the wrong signal?

Event = EfiCreateProtocolNotifyEvent (
            &gEfiDxeSmmReadyToLockProtocolGuid,
            TPL_CALLBACK,
            SmmReadyToLockEventCallBack,
            NULL,
            &Registration
            );

   Stefan

Regards,
Stefan

v7:
- Ditched ARM support in this series
- Using Tcg2PlatformDxe and Tcg2PlaformPei from edk2-platforms now
and revised most of the patches

v6:
- Removed unnecessary entries in .dsc files
- Added support for S3 resume failure case
- Assigned unique FILE_GUID to NULL implementation

v5:
- Modified patch 1 copies the code from edk2-platforms
- Modified patch 2 fixes bugs in the code
- Modified patch 4 introduces required PCD

v4:
- Fixed and simplified code imported from edk2-platforms

v3:
- Referencing Null implementation on Bhyve and Xen platforms
- Add support in Arm


Stefan Berger (9):
SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
edk2-platforms
SecurityPkg/TPM: Fix bugs in imported PeiDxeTpmPlatformHierarchyLib
SecrutiyPkg/Tcg: Import Tcg2PlatformDxe from edk2-platforms
SecurityPkg/Tcg: Make Tcg2PlatformDxe buildable
SecurityPkg: Introduce new PCD PcdRandomizePlatformHierarchy
OvmfPkg: Reference new Tcg2PlatformDxe in the build system for
compilation
SecurityPkg/Tcg: Import Tcg2PlatformPei from edk2-platforms
SecurityPkg/Tcg: Make Tcg2PlatformPei buildable
OvmfPkg: Reference new Tcg2PlatformPei in the build system

OvmfPkg/AmdSev/AmdSevX64.dsc | 8 +
OvmfPkg/AmdSev/AmdSevX64.fdf | 2 +
OvmfPkg/OvmfPkgIa32.dsc | 8 +
OvmfPkg/OvmfPkgIa32.fdf | 2 +
OvmfPkg/OvmfPkgIa32X64.dsc | 8 +
OvmfPkg/OvmfPkgIa32X64.fdf | 2 +
OvmfPkg/OvmfPkgX64.dsc | 8 +
OvmfPkg/OvmfPkgX64.fdf | 2 +
.../Include/Library/TpmPlatformHierarchyLib.h | 27 ++
.../PeiDxeTpmPlatformHierarchyLib.c | 255 ++++++++++++++++++
.../PeiDxeTpmPlatformHierarchyLib.inf | 44 +++
SecurityPkg/SecurityPkg.dec | 6 +
.../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.c | 85 ++++++
.../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf | 43 +++
.../Tcg/Tcg2PlatformPei/Tcg2PlatformPei.c | 107 ++++++++
.../Tcg/Tcg2PlatformPei/Tcg2PlatformPei.inf | 51 ++++
16 files changed, 658 insertions(+)
create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
create mode 100644
SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
chyLib.c
create mode 100644
SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
chyLib.inf
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.c
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformPei/Tcg2PlatformPei.c
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformPei/Tcg2PlatformPei.inf


[PATCH v2 2/2] Ext4Pkg: Support uncleanly unmounted filesystems

Jeff Brasen
 

Support for uncleanly mounted filesystems, if there is a recovery
journal mark filesystem as read-only.

Signed-off-by: Jeff Brasen <jbrasen@...>
---
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 1ceb0d5cbb..251babb320 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -18,7 +18,7 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
EXT4_FEATURE_INCOMPAT_64BIT | EXT4_FEATURE_INCOMPAT_DIRDATA |
EXT4_FEATURE_INCOMPAT_FLEX_BG | EXT4_FEATURE_INCOMPAT_FILETYPE |
EXT4_FEATURE_INCOMPAT_EXTENTS | EXT4_FEATURE_INCOMPAT_LARGEDIR |
- EXT4_FEATURE_INCOMPAT_MMP;
+ EXT4_FEATURE_INCOMPAT_MMP | EXT4_FEATURE_INCOMPAT_RECOVER;

// Future features that may be nice additions in the future:
// 1) Btree support: Required for write support and would speed up lookups in large directories.
@@ -89,10 +89,8 @@ Ext4SuperblockValidate (
return FALSE;
}

- // Is this correct behaviour? Imagine the power cuts out, should the system fail to boot because
- // we're scared of touching something corrupt?
if ((Sb->s_state & EXT4_FS_STATE_UNMOUNTED) == 0) {
- return FALSE;
+ DEBUG ((DEBUG_WARN, "[ext4] Filesystem was not unmounted cleanly\n"));
}

return TRUE;
@@ -215,6 +213,11 @@ Ext4OpenSuperblock (
return EFI_UNSUPPORTED;
}

+ if (EXT4_HAS_INCOMPAT (Partition, EXT4_FEATURE_INCOMPAT_RECOVER)) {
+ DEBUG ((DEBUG_WARN, "[ext4] Needs journal recovery, mounting read-only\n"));
+ Partition->ReadOnly = TRUE;
+ }
+
// At the time of writing, it's the only supported checksum.
if (Partition->FeaturesCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM &&
Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C) {
--
2.17.1


[PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

Jeff Brasen
 

A couple of improvements to improve performance.
Add check to return ACCESS_DENIED if already connected
Add check to verify superblock magic during supported to reduce start calls

Signed-off-by: Jeff Brasen <jbrasen@...>
---
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++------
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++
3 files changed, 95 insertions(+), 12 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 64eab455db..5c60860894 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
OUT UINTN *VolNameLen
);

+/**
+ Checks the superblock's magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ );
+
#endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index ea2e048d77..5ae93d0484 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -631,7 +631,6 @@ Ext4Unload (
@retval EFI_ACCESS_DENIED The device specified by ControllerHandle and
RemainingDevicePath is already being managed by a different
driver or an application that requires exclusive access.
- Currently not implemented.
@retval EFI_UNSUPPORTED The device specified by ControllerHandle and
RemainingDevicePath is not supported by the driver specified by This.
**/
@@ -643,32 +642,67 @@ Ext4IsBindingSupported (
IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL
)
{
- // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
- // protocol and ignore the output argument entirely
+ EFI_STATUS Status;
+ EFI_DISK_IO_PROTOCOL *DiskIo;
+ EFI_BLOCK_IO_PROTOCOL *BlockIo;

- EFI_STATUS Status;
+ DiskIo = NULL;
+ BlockIo = NULL;

+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiDiskIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &DiskIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_BY_DRIVER
);

if (EFI_ERROR (Status)) {
- return Status;
+ goto Exit;
}
-
+ //
+ // Open the IO Abstraction(s) needed to perform the supported test
+ //
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiBlockIoProtocolGuid,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) &BlockIo,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
+ if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
+ Status = EFI_UNSUPPORTED;
+ }
+
+Exit:
+ //
+ // Close the I/O Abstraction(s) used to perform the supported test
+ //
+ if (DiskIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiDiskIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
+ if (BlockIo != NULL) {
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiBlockIoProtocolGuid,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
return Status;
}

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index c321d8c3d8..1ceb0d5cbb 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -34,6 +34,41 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
// this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
// references and add some Ext4SignalCorruption function + function call.

+/**
+ Checks only superblock magic value.
+
+ @param[in] DiskIo Pointer to the DiskIo.
+ @param[in] BlockIo Pointer to the BlockIo.
+
+ @return TRUE if a valid ext4 superblock, else FALSE.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+ IN EFI_DISK_IO_PROTOCOL *DiskIo,
+ IN EFI_BLOCK_IO_PROTOCOL *BlockIo
+ )
+{
+ UINT16 Magic;
+ EFI_STATUS Status;
+
+ Status = DiskIo->ReadDisk (
+ DiskIo,
+ BlockIo->Media->MediaId,
+ EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
+ sizeof (Magic),
+ &Magic
+ );
+ if (EFI_ERROR (Status)) {
+ return FALSE;
+ }
+
+ if (Magic != EXT4_SIGNATURE) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
/**
Does brief validation of the ext4 superblock.

--
2.17.1


[PATCH v2 0/2] ExtPkg Updates

Jeff Brasen
 

I have been using the new Ext4Pkg and been pretty successful and it is solving a use case we had.

Had a couple updates to propose

1. Changed the implementation of the binding protocol to both check if the driver is already bound
to the partition as well as added a really quick check to validate the magic value in supported.
This improves performance when you have a large number of non-ext4 partitions on the system.

2. As we are planning on using this for boot support we want to support unclean filesystem states in
case the user doesn't reset cleanly. I added a check if the recovery journal is present and if so treat
the filesystem as read-only (I know the driver is only RO at this point, but figured if you added write
support prior to recovery journal support we would want that). With this everything seems to work great.
I can add this under a FeaturePcd if desired as well.

Change log

v2 - Minor code review comments
v1 - Initial revision

Jeff Brasen (2):
Ext4Pkg: Improve Ext4IsBindingSupported() behavior
Ext4Pkg: Support uncleanly unmounted filesystems

Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++------
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 46 +++++++++++++++++++--
3 files changed, 102 insertions(+), 16 deletions(-)

--
2.17.1


Re: [PATCH v7 0/9] Ovmf: Disable the TPM2 platform hierarchy

Yao, Jiewen
 

According to the security policy, PP request must be processed before EndOfDxe.

May I know when you trigger PP request?

Thank you
Yao Jiewen

-----Original Message-----
From: Stefan Berger <stefanb@...>
Sent: Friday, September 10, 2021 10:25 PM
To: devel@edk2.groups.io; stefanb@...
Cc: mhaeuser@...; spbrogan@...;
marcandre.lureau@...; kraxel@...; Yao, Jiewen
<jiewen.yao@...>
Subject: Re: [edk2-devel] [PATCH v7 0/9] Ovmf: Disable the TPM2 platform
hierarchy


On 9/9/21 1:35 PM, Stefan Berger wrote:
This series imports code from the edk2-platforms project related to
disabling the TPM2 platform hierarchy in Ovmf. It addresses the Ovmf
aspects of the following bugs:

https://bugzilla.tianocore.org/show_bug.cgi?id=3510
https://bugzilla.tianocore.org/show_bug.cgi?id=3499

I have patched the .dsc files and successfully test-built with most of
them. Some I could not build because they failed for other reasons
unrelated to this series.

I tested the changes with QEMU on x86 following the build of
OvmfPkgX64.dsc.

Neither one of the following commands should work anymore on first
try when run on Linux:

With IBM tss2 tools:
tsshierarchychangeauth -hi p -pwdn newpass

With Intel tss2 tools:
tpm2_changeauth -c platform newpass

While disabling the platform hierarchy works, the unfortunate problem is
now that the signal to disable the TPM 2 platform hierarchy is received
before handling the physical presence interface (PPI) opcodes, which is
bad because some of the opcodes will not go through. The question now is
what is wrong? Are the PPI opcodes handled too late or the signal is
sent to early or is it the wrong signal?

Event = EfiCreateProtocolNotifyEvent (
            &gEfiDxeSmmReadyToLockProtocolGuid,
            TPL_CALLBACK,
            SmmReadyToLockEventCallBack,
            NULL,
            &Registration
            );

   Stefan


Regards,
Stefan

v7:
- Ditched ARM support in this series
- Using Tcg2PlatformDxe and Tcg2PlaformPei from edk2-platforms now
and revised most of the patches

v6:
- Removed unnecessary entries in .dsc files
- Added support for S3 resume failure case
- Assigned unique FILE_GUID to NULL implementation

v5:
- Modified patch 1 copies the code from edk2-platforms
- Modified patch 2 fixes bugs in the code
- Modified patch 4 introduces required PCD

v4:
- Fixed and simplified code imported from edk2-platforms

v3:
- Referencing Null implementation on Bhyve and Xen platforms
- Add support in Arm


Stefan Berger (9):
SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
edk2-platforms
SecurityPkg/TPM: Fix bugs in imported PeiDxeTpmPlatformHierarchyLib
SecrutiyPkg/Tcg: Import Tcg2PlatformDxe from edk2-platforms
SecurityPkg/Tcg: Make Tcg2PlatformDxe buildable
SecurityPkg: Introduce new PCD PcdRandomizePlatformHierarchy
OvmfPkg: Reference new Tcg2PlatformDxe in the build system for
compilation
SecurityPkg/Tcg: Import Tcg2PlatformPei from edk2-platforms
SecurityPkg/Tcg: Make Tcg2PlatformPei buildable
OvmfPkg: Reference new Tcg2PlatformPei in the build system

OvmfPkg/AmdSev/AmdSevX64.dsc | 8 +
OvmfPkg/AmdSev/AmdSevX64.fdf | 2 +
OvmfPkg/OvmfPkgIa32.dsc | 8 +
OvmfPkg/OvmfPkgIa32.fdf | 2 +
OvmfPkg/OvmfPkgIa32X64.dsc | 8 +
OvmfPkg/OvmfPkgIa32X64.fdf | 2 +
OvmfPkg/OvmfPkgX64.dsc | 8 +
OvmfPkg/OvmfPkgX64.fdf | 2 +
.../Include/Library/TpmPlatformHierarchyLib.h | 27 ++
.../PeiDxeTpmPlatformHierarchyLib.c | 255 ++++++++++++++++++
.../PeiDxeTpmPlatformHierarchyLib.inf | 44 +++
SecurityPkg/SecurityPkg.dec | 6 +
.../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.c | 85 ++++++
.../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf | 43 +++
.../Tcg/Tcg2PlatformPei/Tcg2PlatformPei.c | 107 ++++++++
.../Tcg/Tcg2PlatformPei/Tcg2PlatformPei.inf | 51 ++++
16 files changed, 658 insertions(+)
create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
create mode 100644
SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
chyLib.c
create mode 100644
SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
chyLib.inf
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.c
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformPei/Tcg2PlatformPei.c
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformPei/Tcg2PlatformPei.inf


Re: [PATCH v7 0/9] Ovmf: Disable the TPM2 platform hierarchy

Stefan Berger
 

On 9/9/21 1:35 PM, Stefan Berger wrote:
This series imports code from the edk2-platforms project related to
disabling the TPM2 platform hierarchy in Ovmf. It addresses the Ovmf
aspects of the following bugs:

https://bugzilla.tianocore.org/show_bug.cgi?id=3510
https://bugzilla.tianocore.org/show_bug.cgi?id=3499

I have patched the .dsc files and successfully test-built with most of
them. Some I could not build because they failed for other reasons
unrelated to this series.

I tested the changes with QEMU on x86 following the build of
OvmfPkgX64.dsc.

Neither one of the following commands should work anymore on first
try when run on Linux:

With IBM tss2 tools:
tsshierarchychangeauth -hi p -pwdn newpass

With Intel tss2 tools:
tpm2_changeauth -c platform newpass

While disabling the platform hierarchy works, the unfortunate problem is now that the signal to disable the TPM 2 platform hierarchy is received before handling the physical presence interface (PPI) opcodes, which is bad because some of the opcodes will not go through. The question now is what is wrong? Are the PPI opcodes handled too late or the signal is sent to early or is it the wrong signal?

Event = EfiCreateProtocolNotifyEvent (
            &gEfiDxeSmmReadyToLockProtocolGuid,
            TPL_CALLBACK,
            SmmReadyToLockEventCallBack,
            NULL,
            &Registration
            );

   Stefan


Regards,
Stefan

v7:
- Ditched ARM support in this series
- Using Tcg2PlatformDxe and Tcg2PlaformPei from edk2-platforms now
and revised most of the patches

v6:
- Removed unnecessary entries in .dsc files
- Added support for S3 resume failure case
- Assigned unique FILE_GUID to NULL implementation

v5:
- Modified patch 1 copies the code from edk2-platforms
- Modified patch 2 fixes bugs in the code
- Modified patch 4 introduces required PCD

v4:
- Fixed and simplified code imported from edk2-platforms

v3:
- Referencing Null implementation on Bhyve and Xen platforms
- Add support in Arm


Stefan Berger (9):
SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
edk2-platforms
SecurityPkg/TPM: Fix bugs in imported PeiDxeTpmPlatformHierarchyLib
SecrutiyPkg/Tcg: Import Tcg2PlatformDxe from edk2-platforms
SecurityPkg/Tcg: Make Tcg2PlatformDxe buildable
SecurityPkg: Introduce new PCD PcdRandomizePlatformHierarchy
OvmfPkg: Reference new Tcg2PlatformDxe in the build system for
compilation
SecurityPkg/Tcg: Import Tcg2PlatformPei from edk2-platforms
SecurityPkg/Tcg: Make Tcg2PlatformPei buildable
OvmfPkg: Reference new Tcg2PlatformPei in the build system

OvmfPkg/AmdSev/AmdSevX64.dsc | 8 +
OvmfPkg/AmdSev/AmdSevX64.fdf | 2 +
OvmfPkg/OvmfPkgIa32.dsc | 8 +
OvmfPkg/OvmfPkgIa32.fdf | 2 +
OvmfPkg/OvmfPkgIa32X64.dsc | 8 +
OvmfPkg/OvmfPkgIa32X64.fdf | 2 +
OvmfPkg/OvmfPkgX64.dsc | 8 +
OvmfPkg/OvmfPkgX64.fdf | 2 +
.../Include/Library/TpmPlatformHierarchyLib.h | 27 ++
.../PeiDxeTpmPlatformHierarchyLib.c | 255 ++++++++++++++++++
.../PeiDxeTpmPlatformHierarchyLib.inf | 44 +++
SecurityPkg/SecurityPkg.dec | 6 +
.../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.c | 85 ++++++
.../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf | 43 +++
.../Tcg/Tcg2PlatformPei/Tcg2PlatformPei.c | 107 ++++++++
.../Tcg/Tcg2PlatformPei/Tcg2PlatformPei.inf | 51 ++++
16 files changed, 658 insertions(+)
create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.c
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformPei/Tcg2PlatformPei.c
create mode 100644 SecurityPkg/Tcg/Tcg2PlatformPei/Tcg2PlatformPei.inf


Re: [RFC] RISC-V QEMU virtual package

Ni, Ray
 

I asked similar question to Mike who initially set up the CI.
The answer was: it's ok to pull a edk2-platform code in CI process to verify edk2 code change.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif Lindholm
Sent: Friday, September 10, 2021 5:55 PM
To: Chang, Abner <abner.chang@...>
Cc: devel@edk2.groups.io; kraxel@...; Yao, Jiewen <jiewen.yao@...>; gaoliming <gaoliming@...>;
'Ard Biesheuvel' <ard.biesheuvel@...>; Kinney, Michael D <michael.d.kinney@...>; Ni, Ray <ray.ni@...>;
Schaefer, Daniel <daniel.schaefer@...>; 'Sunil V L' <sunilvl@...>; 'Ard Biesheuvel'
<ardb+tianocore@...>
Subject: Re: [edk2-devel] [RFC] RISC-V QEMU virtual package

On Fri, Sep 10, 2021 at 00:08:12 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
Move it to OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMMIO.{c,inf} ?
The naming sounds good to me.

Another question,
Can CI build the package with dependency of edk2-platform? Currently
RiscVPkg in under edk2-platform and the modules provided by RiscVPkg
are referred by RiscVVirPkg.
Ideally not.

I think this serves as a reminder that RISC-V/ProcessorPkg should move
over to edk2.

/
Leif




Re: [PATCH] UefiPayloadPkg: Add script to build UniversalPayload in UefiPayloadPkg

Ni, Ray
 

+ Bob for general python language level review.
more comments embedded.

+def BuildUniversalPayload(args, MacroList):
+ build_target = args.Target
+ tool_chain_tag = args.ToolChain
+ elf_tool_chain = 'CLANGDWARF'
+ entry_module_inf = os.path.join("UefiPayloadPkg", "UefiPayloadEntry", "UniversalPayloadEntry.inf")
1. Lots of hardcode file/directory names here.
Can we write in below way?
entry_module_inf = "UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.inf"
entry_module_inf = os.path.normpath(entry_module_inf)

+ fv = os.path.join(os.environ['WORKSPACE'], "Build", "UefiPayloadPkgX64", "%s_%s"%(build_target, tool_chain_tag), "FV",
"DXEFV.Fv")
+ entry = os.path.join(os.environ['WORKSPACE'], "Build", "UefiPayloadPkgX64", "%s_%s"%(build_target, elf_tool_chain), "X64",
"UefiPayloadPkg", "UefiPayloadEntry", "UniversalPayloadEntry", "DEBUG", "UniversalPayloadEntry.dll")
2.
build_directory = os.path.join(os.environ['WORKSPACE'], os.path.normpath ("Build/UefiPayloadPkgX64")
fv = os.path.join(build_directory, "%s_%s"%(build_target, tool_chain_tag), os.path.normpath("FV/DXEFV.fv"))
entry = ......

+ dsc_patch = os.path.join("UefiPayloadPkg", "UefiPayloadPkg.dsc")
3.
Similar change can be applied to dsc_patch (typo? dsc_path?)


+ macro_list = " "
+ for key in MacroList:
+ macro_list +="-D {0}={1} ".format(key, MacroList[key])
4. you can set macro_list = "", then insert space before each "-D" in the loop.

+
+ #
+ # Building DXE core and DXE drivers as DXEFV.
+ #
+ build_payload = "build -p %s -b %s -a X64 -t %s -y UplBuildReport.txt"%(dsc_patch, build_target, tool_chain_tag)
+ build_payload += macro_list
5. you could write in following way
build_payload = f"build -p {dsc_patch} ..."

+ RunCommand(build_payload)
+ #
+ # Building Universal Payload entry.
+ #
+ build_module = "build -p %s -b %s -a X64 -m %s -t %s -y UplBuildReportEntry.txt"%(dsc_patch, build_target,
entry_module_inf, elf_tool_chain)
6. can you please make sure that there is no file generated in the workspace directory. All files are generated under build directory including the report file and the ELF file.

+ shutil.copy (entry, os.path.join(os.environ['WORKSPACE'], 'UPL.elf'))
7. do you copy out.

+
+def main():
+ print ("Begin to build Universal Payload...")
8. above message is not needed when printing help. maybe just remove the above message completely.

+ parser = argparse.ArgumentParser(description='For build Universal Payload')
+ parser.add_argument('-t', '--ToolChain', help="Using the ToolChain to build the UniversalPayload")
+ parser.add_argument('-b', '--Target', help="Using the TARGET to build the UniversalPayload")
9. set TARGET to DEBUG as default?

11881 - 11900 of 92312