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 Date: Thursday, November 3, 2022 at 2:23 PM To: Desimone, Nathaniel L , devel@edk2.groups.io , Kinney, Michael D Cc: Gao, Liming , Jiang, Guomin , Wang, Jian J 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 > Sent: Thursday, November 3, 2022 2:21 PM > To: Desimone, Nathaniel L ; devel@edk2.groups.io; Kinney, Michael D > Cc: Gao, Liming ; Jiang, Guomin ; Wang, Jian J > 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 > > Sent: Tuesday, October 25, 2022 3:30 PM > > To: devel@edk2.groups.io > > Cc: Gao, Liming ; Jiang, Guomin ; Wang, Jian J ; > > Kinney, Michael D > > 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 > > Cc: Guomin Jiang > > Cc: Jian J Wang > > Cc: Michael D Kinney > > Signed-off-by: Nate DeSimone > > --- > > .../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