Re: [edk2-devel] [PATCH v3 3/4] StandaloneMmPkg/Core: Fix issue that section address might be wrong

["Laszlo Ersek" <lersek@redhat.com>]

On 10/30/23 08:49, Wei6 Xu wrote:
> MmCoreFfsFindMmDriver() assumes FileHeader is EFI_FFS_FILE_HEADER.
> 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.
> 
> 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 | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
> index 9d0ce66ef839..fa335d62c252 100644
> --- a/StandaloneMmPkg/Core/FwVol.c
> +++ b/StandaloneMmPkg/Core/FwVol.c
> @@ -116,23 +116,21 @@ MmCoreFfsFindMmDriver (
>        break;
>      }
>  
> -    Status = FfsFindSectionData (
> +    Status = FfsFindSection (
>                 EFI_SECTION_GUID_DEFINED,
>                 FileHeader,
> -               &SectionData,
> -               &SectionDataSize
> +               &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;
>      }

(1) Can you remove the SectionData and SectionDataSize variables as
well? I think they are unused at this point.

With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Ah wait, you're going to use those variables in the next patch, again.
OK then. Just take my R-b for this patch.


(2) Now that I'm looking at the code in more depth, I don't even
understand what the original intent of the FfsFindSectionData() call was!

The output values SectionData and SectionDataSize were not used for
anything!

So it seems like FfsFindSectionData() was called just to make sure an
EFI_SECTION_GUID_DEFINED section *existed*. And then we'd treat the very
*first* section after the file header -- not too robustly identified, at
that -- as a GUIDed section, for extracting its info.

So this patch actually fixes two warts: one, the file header size is now
considered more generally, two, we don't just assume that the very first
section is the GUID-defined section, but look it up. Phew.

Laszlo



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