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

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

[Oliver Smith-Denny]

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>
---
 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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Oliver Smith-Denny]

On 8/2/2023 9:06 AM, Ard Biesheuvel wrote:
> 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?
> 
> 

Yeah, this is something I worried about when fixing this. It is very
much best effort, because there are no guarantees that OutputString will
not allocate memory (or some other such unsafe operation in an exception
handler). With the ability to BYO ConOut stack and the complications
with graphics that can be involved here, I personally would be happy to
drop the ConOut call entirely and rely on the serial output.

In my testing, this did work on ArmVirtQemu and I can envision a case
where having the ConOut output would be nice, but I tend towards lets
keep the exception handler as simple as possible and make sure we get
the useful information dumped out.

Another option would be to keep this change but move the ConOut calls
to after all of the serial output, so that if we do get a recursive
exception in ConOut, at least we got our serial output.

Let me know what you think, happy to spin up a v2.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107460): https://edk2.groups.io/g/devel/message/107460
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

On Wed, 2 Aug 2023 at 18:21, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 8/2/2023 9:06 AM, Ard Biesheuvel wrote:
> > 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?
> >
> >
>
> Yeah, this is something I worried about when fixing this. It is very
> much best effort, because there are no guarantees that OutputString will
> not allocate memory (or some other such unsafe operation in an exception
> handler). With the ability to BYO ConOut stack and the complications
> with graphics that can be involved here, I personally would be happy to
> drop the ConOut call entirely and rely on the serial output.
>
> In my testing, this did work on ArmVirtQemu and I can envision a case
> where having the ConOut output would be nice, but I tend towards lets
> keep the exception handler as simple as possible and make sure we get
> the useful information dumped out.
>
> Another option would be to keep this change but move the ConOut calls
> to after all of the serial output, so that if we do get a recursive
> exception in ConOut, at least we got our serial output.
>
> Let me know what you think, happy to spin up a v2.
>

Yeah, that seems like a nice approach - it will still be best effort,
but it won't derail the more reliable serial console if it fails.

That probably also means we shouldn't bother printing the 'recursive
exception' to ConOut at all, or perhap have a special 'recursive
exception in conout' flag on top of the existing one but that might be
overkill.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107461): https://edk2.groups.io/g/devel/message/107461
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]
-=-=-=-=-=-=-=-=-=-=-=-