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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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.

Jan