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

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

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

Hi Mike,

All I did was copy the existing error handling in the scenario where we are unable to set up the buffer for any reason:

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c#L191

I agree that seems a little odd, but I haven’t investigated the ramifications of what this unhappy path is from an overall capsule flow perspective. Regardless of that… I feel like figuring that out and making the capsule flow fail completely either in the case of a memory allocation failure or a SetVariable() failure should probably be a different patch series. Don’t let perfect be the enemy of good.

Thanks,
Nate

From: Kinney, Michael D <michael.d.kinney@intel.com>
Date: Thursday, November 3, 2022 at 2:23 PM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>, devel@edk2.groups.io <devel@edk2.groups.io>, Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Gao, Liming <gaoliming@byosoft.com.cn>, Jiang, Guomin <Guomin.Jiang@intel.com>, Wang, Jian J <jian.j.wang@intel.com>
Subject: RE: [PATCH V4] MdeModulePkg: Memory Corruption Error in CapsuleRuntimeDxe
Also...would it be a simpler policy to fail the capsule update all together if any of the
3 allocations fail?  That way, there is no case where is "may fail".

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, November 3, 2022 2:21 PM
> To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Jiang, Guomin <guomin.jiang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: RE: [PATCH V4] MdeModulePkg: Memory Corruption Error in CapsuleRuntimeDxe
>
> 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

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