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:
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