Re: [PATCH V4] MdeModulePkg: Memory Corruption Error in CapsuleRuntimeDxe

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Nate,

The "may fail" messages look a bit odd.  Is this due to the fact that CapsuleRuntimeDxe is in X64 mode,
but this module does not know if PEI Phase will process the capsule in IA32 or X64 execution mode?

We have a PCD that is set if the DXE IPL needs to switch modes.  Can we use that information?

These "may fail" messages will only be generated if there is enough memory to allocate the capsule
image, but not the page tables and/or stack.  Correct?

Thanks,

Mike

> -----Original Message-----
> From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
> Sent: Tuesday, October 25, 2022 3:30 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin <guomin.jiang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH V4] MdeModulePkg: Memory Corruption Error in CapsuleRuntimeDxe
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4112
> 
> In AllocateReservedMemoryBelow4G(), if gBS->AllocatePages()
> returns an error, and ASSERTs are disabled, then the
> function will overwrite memory from 0xFFFFFFFF -> (0xFFFFFFFF + Size).
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
> ---
>  .../X64/SaveLongModeContext.c                 | 25 ++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c
> index dab297dd0a..a8c5de8764 100644
> --- a/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c
> +++ b/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c
> @@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>    @param  Size      Size of memory to allocate.
> 
>    @return Allocated Address for output.
> +  @return NULL - Memory allocation failed.
> 
>  **/
>  VOID *
> @@ -59,7 +60,15 @@ AllocateReservedMemoryBelow4G (
>                    Pages,
>                    &Address
>                    );
> -  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "ERROR AllocateReservedMemoryBelow4G(): %r\n", Status));
> +    return NULL;
> +  }
> +
> +  if (Address == 0) {
> +    DEBUG ((DEBUG_ERROR, "ERROR AllocateReservedMemoryBelow4G(): AllocatePages() returned NULL"));
> +    return NULL;
> +  }
> 
>    Buffer = (VOID *)(UINTN)Address;
>    ZeroMem (Buffer, Size);
> @@ -159,14 +168,23 @@ PrepareContextForCapsulePei (
>    DEBUG ((DEBUG_INFO, "CapsuleRuntimeDxe X64 TotalPagesNum - 0x%x pages\n", TotalPagesNum));
> 
>    LongModeBuffer.PageTableAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateReservedMemoryBelow4G (EFI_PAGES_TO_SIZE
> (TotalPagesNum));
> -  ASSERT (LongModeBuffer.PageTableAddress != 0);
> +  if (LongModeBuffer.PageTableAddress == 0) {
> +    DEBUG ((DEBUG_ERROR, "FATAL ERROR: CapsuleLongModeBuffer cannot be saved, "));
> +    DEBUG ((DEBUG_ERROR, "PageTableAddress allocation failed. Capsule in PEI may fail!\n"));
> +    return;
> +  }
> 
>    //
>    // Allocate stack
>    //
>    LongModeBuffer.StackSize        = PcdGet32 (PcdCapsulePeiLongModeStackSize);
>    LongModeBuffer.StackBaseAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateReservedMemoryBelow4G (PcdGet32
> (PcdCapsulePeiLongModeStackSize));
> -  ASSERT (LongModeBuffer.StackBaseAddress != 0);
> +  if (LongModeBuffer.StackBaseAddress == 0) {
> +    DEBUG ((DEBUG_ERROR, "FATAL ERROR: CapsuleLongModeBuffer cannot be saved, "));
> +    DEBUG ((DEBUG_ERROR, "StackBaseAddress allocation failed. Capsule in PEI may fail!\n"));
> +    gBS->FreePages (LongModeBuffer.PageTableAddress, TotalPagesNum);
> +    return;
> +  }
> 
>    Status = gRT->SetVariable (
>                    EFI_CAPSULE_LONG_MODE_BUFFER_NAME,
> @@ -189,6 +207,7 @@ PrepareContextForCapsulePei (
>        );
>    } else {
>      DEBUG ((DEBUG_ERROR, "FATAL ERROR: CapsuleLongModeBuffer cannot be saved: %r. Capsule in PEI may fail!\n", Status));
> +    gBS->FreePages (LongModeBuffer.PageTableAddress, TotalPagesNum);
>      gBS->FreePages (LongModeBuffer.StackBaseAddress, EFI_SIZE_TO_PAGES (LongModeBuffer.StackSize));
>    }
>  }
> --
> 2.27.0.windows.1