[edk2-devel] [PATCH v2 0/1] StandaloneMmCore finds drivers in uncompressed inner fv.

[edk2-devel] [PATCH v2 0/1] StandaloneMmCore finds drivers in uncompressed inner fv.

[Xu, Wei6]

V1:
This patch is to fix the issue that StandaloneMmCore fails to detect uncompressed inner FV.
PR: https://github.com/tianocore/edk2/pull/4943

V2:
Based on V1, fix some other issues
1. Add Missing object size checks before casting pointers to header types
  a. InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData; 
     This is introduced in V1, add the size check on SectionDataSize against EFI_FIRMWARE_VOLUME_HEADER
  b. Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
     Use FfsFindSection instead of FfsFindSectionData to avoid pointer casting.
2. Fix potential memory leak issue that ScratchBuffer is not freed when page allocation for DstBuffer fails.
PR: https://github.com/tianocore/edk2/pull/4965

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Ray Ni <ray.ni@intel.com>

Wei6 Xu (1):
  StandaloneMmPkg: Fix some issues in function MmCoreFfsFindMmDriver.

 StandaloneMmPkg/Core/FwVol.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110160): https://edk2.groups.io/g/devel/message/110160
Mute This Topic: https://groups.io/mt/102212657/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/1] StandaloneMmPkg: Fix some issues in function MmCoreFfsFindMmDriver.

[Xu, Wei6]

1. The MmCoreFfsFindMmDriver only checks for encapsulated compressed
FVs. When an inner FV is uncompressed, StandaloneMmCore will miss the
FV and all the MM drivers in the FV will not be dispatched. Add checks
for uncompressed inner FV to fix this issue.
2. If FileHeader is an EFI_FFS_FILE_HEADER2, 'FileHeader + 1' will get
a wrong section address. Use FfsFindSection to get the section directly,
instead of 'FileHeader + 1' to avoid this issue.
3. ScratchBuffer is not freed in the error return path that DstBuffer
page allocation fails. Free ScratchBuffer before return with error.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
---
 StandaloneMmPkg/Core/FwVol.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
index 1f6d7714ba97..fb483bd62696 100644
--- a/StandaloneMmPkg/Core/FwVol.c
+++ b/StandaloneMmPkg/Core/FwVol.c
@@ -104,23 +104,40 @@ MmCoreFfsFindMmDriver (
       break;
     }
 
+    //
+    // Check uncompressed firmware volumes
+    //
     Status = FfsFindSectionData (
-               EFI_SECTION_GUID_DEFINED,
+               EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
                FileHeader,
                &SectionData,
                &SectionDataSize
                );
+    if (!EFI_ERROR (Status)) {
+      if (SectionDataSize > sizeof (EFI_FIRMWARE_VOLUME_HEADER)) {
+        InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData;
+        MmCoreFfsFindMmDriver (InnerFvHeader);
+      }
+    }
+
+    //
+    // Check compressed firmware volumes
+    //
+    Status = FfsFindSection (
+               EFI_SECTION_GUID_DEFINED,
+               FileHeader,
+               &Section
+               );
     if (EFI_ERROR (Status)) {
       break;
     }
 
-    Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
-    Status  = ExtractGuidedSectionGetInfo (
-                Section,
-                &DstBufferSize,
-                &ScratchBufferSize,
-                &SectionAttribute
-                );
+    Status = ExtractGuidedSectionGetInfo (
+               Section,
+               &DstBufferSize,
+               &ScratchBufferSize,
+               &SectionAttribute
+               );
     if (EFI_ERROR (Status)) {
       break;
     }
@@ -138,6 +155,7 @@ MmCoreFfsFindMmDriver (
     //
     DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES (DstBufferSize));
     if (DstBuffer == NULL) {
+      FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES (ScratchBufferSize));
       return EFI_OUT_OF_RESOURCES;
     }
 
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110161): https://edk2.groups.io/g/devel/message/110161
Mute This Topic: https://groups.io/mt/102212658/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/1] StandaloneMmCore finds drivers in uncompressed inner fv.

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]

Wei,
Thanks for fixing the 3 issues.
Can you kindly separate the one patch to at least 2 patches?
One patch is to fix minor issues.
The other is to add support of nested uncompressed FV.

Thanks,
Ray
________________________________
From: Xu, Wei6 <wei6.xu@intel.com>
Sent: Friday, October 27, 2023 8:59 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Xu, Wei6 <wei6.xu@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
Subject: [PATCH v2 0/1] StandaloneMmCore finds drivers in uncompressed inner fv.

V1:
This patch is to fix the issue that StandaloneMmCore fails to detect uncompressed inner FV.
PR: https://github.com/tianocore/edk2/pull/4943

V2:
Based on V1, fix some other issues
1. Add Missing object size checks before casting pointers to header types
  a. InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData;
     This is introduced in V1, add the size check on SectionDataSize against EFI_FIRMWARE_VOLUME_HEADER
  b. Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
     Use FfsFindSection instead of FfsFindSectionData to avoid pointer casting.
2. Fix potential memory leak issue that ScratchBuffer is not freed when page allocation for DstBuffer fails.
PR: https://github.com/tianocore/edk2/pull/4965

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Ray Ni <ray.ni@intel.com>

Wei6 Xu (1):
  StandaloneMmPkg: Fix some issues in function MmCoreFfsFindMmDriver.

 StandaloneMmPkg/Core/FwVol.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

--
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110189): https://edk2.groups.io/g/devel/message/110189
Mute This Topic: https://groups.io/mt/102212657/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 4950 bytes --]

Re: [edk2-devel] [PATCH v2 0/1] StandaloneMmCore finds drivers in uncompressed inner fv.

[Laszlo Ersek]

On 10/27/23 07:49, Ni, Ray wrote:
> Wei,
> Thanks for fixing the 3 issues.
> Can you kindly separate the one patch to at least 2 patches?
> One patch is to fix minor issues.
> The other is to add support of nested uncompressed FV.

Yes please!

I'd even prefer a separate patch per individual issue fix (especially if
you count the recursion limiting too).

Thanks!
Laszlo

> 
> Thanks,
> Ray
> ------------------------------------------------------------------------
> *From:* Xu, Wei6 <wei6.xu@intel.com>
> *Sent:* Friday, October 27, 2023 8:59 AM
> *To:* devel@edk2.groups.io <devel@edk2.groups.io>
> *Cc:* Xu, Wei6 <wei6.xu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
> <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
> *Subject:* [PATCH v2 0/1] StandaloneMmCore finds drivers in uncompressed
> inner fv.
>  
> V1:
> This patch is to fix the issue that StandaloneMmCore fails to detect
> uncompressed inner FV.
> PR: https://github.com/tianocore/edk2/pull/4943
> <https://github.com/tianocore/edk2/pull/4943>
> 
> V2:
> Based on V1, fix some other issues
> 1. Add Missing object size checks before casting pointers to header types
>   a. InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData;
>      This is introduced in V1, add the size check on SectionDataSize
> against EFI_FIRMWARE_VOLUME_HEADER
>   b. Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
>      Use FfsFindSection instead of FfsFindSectionData to avoid pointer
> casting.
> 2. Fix potential memory leak issue that ScratchBuffer is not freed when
> page allocation for DstBuffer fails.
> PR: https://github.com/tianocore/edk2/pull/4965
> <https://github.com/tianocore/edk2/pull/4965>
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Ray Ni <ray.ni@intel.com>
> 
> Wei6 Xu (1):
>   StandaloneMmPkg: Fix some issues in function MmCoreFfsFindMmDriver.
> 
>  StandaloneMmPkg/Core/FwVol.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> -- 
> 2.29.2.windows.2
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110241): https://edk2.groups.io/g/devel/message/110241
Mute This Topic: https://groups.io/mt/102212657/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/1] StandaloneMmCore finds drivers in uncompressed inner fv.

[Xu, Wei6]

Hi Laszlo and Ray,

Thanks a lot for the review. Patch V3 is sent out to:
1. Separate patch per individual issue fix
2. Limit FwVol encapsulation section recursion in MmCoreFfsFindMmDriver().
(https://edk2.groups.io/g/devel/message/110296)

Could you please help to review it again? Thanks a lot!

BR,
Wei


-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Saturday, October 28, 2023 7:08 PM
To: Ni, Ray <ray.ni@intel.com>; Xu, Wei6 <wei6.xu@intel.com>; devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>
Subject: Re: [PATCH v2 0/1] StandaloneMmCore finds drivers in uncompressed inner fv.

On 10/27/23 07:49, Ni, Ray wrote:
> Wei,
> Thanks for fixing the 3 issues.
> Can you kindly separate the one patch to at least 2 patches?
> One patch is to fix minor issues.
> The other is to add support of nested uncompressed FV.

Yes please!

I'd even prefer a separate patch per individual issue fix (especially if you count the recursion limiting too).

Thanks!
Laszlo

> 
> Thanks,
> Ray
> ----------------------------------------------------------------------
> --
> *From:* Xu, Wei6 <wei6.xu@intel.com>
> *Sent:* Friday, October 27, 2023 8:59 AM
> *To:* devel@edk2.groups.io <devel@edk2.groups.io>
> *Cc:* Xu, Wei6 <wei6.xu@intel.com>; Laszlo Ersek <lersek@redhat.com>; 
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar 
> <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
> *Subject:* [PATCH v2 0/1] StandaloneMmCore finds drivers in 
> uncompressed inner fv.
>  
> V1:
> This patch is to fix the issue that StandaloneMmCore fails to detect 
> uncompressed inner FV.
> PR: https://github.com/tianocore/edk2/pull/4943
> <https://github.com/tianocore/edk2/pull/4943>
> 
> V2:
> Based on V1, fix some other issues
> 1. Add Missing object size checks before casting pointers to header 
> types
>   a. InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData;
>      This is introduced in V1, add the size check on SectionDataSize 
> against EFI_FIRMWARE_VOLUME_HEADER
>   b. Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
>      Use FfsFindSection instead of FfsFindSectionData to avoid pointer 
> casting.
> 2. Fix potential memory leak issue that ScratchBuffer is not freed 
> when page allocation for DstBuffer fails.
> PR: https://github.com/tianocore/edk2/pull/4965
> <https://github.com/tianocore/edk2/pull/4965>
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Ray Ni <ray.ni@intel.com>
> 
> Wei6 Xu (1):
>   StandaloneMmPkg: Fix some issues in function MmCoreFfsFindMmDriver.
> 
>  StandaloneMmPkg/Core/FwVol.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> --
> 2.29.2.windows.2
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110301): https://edk2.groups.io/g/devel/message/110301
Mute This Topic: https://groups.io/mt/102212657/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-