Re: [PATCH v2] libelf: improve PVH elfnote parsing

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, May 21, 2021 at 07:26:21AM +0200, Juergen Gross wrote:
> On 20.05.21 11:28, Jan Beulich wrote:
> > On 20.05.2021 11:27, Roger Pau Monné wrote:
> > > On Wed, May 19, 2021 at 12:34:19PM +0200, Jan Beulich wrote:
> > > > On 18.05.2021 16:47, Roger Pau Monne wrote:
> > > > > @@ -425,8 +425,11 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
> > > > >           return -1;
> > > > >       }
> > > > > -    /* Initial guess for virt_base is 0 if it is not explicitly defined. */
> > > > > -    if ( parms->virt_base == UNSET_ADDR )
> > > > > +    /*
> > > > > +     * Initial guess for virt_base is 0 if it is not explicitly defined in the
> > > > > +     * PV case. For PVH virt_base is forced to 0 because paging is disabled.
> > > > > +     */
> > > > > +    if ( parms->virt_base == UNSET_ADDR || hvm )
> > > > >       {
> > > > >           parms->virt_base = 0;
> > > > >           elf_msg(elf, "ELF: VIRT_BASE unset, using %#" PRIx64 "\n",
> > > > 
> > > > This message is wrong now if hvm is true but parms->virt_base != UNSET_ADDR.
> > > > Best perhaps is to avoid emitting the message altogether when hvm is true.
> > > > (Since you'll be touching it anyway, perhaps a good opportunity
> > > > to do
> away
> > > > with passing parms->virt_base to elf_msg(), and instead simply use a literal
> > > > zero.)
> > > > 
> > > > > @@ -441,8 +444,10 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
> > > > >        *
> > > > >        * If we are using the modern ELF notes interface then the default
> > > > >        * is 0.
> > > > > +     *
> > > > > +     * For PVH this is forced to 0, as it's already a
> > > > > legacy option
> for PV.
> > > > >        */
> > > > > -    if ( parms->elf_paddr_offset == UNSET_ADDR )
> > > > > +    if ( parms->elf_paddr_offset == UNSET_ADDR || hvm )
> > > > >       {
> > > > >           if ( parms->elf_note_start )
> > > > 
> > > > Don't you want "|| hvm" here as well, or alternatively suppress the
> > > > fallback to the __xen_guest section in the PVH case (near the end of
> > > > elf_xen_parse())?
> > > 
> > > The legacy __xen_guest section doesn't support PHYS32_ENTRY, so yes,
> > > that part could be completely skipped when called from an HVM
> > > container.
> > > 
> > > I think I will fix that in another patch though if you are OK, as
> > > it's not strictly related to the calculation fixes done here.
> > 
> > That's fine; it wants to be a prereq to the one here then, though,
> > I think.
> 
> Would it be possible to add some comment to xen/include/public/elfnote.h
> Indicating which elfnotes are evaluated for which guest types,

For PVH after this patch the only mandatory note should be
PHYS32_ENTRY. BSD_SYMTAB is also parsed if found on that case.

> including
> a hint which elfnotes _have_ been evaluated before this series?

Before this patch mostly all PV notes are parsed, and you have to
manage to get suitable values set so that:

    if ( (parms->virt_kstart > parms->virt_kend) ||
         (parms->virt_entry < parms->virt_kstart) ||
         (parms->virt_entry > parms->virt_kend) ||
         (parms->virt_base > parms->virt_kstart) )
    {
        elf_err(elf, "ERROR: ELF start or entries are out of bounds\n");
        return -1;
    }

Doesn't trigger. That can be done by setting virt_entry or the native
elf entry point to a value that satisfies the condition. Or by setting
a suitable offset in VIRT_BASE to a value that matches the offset
added to the native entry point in the ELF header.

I can try to write this up, albeit I think it can get messy. Not sure
what's the best approach here regarding recommended settings to
satisfy old Xen versions.

Roger.