| Message ID | 20250217162659.151232-1-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] Avoid crash calling PrintErrMesg from efi_multiboot2 | expand |
On 17.02.2025 17:26, Frediano Ziglio wrote: > --- 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) Do source files other than boot.c really need this? Do any other files outside of efi/ maybe also need this (iirc this point was made along with the v5 comment you got)? Jan
On 17/02/2025 4:31 pm, Jan Beulich wrote: > On 17.02.2025 17:26, Frediano Ziglio wrote: >> --- 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) > Do source files other than boot.c really need this? Do any other files outside > of efi/ maybe also need this (iirc this point was made along with the v5 comment > you got)? Every TU reachable from efi_multiboot2() needs this, because all of those have logic executed at an incorrect virtual address. ~Andrew
On Mon, Feb 17, 2025 at 4:41 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 17/02/2025 4:31 pm, Jan Beulich wrote: > > On 17.02.2025 17:26, Frediano Ziglio wrote: > >> --- 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) > > Do source files other than boot.c really need this? Do any other files outside > > of efi/ maybe also need this (iirc this point was made along with the v5 comment > > you got)? > > Every TU reachable from efi_multiboot2() needs this, because all of > those have logic executed at an incorrect virtual address. > > ~Andrew I assume all the files in efi directory will deal with EFI boot so it's good to have the option enabled for the files inside the directory. The only exception seems runtime.c file. About external dependencies not sure how to detect them (beside looking at all symbols). Frediano
On 17.02.2025 17:52, Frediano Ziglio wrote: > On Mon, Feb 17, 2025 at 4:41 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> On 17/02/2025 4:31 pm, Jan Beulich wrote: >>> On 17.02.2025 17:26, Frediano Ziglio wrote: >>>> --- 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) >>> Do source files other than boot.c really need this? Do any other files outside >>> of efi/ maybe also need this (iirc this point was made along with the v5 comment >>> you got)? >> >> Every TU reachable from efi_multiboot2() needs this, because all of >> those have logic executed at an incorrect virtual address. >> >> ~Andrew > > I assume all the files in efi directory will deal with EFI boot so > it's good to have the option enabled for the files inside the > directory. The only exception seems runtime.c file. And compat.c, being a wrapper around runtime.c. > About external dependencies not sure how to detect them (beside > looking at all symbols). Which raises the question whether we don't need to turn on that option globally, without (as we have it now) any condition. Jan
On Mon, Feb 17, 2025 at 4:56 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 17.02.2025 17:52, Frediano Ziglio wrote: > > On Mon, Feb 17, 2025 at 4:41 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> > >> On 17/02/2025 4:31 pm, Jan Beulich wrote: > >>> On 17.02.2025 17:26, Frediano Ziglio wrote: > >>>> --- 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) > >>> Do source files other than boot.c really need this? Do any other files outside > >>> of efi/ maybe also need this (iirc this point was made along with the v5 comment > >>> you got)? > >> > >> Every TU reachable from efi_multiboot2() needs this, because all of > >> those have logic executed at an incorrect virtual address. > >> > >> ~Andrew > > > > I assume all the files in efi directory will deal with EFI boot so > > it's good to have the option enabled for the files inside the > > directory. The only exception seems runtime.c file. > > And compat.c, being a wrapper around runtime.c. > > > About external dependencies not sure how to detect them (beside > > looking at all symbols). > > Which raises the question whether we don't need to turn on that option > globally, without (as we have it now) any condition. > > Jan Are you saying enabling that option for all Xen? That potentially would decrease performances, we have a lot of switches in the code. Frediano
On 18/02/2025 12:05 pm, Frediano Ziglio wrote: > On Mon, Feb 17, 2025 at 4:56 PM Jan Beulich <jbeulich@suse.com> wrote: >> On 17.02.2025 17:52, Frediano Ziglio wrote: >>> On Mon, Feb 17, 2025 at 4:41 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>> On 17/02/2025 4:31 pm, Jan Beulich wrote: >>>>> On 17.02.2025 17:26, Frediano Ziglio wrote: >>>>>> --- 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) >>>>> Do source files other than boot.c really need this? Do any other files outside >>>>> of efi/ maybe also need this (iirc this point was made along with the v5 comment >>>>> you got)? >>>> Every TU reachable from efi_multiboot2() needs this, because all of >>>> those have logic executed at an incorrect virtual address. >>>> >>>> ~Andrew >>> I assume all the files in efi directory will deal with EFI boot so >>> it's good to have the option enabled for the files inside the >>> directory. The only exception seems runtime.c file. >> And compat.c, being a wrapper around runtime.c. >> >>> About external dependencies not sure how to detect them (beside >>> looking at all symbols). >> Which raises the question whether we don't need to turn on that option >> globally, without (as we have it now) any condition. >> >> Jan > Are you saying enabling that option for all Xen? That potentially > would decrease performances, we have a lot of switches in the code. -fno-switch-tables is active by default whenever INDIRECT_THUNK is enabled, and when CET-IBT is enabled to work around a GCC bug/misfeature. With speculation protections active, indirect branches are *far* slower than alternative ways of expressing a switch statement. ~Andrew
On Mon, Feb 17, 2025 at 4:56 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 17.02.2025 17:52, Frediano Ziglio wrote: > > On Mon, Feb 17, 2025 at 4:41 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> > >> On 17/02/2025 4:31 pm, Jan Beulich wrote: > >>> On 17.02.2025 17:26, Frediano Ziglio wrote: > >>>> --- 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) > >>> Do source files other than boot.c really need this? Do any other files outside > >>> of efi/ maybe also need this (iirc this point was made along with the v5 comment > >>> you got)? > >> > >> Every TU reachable from efi_multiboot2() needs this, because all of > >> those have logic executed at an incorrect virtual address. > >> > >> ~Andrew > > > > I assume all the files in efi directory will deal with EFI boot so > > it's good to have the option enabled for the files inside the > > directory. The only exception seems runtime.c file. > > And compat.c, being a wrapper around runtime.c. > > > About external dependencies not sure how to detect them (beside > > looking at all symbols). > > Which raises the question whether we don't need to turn on that option > globally, without (as we have it now) any condition. > > Jan I would avoid adding a potential option that could affect performances so badly at the moment. These changes are pretty contained. I would merge this patch and check any external dependencies as a follow up. Frediano
On 19.02.2025 17:34, Frediano Ziglio wrote: > On Mon, Feb 17, 2025 at 4:56 PM Jan Beulich <jbeulich@suse.com> wrote: >> On 17.02.2025 17:52, Frediano Ziglio wrote: >>> On Mon, Feb 17, 2025 at 4:41 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>> On 17/02/2025 4:31 pm, Jan Beulich wrote: >>>>> On 17.02.2025 17:26, Frediano Ziglio wrote: >>>>>> --- 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) >>>>> Do source files other than boot.c really need this? Do any other files outside >>>>> of efi/ maybe also need this (iirc this point was made along with the v5 comment >>>>> you got)? >>>> >>>> Every TU reachable from efi_multiboot2() needs this, because all of >>>> those have logic executed at an incorrect virtual address. >>> >>> I assume all the files in efi directory will deal with EFI boot so >>> it's good to have the option enabled for the files inside the >>> directory. The only exception seems runtime.c file. >> >> And compat.c, being a wrapper around runtime.c. >> >>> About external dependencies not sure how to detect them (beside >>> looking at all symbols). >> >> Which raises the question whether we don't need to turn on that option >> globally, without (as we have it now) any condition. > > I would avoid adding a potential option that could affect performances > so badly at the moment. > These changes are pretty contained. > I would merge this patch and check any external dependencies as a follow up. Well. It's a judgement call to the maintainers. If I were them, I'd demand that Andrew's remark be addressed, one way or another. Jan
On Thu, Feb 20, 2025 at 7:32 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 19.02.2025 17:34, Frediano Ziglio wrote: > > On Mon, Feb 17, 2025 at 4:56 PM Jan Beulich <jbeulich@suse.com> wrote: > >> On 17.02.2025 17:52, Frediano Ziglio wrote: > >>> On Mon, Feb 17, 2025 at 4:41 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >>>> On 17/02/2025 4:31 pm, Jan Beulich wrote: > >>>>> On 17.02.2025 17:26, Frediano Ziglio wrote: > >>>>>> --- 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) > >>>>> Do source files other than boot.c really need this? Do any other files outside > >>>>> of efi/ maybe also need this (iirc this point was made along with the v5 comment > >>>>> you got)? > >>>> > >>>> Every TU reachable from efi_multiboot2() needs this, because all of > >>>> those have logic executed at an incorrect virtual address. > >>> > >>> I assume all the files in efi directory will deal with EFI boot so > >>> it's good to have the option enabled for the files inside the > >>> directory. The only exception seems runtime.c file. > >> > >> And compat.c, being a wrapper around runtime.c. > >> > >>> About external dependencies not sure how to detect them (beside > >>> looking at all symbols). > >> > >> Which raises the question whether we don't need to turn on that option > >> globally, without (as we have it now) any condition. > > > > I would avoid adding a potential option that could affect performances > > so badly at the moment. > > These changes are pretty contained. > > I would merge this patch and check any external dependencies as a follow up. > > Well. It's a judgement call to the maintainers. If I were them, I'd demand > that Andrew's remark be addressed, one way or another. > > Jan I think I did, but only Andres can confirm it. Frediano
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)
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@cloud.com> --- 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(-)