From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 696A3D80D40 for ; Wed, 2 Aug 2023 16:06:25 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=qH5m2jF4BkatjYRd8NotZnt0dHBU7zO/NdH5pB19Aig=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1690992384; v=1; b=InjwFVJQQ6S/8iLawSqKlBwKE1++orinkGpN7JYn54e++WCGBwEr+dlyaOlJMGnJ1+CY8HXD rjpDOCLfdJQnw3J1MKOLDef44FAlLpXdQ05kj1sb2mMLY1gjbRBocD75rZjPs5NAzCDzp502/Ro r2zoz+tA0bUWeZBQlOFNdiWM= X-Received: by 127.0.0.2 with SMTP id m8jtYY7687511xaseT98Y1TU; Wed, 02 Aug 2023 09:06:24 -0700 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.299.1690992383316210551 for ; Wed, 02 Aug 2023 09:06:23 -0700 X-Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id A17EF61A33 for ; Wed, 2 Aug 2023 16:06:22 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9536AC433CA for ; Wed, 2 Aug 2023 16:06:21 +0000 (UTC) X-Received: by mail-lf1-f48.google.com with SMTP id 2adb3069b0e04-4fb7dc16ff0so11631029e87.2 for ; Wed, 02 Aug 2023 09:06:21 -0700 (PDT) X-Gm-Message-State: pqp3LGPnnUJIcuRgRPCj4Wdrx7686176AA= X-Google-Smtp-Source: APBJJlFK3FtUhWCpjbTe5qhFqREnnNtnR7cvu4lnN1ud+R0/dvyeGqXI6WTM8EihZQwmc5vd+Av0J4W3IK87S1a3+Nc= X-Received: by 2002:ac2:4e06:0:b0:4fd:d6ba:73ba with SMTP id e6-20020ac24e06000000b004fdd6ba73bamr6476345lfr.37.1690992379547; Wed, 02 Aug 2023 09:06:19 -0700 (PDT) MIME-Version: 1.0 References: <20230731220428.23002-1-osde@linux.microsoft.com> In-Reply-To: <20230731220428.23002-1-osde@linux.microsoft.com> From: "Ard Biesheuvel" Date: Wed, 2 Aug 2023 18:06:08 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: DefaultExceptionHandlerLib: Do Not Allocate Memory To: devel@edk2.groups.io, osde@linux.microsoft.com Cc: Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Gerd Hoffmann Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=InjwFVJQ; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) On Tue, 1 Aug 2023 at 00:04, Oliver Smith-Denny 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 > Cc: Ard Biesheuvel > Cc: Sami Mujawar > Cc: Gerd Hoffmann > > Signed-off-by: Oliver Smith-Denny 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 > #include > > +// > +// 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 > #include > > +// > +// 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] -=-=-=-=-=-=-=-=-=-=-=-