Re: [PATCH] xen: consider alloc-only segments when loading PV dom0 kernel

[Juergen Gross <jgross@xxxxxxxx>]

On 23.06.22 11:04, Jan Beulich wrote:
> On 23.06.2022 10:02, Juergen Gross wrote:
> > When loading the dom0 kernel for PV mode, the first free usable memory
> > location after the kernel needs to take segments into account, which
> > have only the ALLOC flag set, but are not specified to be loaded in
> > the program headers of the ELF file.
> >
> > This is e.g. a problem for Linux kernels from 5.19 onwards, as those
> > can have a final NOLOAD section at the end, which must not be used by
> > e.g. the start_info structure or the initial page tables allocated by
> > the hypervisor.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   xen/common/libelf/libelf-loader.c | 33 +++++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
> >
> > diff --git a/xen/common/libelf/libelf-loader.c 
> > b/xen/common/libelf/libelf-loader.c
> > index 629cc0d3e6..4b0e3ced55 100644
> > --- a/xen/common/libelf/libelf-loader.c
> > +++ b/xen/common/libelf/libelf-loader.c
> > @@ -467,7 +467,9 @@ do {                                                        
> >         \
> >   void elf_parse_binary(struct elf_binary *elf)
> >   {
> >       ELF_HANDLE_DECL(elf_phdr) phdr;
> > +    ELF_HANDLE_DECL(elf_shdr) shdr;
> >       uint64_t low = -1, high = 0, paddr, memsz;
> > +    uint64_t vlow = -1, vhigh = 0, vaddr, voff;
> >       unsigned i, count;
> >   
> >       count = elf_phdr_count(elf);
> > @@ -480,6 +482,7 @@ void elf_parse_binary(struct elf_binary *elf)
> >           if ( !elf_phdr_is_loadable(elf, phdr) )
> >               continue;
> >           paddr = elf_uval(elf, phdr, p_paddr);
> > +        vaddr = elf_uval(elf, phdr, p_vaddr);
> >           memsz = elf_uval(elf, phdr, p_memsz);
> >           elf_msg(elf, "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 "\n",
> >                   paddr, memsz);
> > @@ -487,7 +490,37 @@ void elf_parse_binary(struct elf_binary *elf)
> >               low = paddr;
> >           if ( high < paddr + memsz )
> >               high = paddr + memsz;
> > +        if ( vlow > vaddr )
> > +            vlow = vaddr;
> > +        if ( vhigh < vaddr + memsz )
> > +            vhigh = vaddr + memsz;
> >       }
> > +
> > +    voff = vhigh - high;
> > +
> > +    count = elf_shdr_count(elf);
> > +    for ( i = 0; i < count; i++ )
> > +    {
> > +        shdr = elf_shdr_by_index(elf, i);
> > +        if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
> > +            /* input has an insane section header count field */
> > +            break;
> > +        if ( !(elf_uval(elf, shdr, sh_flags) & SHF_ALLOC) )
> > +            continue;
> > +        vaddr = elf_uval(elf, shdr, sh_addr);
> > +        memsz = elf_uval(elf, shdr, sh_size);
> > +        if ( vlow > vaddr )
> > +        {
> > +            vlow = vaddr;
> > +            low = vaddr - voff;
> > +        }
> > +        if ( vhigh < vaddr + memsz )
> > +        {
> > +            vhigh = vaddr + memsz;
> > +            high = vaddr + memsz - voff;
> > +        }
> > +    }
> As said in the reply to your problem report: The set of PHDRs doesn't
> cover all sections. For loading one should never need to resort to
> parsing section headers - in a loadable binary it is no error if
> there's no section table in the first place. (The title is also
The problem isn't the loading, but the memory usage after doing the
loading. The hypervisor is placing page tables in a memory region
the kernel has other plans with.

> misleading, as you really mean sections there, not segments. Afaik
> there's no concept of "alloc" for segments, which are what program
> headers describe.)
Sorry, will reword.

> Also: Needing to fix this in the hypervisor would mean that Linux
> 5.19 and onwards cannot be booted on Xen without whichever fix
> backported.
Correct. See my reply to the reply you mentioned above.

> Finally, you changing libelf but referring to only Dom0 in the title
> looks inconsistent to me.
Hmm, yes. Will drop the dom0 aspect.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature