|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6] Avoid crash calling PrintErrMesg from efi_multiboot2
On Mon, Feb 17, 2025 at 04:26:59PM +0000, Frediano Ziglio wrote:
> Although code is compiled with -fpic option data is not position
> independent. This causes data pointer to become invalid if
> code is not relocated properly which is what happens for
> efi_multiboot2 which is called by multiboot entry code.
>
> Code tested adding
> PrintErrMesg(L"Test message", EFI_BUFFER_TOO_SMALL);
> in efi_multiboot2 before calling efi_arch_edd (this function
> can potentially call PrintErrMesg).
>
> Before the patch (XenServer installation on Qemu, xen replaced
> with vanilla xen.gz):
> Booting `XenServer (Serial)'Booting `XenServer (Serial)'
> Test message: !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID -
> 00000000 !!!!
> ExceptionData - 0000000000000000 I:0 R:0 U:0 W:0 P:0 PK:0 SS:0 SGX:0
> RIP - 000000007EE21E9A, CS - 0000000000000038, RFLAGS - 0000000000210246
> RAX - 000000007FF0C1B5, RCX - 0000000000000050, RDX - 0000000000000010
> RBX - 0000000000000000, RSP - 000000007FF0C180, RBP - 000000007FF0C210
> RSI - FFFF82D040467CE8, RDI - 0000000000000000
> R8 - 000000007FF0C1C8, R9 - 000000007FF0C1C0, R10 - 0000000000000000
> R11 - 0000000000001020, R12 - FFFF82D040467CE8, R13 - 000000007FF0C1B8
> R14 - 000000007EA33328, R15 - 000000007EA332D8
> DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030
> GS - 0000000000000030, SS - 0000000000000030
> CR0 - 0000000080010033, CR2 - FFFF82D040467CE8, CR3 - 000000007FC01000
> CR4 - 0000000000000668, CR8 - 0000000000000000
> DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> GDTR - 000000007F9DB000 0000000000000047, LDTR - 0000000000000000
> IDTR - 000000007F48E018 0000000000000FFF, TR - 0000000000000000
> FXSAVE_STATE - 000000007FF0BDE0
> !!!! Find image based on IP(0x7EE21E9A) (No PDB)
> (ImageBase=000000007EE20000, EntryPoint=000000007EE23935) !!!!
>
> After the patch:
> Booting `XenServer (Serial)'Booting `XenServer (Serial)'
> Test message: Buffer too small
> BdsDxe: loading Boot0000 "UiApp" from
> Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331)
> BdsDxe: starting Boot0000 "UiApp" from
> Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331)
>
> This partially rollback commit 00d5d5ce23e6.
>
> Fixes: 9180f5365524 ("x86: add multiboot2 protocol support for EFI platforms")
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
I tried testing this patch, but it seems I cannot reproduce the original
failure...
I did as the commit message suggests here:
https://gitlab.com/xen-project/people/marmarek/xen/-/commit/ca3d6911c448eb886990f33d4380b5646617a982
With blexit() in PrintErrMesg(), it went back to the bootloader, so I'm
sure this code path was reached. But with blexit() commented out, Xen
started correctly both with and without this patch... The branch I used
is here:
https://gitlab.com/xen-project/people/marmarek/xen/-/commits/automation-tests?ref_type=heads
Are there some extra condition to reproduce the issue? Maybe it depends
on the compiler version? I guess I can try also on QEMU, but based on
the description, I would expect it to crash in any case.
> ---
> Changes since v1:
> - added "Fixes:" tag;
> - fixed cast style change.
>
> Changes since v2:
> - wrap long line.
>
> Changes since v3:
> - fixed "Fixes:" tag.
>
> Changes since v4:
> - use switch instead of tables.
>
> Changes since v5:
> - added -fno-jump-tables option.
> ---
> xen/common/efi/boot.c | 58 ++++++++++++++++++++++++------------
> xen/common/efi/efi-common.mk | 1 +
> 2 files changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index efbec00af9..143b5681ba 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -287,33 +287,53 @@ static bool __init match_guid(const EFI_GUID *guid1,
> const EFI_GUID *guid2)
> /* generic routine for printing error messages */
> static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
> {
> - static const CHAR16* const ErrCodeToStr[] __initconstrel = {
> - [~EFI_ERROR_MASK & EFI_NOT_FOUND] = L"Not found",
> - [~EFI_ERROR_MASK & EFI_NO_MEDIA] = L"The device has no
> media",
> - [~EFI_ERROR_MASK & EFI_MEDIA_CHANGED] = L"Media changed",
> - [~EFI_ERROR_MASK & EFI_DEVICE_ERROR] = L"Device error",
> - [~EFI_ERROR_MASK & EFI_VOLUME_CORRUPTED] = L"Volume corrupted",
> - [~EFI_ERROR_MASK & EFI_ACCESS_DENIED] = L"Access denied",
> - [~EFI_ERROR_MASK & EFI_OUT_OF_RESOURCES] = L"Out of resources",
> - [~EFI_ERROR_MASK & EFI_VOLUME_FULL] = L"Volume is full",
> - [~EFI_ERROR_MASK & EFI_SECURITY_VIOLATION] = L"Security violation",
> - [~EFI_ERROR_MASK & EFI_CRC_ERROR] = L"CRC error",
> - [~EFI_ERROR_MASK & EFI_COMPROMISED_DATA] = L"Compromised data",
> - [~EFI_ERROR_MASK & EFI_BUFFER_TOO_SMALL] = L"Buffer too small",
> - };
> - EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK;
> -
> StdOut = StdErr;
> PrintErr(mesg);
> PrintErr(L": ");
>
> - if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] )
> - mesg = ErrCodeToStr[ErrIdx];
> - else
> + switch (ErrCode)
> {
> + case EFI_NOT_FOUND:
> + mesg = L"Not found";
> + break;
> + case EFI_NO_MEDIA:
> + mesg = L"The device has no media";
> + break;
> + case EFI_MEDIA_CHANGED:
> + mesg = L"Media changed";
> + break;
> + case EFI_DEVICE_ERROR:
> + mesg = L"Device error";
> + break;
> + case EFI_VOLUME_CORRUPTED:
> + mesg = L"Volume corrupted";
> + break;
> + case EFI_ACCESS_DENIED:
> + mesg = L"Access denied";
> + break;
> + case EFI_OUT_OF_RESOURCES:
> + mesg = L"Out of resources";
> + break;
> + case EFI_VOLUME_FULL:
> + mesg = L"Volume is full";
> + break;
> + case EFI_SECURITY_VIOLATION:
> + mesg = L"Security violation";
> + break;
> + case EFI_CRC_ERROR:
> + mesg = L"CRC error";
> + break;
> + case EFI_COMPROMISED_DATA:
> + mesg = L"Compromised data";
> + break;
> + case EFI_BUFFER_TOO_SMALL:
> + mesg = L"Buffer too small";
> + break;
> + default:
> PrintErr(L"ErrCode: ");
> DisplayUint(ErrCode, 0);
> mesg = NULL;
> + break;
> }
> blexit(mesg);
> }
> diff --git a/xen/common/efi/efi-common.mk b/xen/common/efi/efi-common.mk
> index 23cafcf20c..06b1c19674 100644
> --- a/xen/common/efi/efi-common.mk
> +++ b/xen/common/efi/efi-common.mk
> @@ -2,6 +2,7 @@ EFIOBJ-y := boot.init.o pe.init.o ebmalloc.o runtime.o
> EFIOBJ-$(CONFIG_COMPAT) += compat.o
>
> CFLAGS-y += -fshort-wchar
> +CFLAGS-y += -fno-jump-tables
> CFLAGS-y += -iquote $(srctree)/common/efi
> CFLAGS-y += -iquote $(srcdir)
>
> --
> 2.34.1
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Attachment:
signature.asc
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |