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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 15, 2020 at 09:00:09AM +0200, Jan Beulich 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?
> 
> As far as actual code adjustments go - how much of
> elf_xen_addr_calc_check() is actually applicable when booting PVH?

I think the only relevant check for PVH would be the symtab loading
(XEN_ELFNOTE_BSD_SYMTAB).

> And why is there no bounds check of ->phys_entry paralleling the
> ->virt_entry one?
> 
> 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. 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.

That seems the only viable approach. Maybe an intended mode field could
be added to elf_dom_parms in order to signal this?

Thanks, Roger.