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

From: Jan Beulich <jbeulich@suse.com>
To: Juergen Gross <jgross@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH] xen: consider alloc-only segments when loading PV dom0 kernel
Date: Thu, 23 Jun 2022 11:04:11 +0200	[thread overview]
Message-ID: <c5961627-1719-dd54-bbcf-c08a826ba14d@suse.com> (raw)
In-Reply-To: <20220623080208.2214-1-jgross@suse.com>

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@suse.com>
> ---
>  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
misleading, as you really mean sections there, not segments. Afaik
there's no concept of "alloc" for segments, which are what program
headers describe.)

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.

Finally, you changing libelf but referring to only Dom0 in the title
looks inconsistent to me.

Jan