[edk2-devel] [PATCH V4] MdeModulePkg: Memory Corruption Error in CapsuleRuntimeDxe

[Michael D Kinney]

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 at intel.com>
> Sent: Tuesday, October 25, 2022 3:30 PM
> To: devel at edk2.groups.io
> Cc: Gao, Liming <gaoliming at byosoft.com.cn>; Jiang, Guomin <guomin.jiang at intel.com>; Wang, Jian J <jian.j.wang at intel.com>;
> Kinney, Michael D <michael.d.kinney at 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 at byosoft.com.cn>
> Cc: Guomin Jiang <guomin.jiang at intel.com>
> Cc: Jian J Wang <jian.j.wang at intel.com>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Signed-off-by: Nate DeSimone <nathaniel.l.desimone at 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#95903): https://edk2.groups.io/g/devel/message/95903
Mute This Topic: https://groups.io/mt/94569829/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-