[PATCH v2] libelf: improve PVH elfnote parsing

[PATCH v2] libelf: improve PVH elfnote parsing

[Roger Pau Monne]

Pass an hvm boolean parameter to the elf note parsing and checking
routines, so that better checking can be done in case libelf is
dealing with an hvm container.

elf_xen_note_check shouldn't return early unless PHYS32_ENTRY is set
and the container is of type HVM, or else the loader and version
checks would be avoided for kernels intended to be booted as PV but
that also have PHYS32_ENTRY set.

Adjust elf_xen_addr_calc_check so that the virtual addresses are
actually physical ones (by setting virt_base and elf_paddr_offset to
zero) when the container is of type HVM, as that container is always
started with paging disabled.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Expand comments.
 - Do not set virt_entry to phys_entry unless it's an HVM container.
---
 tools/fuzz/libelf/libelf-fuzzer.c   |  3 ++-
 tools/libs/guest/xg_dom_elfloader.c |  6 ++++--
 tools/libs/guest/xg_dom_hvmloader.c |  2 +-
 xen/arch/x86/hvm/dom0_build.c       |  2 +-
 xen/arch/x86/pv/dom0_build.c        |  2 +-
 xen/common/libelf/libelf-dominfo.c  | 32 +++++++++++++++++++----------
 xen/include/xen/libelf.h            |  2 +-
 7 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/tools/fuzz/libelf/libelf-fuzzer.c b/tools/fuzz/libelf/libelf-fuzzer.c
index 1ba85717114..84fb84720fa 100644
--- a/tools/fuzz/libelf/libelf-fuzzer.c
+++ b/tools/fuzz/libelf/libelf-fuzzer.c
@@ -17,7 +17,8 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
         return -1;
 
     elf_parse_binary(elf);
-    elf_xen_parse(elf, &parms);
+    elf_xen_parse(elf, &parms, false);
+    elf_xen_parse(elf, &parms, true);
 
     return 0;
 }
diff --git a/tools/libs/guest/xg_dom_elfloader.c b/tools/libs/guest/xg_dom_elfloader.c
index 06e713fe111..ad71163dd92 100644
--- a/tools/libs/guest/xg_dom_elfloader.c
+++ b/tools/libs/guest/xg_dom_elfloader.c
@@ -135,7 +135,8 @@ static elf_negerrnoval xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
      * or else we might be trying to load a plain ELF.
      */
     elf_parse_binary(&elf);
-    rc = elf_xen_parse(&elf, dom->parms);
+    rc = elf_xen_parse(&elf, dom->parms,
+                       dom->container_type == XC_DOM_HVM_CONTAINER);
     if ( rc != 0 )
         return rc;
 
@@ -166,7 +167,8 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
 
     /* parse binary and get xen meta info */
     elf_parse_binary(elf);
-    if ( elf_xen_parse(elf, dom->parms) != 0 )
+    if ( elf_xen_parse(elf, dom->parms,
+                       dom->container_type == XC_DOM_HVM_CONTAINER) != 0 )
     {
         rc = -EINVAL;
         goto out;
diff --git a/tools/libs/guest/xg_dom_hvmloader.c b/tools/libs/guest/xg_dom_hvmloader.c
index ec6ebad7fd5..3a63b23ba39 100644
--- a/tools/libs/guest/xg_dom_hvmloader.c
+++ b/tools/libs/guest/xg_dom_hvmloader.c
@@ -73,7 +73,7 @@ static elf_negerrnoval xc_dom_probe_hvm_kernel(struct xc_dom_image *dom)
      * else we might be trying to load a PV kernel.
      */
     elf_parse_binary(&elf);
-    rc = elf_xen_parse(&elf, dom->parms);
+    rc = elf_xen_parse(&elf, dom->parms, true);
     if ( rc == 0 )
         return -EINVAL;
 
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 878dc1d808e..c24b9efdb0a 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -561,7 +561,7 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
     elf_set_verbose(&elf);
 #endif
     elf_parse_binary(&elf);
-    if ( (rc = elf_xen_parse(&elf, &parms)) != 0 )
+    if ( (rc = elf_xen_parse(&elf, &parms, true)) != 0 )
     {
         printk("Unable to parse kernel for ELFNOTES\n");
         return rc;
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index e0801a9e6d1..af47615b226 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -353,7 +353,7 @@ int __init dom0_construct_pv(struct domain *d,
         elf_set_verbose(&elf);
 
     elf_parse_binary(&elf);
-    if ( (rc = elf_xen_parse(&elf, &parms)) != 0 )
+    if ( (rc = elf_xen_parse(&elf, &parms, false)) != 0 )
         goto out;
 
     /* compatibility check */
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 69c94b6f3bb..5ad2832aa75 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -360,7 +360,7 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
 /* sanity checks                                                            */
 
 static elf_errorstatus elf_xen_note_check(struct elf_binary *elf,
-                              struct elf_dom_parms *parms)
+                              struct elf_dom_parms *parms, bool hvm)
 {
     if ( (ELF_PTRVAL_INVALID(parms->elf_note_start)) &&
          (ELF_PTRVAL_INVALID(parms->guest_info)) )
@@ -382,7 +382,7 @@ static elf_errorstatus elf_xen_note_check(struct elf_binary *elf,
     }
 
     /* PVH only requires one ELF note to be set */
-    if ( parms->phys_entry != UNSET_ADDR32 )
+    if ( parms->phys_entry != UNSET_ADDR32 && hvm )
     {
         elf_msg(elf, "ELF: Found PVH image\n");
         return 0;
@@ -414,7 +414,7 @@ static elf_errorstatus elf_xen_note_check(struct elf_binary *elf,
 }
 
 static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
-                                   struct elf_dom_parms *parms)
+                                   struct elf_dom_parms *parms, bool hvm)
 {
     uint64_t virt_offset;
 
@@ -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",
@@ -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 )
             parms->elf_paddr_offset = 0;
@@ -456,8 +461,13 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
     parms->virt_kstart = elf->pstart + virt_offset;
     parms->virt_kend   = elf->pend   + virt_offset;
 
-    if ( parms->virt_entry == UNSET_ADDR )
-        parms->virt_entry = elf_uval(elf, elf->ehdr, e_entry);
+    if ( parms->virt_entry == UNSET_ADDR || hvm )
+    {
+        if ( parms->phys_entry != UNSET_ADDR32 && hvm )
+            parms->virt_entry = parms->phys_entry;
+        else
+            parms->virt_entry = elf_uval(elf, elf->ehdr, e_entry);
+    }
 
     if ( parms->bsd_symtab )
     {
@@ -499,7 +509,7 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
 /* glue it all together ...                                                 */
 
 elf_errorstatus elf_xen_parse(struct elf_binary *elf,
-                  struct elf_dom_parms *parms)
+                  struct elf_dom_parms *parms, bool hvm)
 {
     ELF_HANDLE_DECL(elf_shdr) shdr;
     ELF_HANDLE_DECL(elf_phdr) phdr;
@@ -594,9 +604,9 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
         }
     }
 
-    if ( elf_xen_note_check(elf, parms) != 0 )
+    if ( elf_xen_note_check(elf, parms, hvm) != 0 )
         return -1;
-    if ( elf_xen_addr_calc_check(elf, parms) != 0 )
+    if ( elf_xen_addr_calc_check(elf, parms, hvm) != 0 )
         return -1;
     return 0;
 }
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index b73998150fc..be47b0cc366 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -454,7 +454,7 @@ int elf_xen_parse_note(struct elf_binary *elf,
 int elf_xen_parse_guest_info(struct elf_binary *elf,
                              struct elf_dom_parms *parms);
 int elf_xen_parse(struct elf_binary *elf,
-                  struct elf_dom_parms *parms);
+                  struct elf_dom_parms *parms, bool hvm);
 
 static inline void *elf_memcpy_unchecked(void *dest, const void *src, size_t n)
     { return memcpy(dest, src, n); }
-- 
2.31.1

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

[Jan Beulich]

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())?

>              parms->elf_paddr_offset = 0;

Similar remark as further up for the elf_msg() down below here.

Jan

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

[Roger Pau Monné]

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.

Thanks, Roger.

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

[Jan Beulich]

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.

Jan

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2635 bytes --]

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, including
a hint which elfnotes _have_ been evaluated before this series? This
will help cleaning up guests regarding advertisement of elfnotes
(something I've been planning to do for the Linux kernel).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Roger Pau Monné]

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.