Re: [edk2-devel] [PATCH v2 1/1] ArmPkg: DefaultExceptionHandlerLib: Do Not Allocate\r Memory

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 2 Aug 2023 at 20:59, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> Changes since v1:
> - Move OutputString write to after serial logging
> - Drop sending "Recursive Exception" to ConOut
>
> If gST->ConOut is available when Arm's DefaultExceptionHandler
> is running, AsciiPrint will get called to attempt to print to
> ConOut, in addition to the serial output.
>
> AsciiPrint calls AsciiInternalPrint in UefiLibPrint.c which
> in turn calls AllocatePool to allocate a buffer to convert
> the Ascii input string to a Unicode string to pass to
> ConOut->OutputString.
>
> Per the comment on DefaultExceptionHandler, we should not be
> allocating memory in the exception handler, as this can cause
> the exception handler to fail if we had a memory exception or
> the system state is such that we cannot allocate memory.
>
> It has been observed on ArmVirtQemu that exceptions generated
> in the memory handling code will fail to output the stack dump
> and CPU state that is critical to debugging because the
> AllocatePool will fail.
>
> This patch fixes the Arm and AARCH64 DefaultExceptionHandlers to
> not allocate memory when ConOut is available and instead use
> stack memory to convert the Ascii string needed for SerialPortWrite
> to the Unicode string needed for ConOut->OutputString. Correspondingly,
> ArmVirtQemu can now output the stack dump and CPU state when hitting
> an exception in memory code.
>
> GitHub PR: https://github.com/tianocore/edk2/pull/4713
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
>
> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>

Merged as #4717

Thanks!

> ---
>  ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 30 +++++++++++++-------
>  ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c     | 22 +++++++++++---
>  2 files changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> index f2bca5d74005..f4967dabed14 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> @@ -22,6 +22,11 @@
>  #include <Protocol/DebugSupport.h>
>  #include <Protocol/LoadedImage.h>
>
> +//
> +// Maximum number of characters to print to serial (UINT8s) and to console if available (as UINT16s)
> +//
> +#define MAX_PRINT_CHARS  100
> +
>  STATIC CHAR8  *gExceptionTypeString[] = {
>    "Synchronous",
>    "IRQ",
> @@ -188,18 +193,14 @@ DefaultExceptionHandler (
>    IN OUT EFI_SYSTEM_CONTEXT  SystemContext
>    )
>  {
> -  CHAR8  Buffer[100];
> -  UINTN  CharCount;
> -  INT32  Offset;
> +  CHAR8   Buffer[MAX_PRINT_CHARS];
> +  CHAR16  UnicodeBuffer[MAX_PRINT_CHARS];
> +  UINTN   CharCount;
> +  INT32   Offset;
>
>    if (mRecursiveException) {
>      STATIC CHAR8 CONST  Message[] = "\nRecursive exception occurred while dumping the CPU state\n";
> -
>      SerialPortWrite ((UINT8 *)Message, sizeof Message - 1);
> -    if (gST->ConOut != NULL) {
> -      AsciiPrint (Message);
> -    }
> -
>      CpuDeadLoop ();
>    }
>
> @@ -207,9 +208,9 @@ DefaultExceptionHandler (
>
>    CharCount = AsciiSPrint (Buffer, sizeof (Buffer), "\n\n%a Exception at 0x%016lx\n", gExceptionTypeString[ExceptionType], SystemContext.SystemContextAArch64->ELR);
>    SerialPortWrite ((UINT8 *)Buffer, CharCount);
> -  if (gST->ConOut != NULL) {
> -    AsciiPrint (Buffer);
> -  }
> +
> +  // prepare a unicode buffer for ConOut, if applicable, in case the buffer gets reused
> +  UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
>
>    DEBUG_CODE_BEGIN ();
>    CHAR8   *Pdb, *PrevPdb;
> @@ -330,6 +331,13 @@ DefaultExceptionHandler (
>        ));
>    }
>
> +  // attempt to print that we had a synchronous exception to ConOut
> +  // we do this after the serial logging as ConOut's logging is more
> +  // complex and we aren't guaranteed to succeed
> +  if (gST->ConOut != NULL) {
> +    gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
> +  }
> +
>    ASSERT (FALSE);
>    CpuDeadLoop ();
>  }
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> index 13b321e45615..f5fce4c4b516 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> @@ -23,6 +23,11 @@
>  #include <Protocol/DebugSupport.h>
>  #include <Library/DefaultExceptionHandlerLib.h>
>
> +//
> +// Maximum number of characters to print to serial (UINT8s) and to console if available (as UINT16s)
> +//
> +#define MAX_PRINT_CHARS  100
> +
>  //
>  // The number of elements in a CHAR8 array, including the terminating NUL, that
>  // is meant to hold the string rendering of the CPSR.
> @@ -198,7 +203,8 @@ DefaultExceptionHandler (
>    IN OUT EFI_SYSTEM_CONTEXT  SystemContext
>    )
>  {
> -  CHAR8    Buffer[100];
> +  CHAR8    Buffer[MAX_PRINT_CHARS];
> +  CHAR16   UnicodeBuffer[MAX_PRINT_CHARS];
>    UINTN    CharCount;
>    UINT32   DfsrStatus;
>    UINT32   IfsrStatus;
> @@ -216,9 +222,9 @@ DefaultExceptionHandler (
>                  SystemContext.SystemContextArm->CPSR
>                  );
>    SerialPortWrite ((UINT8 *)Buffer, CharCount);
> -  if (gST->ConOut != NULL) {
> -    AsciiPrint (Buffer);
> -  }
> +
> +  // prepare a unicode buffer for ConOut, if applicable, as Buffer is used below
> +  UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
>
>    DEBUG_CODE_BEGIN ();
>    CHAR8   *Pdb;
> @@ -289,6 +295,14 @@ DefaultExceptionHandler (
>    }
>
>    DEBUG ((DEBUG_ERROR, "\n"));
> +
> +  // attempt to print that we had a synchronous exception to ConOut
> +  // we do this after the serial logging as ConOut's logging is more
> +  // complex and we aren't guaranteed to succeed
> +  if (gST->ConOut != NULL) {
> +    gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
> +  }
> +
>    ASSERT (FALSE);
>
>    CpuDeadLoop ();   // may return if executing under a debugger
> --
> 2.40.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107548): https://edk2.groups.io/g/devel/message/107548
Mute This Topic: https://groups.io/mt/100512229/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-