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

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

On Tue, 1 Aug 2023 at 00:04, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> 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/4703
>
> 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>

Thanks a lot for fixing this.

Is calling into gST->ConOut guaranteed to be safe in this regard? Or
is it still best effort?


> ---
>  ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 18 +++++++++++++-----
>  ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c     | 11 +++++++++--
>  2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> index f2bca5d74005..07c8aade1c5f 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,16 +193,18 @@ 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);
> +      UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
> +      gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
>      }
>
>      CpuDeadLoop ();
> @@ -208,7 +215,8 @@ 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);
> +    UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
> +    gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
>    }
>
>    DEBUG_CODE_BEGIN ();
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> index 13b321e45615..08d62c0dbfa2 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;
> @@ -217,7 +223,8 @@ DefaultExceptionHandler (
>                  );
>    SerialPortWrite ((UINT8 *)Buffer, CharCount);
>    if (gST->ConOut != NULL) {
> -    AsciiPrint (Buffer);
> +    UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
> +    gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
>    }
>
>    DEBUG_CODE_BEGIN ();
> --
> 2.40.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#107412): https://edk2.groups.io/g/devel/message/107412
> Mute This Topic: https://groups.io/mt/100472023/5717338
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org]
> ------------
>
>


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