Re: [PATCH v6] Avoid crash calling PrintErrMesg from efi_multiboot2

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Feb 24, 2025 at 12:57:13PM +0000, Frediano Ziglio wrote:
> On Fri, Feb 21, 2025 at 8:20 PM Marek Marczykowski-Górecki
> <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > 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.
> >
> 
> Did you see the correct message in both cases?
> Did you use Grub or direct EFI?
> 
> With Grub and without this patch you won't see the message, with grub
> with the patch you see the correct message.

I did use grub, and I didn't see the message indeed.
But in the case it was supposed to crash (with added PrintErrMesg(),
commented out blexit and without your patch) it did _not_ crashed and
continued to normal boot. Is that #PF non-fatal here?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature