Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Oct 15, 2020 at 3:00 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 14.10.2020 18:27, Jason Andryuk wrote:
> > On Wed, Oct 14, 2020 at 12:02 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 14.10.2020 17:31, Jason Andryuk wrote:
> >>> Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
> >>> kernel build CONFIG_PVH=y CONFIG_PV=n lacks the note.  In this case,
> >>> virt_entry will be UNSET_ADDR, overwritten by the ELF header e_entry,
> >>> and fail the check against the virt address range.
> >
> > Oh, these should be CONFIG_XEN_PVH=y and CONFIG_XEN_PV=n
> >
> >>> Change the code to only check virt_entry against the virtual address
> >>> range if it was set upon entry to the function.
> >>
> >> Not checking at all seems wrong to me. The ELF spec anyway says
> >> "virtual address", so an out of bounds value is at least suspicious.
> >>
> >>> Maybe the overwriting of virt_entry could be removed, but I don't know
> >>> if there would be unintended consequences where (old?) kernels don't
> >>> have an elfnote, but do have an in-range e_entry?  The failing kernel I
> >>> just looked at has an e_entry of 0x1000000.
> >>
> >> And if you dropped the overwriting, what entry point would we use
> >> in the absence of an ELF note?
> >
> > elf_xen_note_check currently has:
> >
> >     /* PVH only requires one ELF note to be set */
> >     if ( parms->phys_entry != UNSET_ADDR32 )
> >     {
> >         elf_msg(elf, "ELF: Found PVH image\n");
> >         return 0;
> >     }
> >
> >> I'd rather put up the option of adjusting the entry (or the check),
> >> if it looks like a valid physical address.
> >
> > The function doesn't know if the image will be booted PV or PVH, so I
> > guess we do all the checks, but use 'parms->phys_entry != UNSET_ADDR32
> > && parms->virt_entry == UNSET_ADDR' to conditionally skip checking
> > virt?
>
> Like Jürgen, the purpose of the patch hadn't become clear to me
> from reading the description. As I understand it now, we're currently
> refusing to boot such a kernel for no reason. If that's correct,
> perhaps you could say so in the description in a more direct way?

Yes, sorry I didn't state it clearly.  You are correct, libxc fails
with "xc_dom_find_loader: no loader found" for a linux kernel with
PHYS32_ENTRY but without ENTRY.

> As far as actual code adjustments go - how much of
> elf_xen_addr_calc_check() is actually applicable when booting PVH?

I don't know...

> And why is there no bounds check of ->phys_entry paralleling the
> ->virt_entry one?

What is the purpose of this checking?  It's sanity checking which is
generally good, but what is the harm from failing the checks?  A
corrupt kernel can crash itself?  Maybe you could start executing
something (the initramfs?) instead of the actual kernel?

> On the whole, as long as we don't know what mode we're planning to
> boot in, we can't skip any checks, as the mere presence of
> XEN_ELFNOTE_PHYS32_ENTRY doesn't mean that's also what gets used.
> Therefore simply bypassing any of the checks is not an option.

elf_xen_note_check() early exits when it finds phys_entry set, so
there is already some bypassing.

> In
> particular what you suggest would lead to failure to check
> e_entry-derived ->virt_entry when the PVH-specific note is
> present but we're booting in PV mode. For now I don't see how to
> address this without making the function aware of the intended
> booting mode.

Yes, the relevant checks depend on the desired booting mode.

The e_entry use seems a little problematic.  You said the ELF
Specification states it should be a virtual address, but Linux seems
to fill it with a physical address.  You could use a heuristic e_entry
< 0 (0xffff...) to compare with the virtual addresses otherwise check
against physical?

Regards,
Jason