| Message ID | 20240819125401.218931-1-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] Avoid crash calling PrintErrMesg from efi_multiboot2 | expand |
On 19.08.2024 14:54, 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 - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 > RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 > RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 > RSI - FFFF82D040467A88, RDI - 0000000000000000 > R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 > R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 > R14 - 000000007EFA1225, R15 - 000000007DAB45A8 > DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 > GS - 0000000000000030, SS - 0000000000000030 > CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 > CR4 - 0000000000000668, CR8 - 0000000000000000 > DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 > DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 > GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 > IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 > FXSAVE_STATE - 000000007EFA0E60 > !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! > > 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) > > Fixes: 00d5d5ce23e6 ("work around Clang generating .data.rel.ro section for init-only files") > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > --- > xen/common/efi/boot.c | 46 ++++++++++++++++++++++++++++++------------- > 1 file changed, 32 insertions(+), 14 deletions(-) > --- > Changes since v1: > - added "Fixes:" tag; > - fixed cast style change. > > Changes since v2: > - wrap long line. And what about the Fixes: tag? Jan
On Mon, Aug 19, 2024 at 2:19 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 19.08.2024 14:54, 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 - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 > > RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 > > RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 > > RSI - FFFF82D040467A88, RDI - 0000000000000000 > > R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 > > R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 > > R14 - 000000007EFA1225, R15 - 000000007DAB45A8 > > DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 > > GS - 0000000000000030, SS - 0000000000000030 > > CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 > > CR4 - 0000000000000668, CR8 - 0000000000000000 > > DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 > > DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 > > GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 > > IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 > > FXSAVE_STATE - 000000007EFA0E60 > > !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! > > > > 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) > > > > Fixes: 00d5d5ce23e6 ("work around Clang generating .data.rel.ro section for init-only files") ^^^^ here ?? > > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > > --- > > xen/common/efi/boot.c | 46 ++++++++++++++++++++++++++++++------------- > > 1 file changed, 32 insertions(+), 14 deletions(-) > > --- > > Changes since v1: > > - added "Fixes:" tag; > > - fixed cast style change. > > > > Changes since v2: > > - wrap long line. > > And what about the Fixes: tag? > > Jan
On 19.08.2024 16:02, Frediano Ziglio wrote: > On Mon, Aug 19, 2024 at 2:19 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 19.08.2024 14:54, 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 - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 >>> RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 >>> RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 >>> RSI - FFFF82D040467A88, RDI - 0000000000000000 >>> R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 >>> R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 >>> R14 - 000000007EFA1225, R15 - 000000007DAB45A8 >>> DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 >>> GS - 0000000000000030, SS - 0000000000000030 >>> CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 >>> CR4 - 0000000000000668, CR8 - 0000000000000000 >>> DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 >>> DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 >>> GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 >>> IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 >>> FXSAVE_STATE - 000000007EFA0E60 >>> !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! >>> >>> 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) >>> >>> Fixes: 00d5d5ce23e6 ("work around Clang generating .data.rel.ro section for init-only files") > > ^^^^ here ?? Did you not see ... >>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> >>> --- >>> xen/common/efi/boot.c | 46 ++++++++++++++++++++++++++++++------------- >>> 1 file changed, 32 insertions(+), 14 deletions(-) >>> --- >>> Changes since v1: >>> - added "Fixes:" tag; >>> - fixed cast style change. >>> >>> Changes since v2: >>> - wrap long line. >> >> And what about the Fixes: tag? ... my respective v2 comment then? There I said: "I don't think this is right. While this is where the array was introduced, it was correct at that time afaict. It went wrong when MB2 support was added about a year later. 9180f5365524 ("x86: add multiboot2 protocol support for EFI platforms") may be reasonable to blame, albeit I'm not sure that was the final one, after which MB2 support was considered complete." Jan
On Mon, Aug 19, 2024 at 3:14 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 19.08.2024 16:02, Frediano Ziglio wrote: > > On Mon, Aug 19, 2024 at 2:19 PM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 19.08.2024 14:54, 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 - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 > >>> RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 > >>> RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 > >>> RSI - FFFF82D040467A88, RDI - 0000000000000000 > >>> R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 > >>> R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 > >>> R14 - 000000007EFA1225, R15 - 000000007DAB45A8 > >>> DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 > >>> GS - 0000000000000030, SS - 0000000000000030 > >>> CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 > >>> CR4 - 0000000000000668, CR8 - 0000000000000000 > >>> DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 > >>> DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 > >>> GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 > >>> IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 > >>> FXSAVE_STATE - 000000007EFA0E60 > >>> !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! > >>> > >>> 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) > >>> > >>> Fixes: 00d5d5ce23e6 ("work around Clang generating .data.rel.ro section for init-only files") > > > > ^^^^ here ?? > > Did you not see ... > > >>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > >>> --- > >>> xen/common/efi/boot.c | 46 ++++++++++++++++++++++++++++++------------- > >>> 1 file changed, 32 insertions(+), 14 deletions(-) > >>> --- > >>> Changes since v1: > >>> - added "Fixes:" tag; > >>> - fixed cast style change. > >>> > >>> Changes since v2: > >>> - wrap long line. > >> > >> And what about the Fixes: tag? > > ... my respective v2 comment then? There I said: > > "I don't think this is right. While this is where the array was introduced, > it was correct at that time afaict. It went wrong when MB2 support was added > about a year later. 9180f5365524 ("x86: add multiboot2 protocol support for > EFI platforms") may be reasonable to blame, albeit I'm not sure that was the > final one, after which MB2 support was considered complete." > > Jan Yes, missed, updated Frediano
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index efbec00af9..fdbe75005c 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -287,19 +287,36 @@ 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", +#define ERROR_MESSAGE_LIST \ + ERROR_MESSAGE(EFI_NOT_FOUND, "Not found") \ + ERROR_MESSAGE(EFI_NO_MEDIA, "The device has no media") \ + ERROR_MESSAGE(EFI_MEDIA_CHANGED, "Media changed") \ + ERROR_MESSAGE(EFI_DEVICE_ERROR, "Device error") \ + ERROR_MESSAGE(EFI_VOLUME_CORRUPTED, "Volume corrupted") \ + ERROR_MESSAGE(EFI_ACCESS_DENIED, "Access denied") \ + ERROR_MESSAGE(EFI_OUT_OF_RESOURCES, "Out of resources") \ + ERROR_MESSAGE(EFI_VOLUME_FULL, "Volume is full") \ + ERROR_MESSAGE(EFI_SECURITY_VIOLATION, "Security violation") \ + ERROR_MESSAGE(EFI_CRC_ERROR, "CRC error") \ + ERROR_MESSAGE(EFI_COMPROMISED_DATA, "Compromised data") \ + ERROR_MESSAGE(EFI_BUFFER_TOO_SMALL, "Buffer too small") + + static const struct ErrorStrings { + CHAR16 start; +#undef ERROR_MESSAGE +#define ERROR_MESSAGE(code, str) CHAR16 msg_ ## code[sizeof(str)]; + ERROR_MESSAGE_LIST + } ErrorStrings __initconst = { + 0 +#undef ERROR_MESSAGE +#define ERROR_MESSAGE(code, str) , L ## str + ERROR_MESSAGE_LIST + }; + static const uint16_t ErrCodeToStr[] __initconst = { +#undef ERROR_MESSAGE +#define ERROR_MESSAGE(code, str) \ + [~EFI_ERROR_MASK & code] = offsetof(struct ErrorStrings, msg_ ## code), + ERROR_MESSAGE_LIST }; EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK; @@ -308,7 +325,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) PrintErr(L": "); if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] ) - mesg = ErrCodeToStr[ErrIdx]; + mesg = (const CHAR16 *)((const void *)&ErrorStrings + + ErrCodeToStr[ErrIdx]); else { PrintErr(L"ErrCode: ");
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 - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 RSI - FFFF82D040467A88, RDI - 0000000000000000 R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 R14 - 000000007EFA1225, R15 - 000000007DAB45A8 DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 GS - 0000000000000030, SS - 0000000000000030 CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 CR4 - 0000000000000668, CR8 - 0000000000000000 DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 FXSAVE_STATE - 000000007EFA0E60 !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! 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) Fixes: 00d5d5ce23e6 ("work around Clang generating .data.rel.ro section for init-only files") Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- xen/common/efi/boot.c | 46 ++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 14 deletions(-) --- Changes since v1: - added "Fixes:" tag; - fixed cast style change. Changes since v2: - wrap long line.