[PATCH] libelf: improve PVH elfnote parsing

[PATCH] 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>
---
 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  | 25 +++++++++++++++----------
 xen/include/xen/libelf.h            |  2 +-
 7 files changed, 25 insertions(+), 17 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..584be0f6fb2 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;
 
@@ -426,7 +426,7 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
     }
 
     /* Initial guess for virt_base is 0 if it is not explicitly defined. */
-    if ( parms->virt_base == UNSET_ADDR )
+    if ( parms->virt_base == UNSET_ADDR || hvm )
     {
         parms->virt_base = 0;
         elf_msg(elf, "ELF: VIRT_BASE unset, using %#" PRIx64 "\n",
@@ -442,7 +442,7 @@ 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.
      */
-    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 +456,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 )
+            parms->virt_entry = parms->phys_entry;
+        else
+            parms->virt_entry = elf_uval(elf, elf->ehdr, e_entry);
+    }
 
     if ( parms->bsd_symtab )
     {
@@ -499,7 +504,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 +599,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] libelf: improve PVH elfnote parsing

[Jason Andryuk]

On Fri, May 14, 2021 at 9:50 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> 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.

Should elf_xen_addr_calc_check be changed so that PV operates on
virtual addresses and HVM operates on physical addresses?

I worked on some patches for this a while back, but lost track when
other work pulled me away.  I'll send out what I had, but I think I
had not tested many of the cases.  Also, I had other questions about
the approach.  Fundamentally, what notes and limits need to be checked
for PVH vs. PV?

Regards,
Jason

[RFC PATCH 1/3] libelf: Introduce phys_kstart/end

[Jason Andryuk]

The physical start and end matter for PVH.  These are only used by a PVH
dom0, but will help when separating the PV and PVH ELF checking in the
next patch.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 xen/arch/x86/hvm/dom0_build.c      | 4 ++--
 xen/common/libelf/libelf-dominfo.c | 3 +++
 xen/include/xen/libelf.h           | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 878dc1d808..5b9192ecc6 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -574,8 +574,8 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
     }
 
     /* Copy the OS image and free temporary buffer. */
-    elf.dest_base = (void *)(parms.virt_kstart - parms.virt_base);
-    elf.dest_size = parms.virt_kend - parms.virt_kstart;
+    elf.dest_base = (void *)parms.phys_kstart - parms.elf_paddr_offset;
+    elf.dest_size = parms.phys_kend - parms.phys_kstart;
 
     elf_set_vcpu(&elf, v);
     rc = elf_load_binary(&elf);
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 69c94b6f3b..b1f36866eb 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -453,6 +453,8 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
     }
 
     virt_offset = parms->virt_base - parms->elf_paddr_offset;
+    parms->phys_kstart = elf->pstart;
+    parms->phys_kend   = elf->pend;
     parms->virt_kstart = elf->pstart + virt_offset;
     parms->virt_kend   = elf->pend   + virt_offset;
 
@@ -464,6 +466,7 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
         elf_parse_bsdsyms(elf, elf->pend);
         if ( elf->bsd_symtab_pend )
             parms->virt_kend = elf->bsd_symtab_pend + virt_offset;
+            parms->phys_kend = elf->bsd_symtab_pend;
     }
 
     elf_msg(elf, "ELF: addresses:\n");
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index b73998150f..8d80d0812a 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -434,6 +434,8 @@ struct elf_dom_parms {
     /* calculated */
     uint64_t virt_kstart;
     uint64_t virt_kend;
+    uint64_t phys_kstart;
+    uint64_t phys_kend;
 };
 
 static inline void elf_xen_feature_set(int nr, uint32_t * addr)
-- 
2.31.1

[RFC PATCH 2/3] libelf: Use flags to check pv or pvh in elf_xen_parse

[Jason Andryuk]

Certain checks are only applicable to PV vs. PVH, so split them and run
only the appropriate checks for each.

This fixes loading a PVH kernel that has a PHYS32_ENTRY but not an ENTRY
ELF note.  Such a kernel would fail the virt_entry check which is not
applicable for PVH.

This re-instatates loader and xen version checks for the PV case that
were omited for kernels passing the PHYS32_ENTRY check.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/fuzz/libelf/libelf-fuzzer.c   |  2 +-
 tools/libs/guest/xg_dom_elfloader.c | 11 +++-
 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  | 91 +++++++++++++++++++++++------
 xen/include/xen/libelf.h            |  7 ++-
 7 files changed, 93 insertions(+), 24 deletions(-)

diff --git a/tools/fuzz/libelf/libelf-fuzzer.c b/tools/fuzz/libelf/libelf-fuzzer.c
index 1ba8571711..f488510618 100644
--- a/tools/fuzz/libelf/libelf-fuzzer.c
+++ b/tools/fuzz/libelf/libelf-fuzzer.c
@@ -17,7 +17,7 @@ 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, ELF_XEN_CHECK_PV | ELF_XEN_CHECK_PVH);
 
     return 0;
 }
diff --git a/tools/libs/guest/xg_dom_elfloader.c b/tools/libs/guest/xg_dom_elfloader.c
index 06e713fe11..c3280b1603 100644
--- a/tools/libs/guest/xg_dom_elfloader.c
+++ b/tools/libs/guest/xg_dom_elfloader.c
@@ -120,6 +120,7 @@ static elf_negerrnoval check_elf_kernel(struct xc_dom_image *dom, bool verbose)
 static elf_negerrnoval xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
 {
     struct elf_binary elf;
+    unsigned int flags;
     int rc;
 
     rc = check_elf_kernel(dom, 0);
@@ -135,7 +136,9 @@ 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);
+    flags = dom->container_type == XC_DOM_PV_CONTAINER ? ELF_XEN_CHECK_PV :
+                                                         ELF_XEN_CHECK_PVH;
+    rc = elf_xen_parse(&elf, dom->parms, flags);
     if ( rc != 0 )
         return rc;
 
@@ -146,6 +149,7 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
 {
     struct elf_binary *elf;
     elf_negerrnoval rc;
+    unsigned int flags;
 
     rc = check_elf_kernel(dom, 1);
     if ( rc != 0 )
@@ -166,7 +170,10 @@ 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 )
+    flags = dom->container_type == XC_DOM_PV_CONTAINER ? ELF_XEN_CHECK_PV :
+                                                         ELF_XEN_CHECK_PVH;
+    rc = elf_xen_parse(elf, dom->parms, flags);
+    if ( rc != 0 )
     {
         rc = -EINVAL;
         goto out;
diff --git a/tools/libs/guest/xg_dom_hvmloader.c b/tools/libs/guest/xg_dom_hvmloader.c
index ec6ebad7fd..bf28690415 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, ELF_XEN_CHECK_PV | ELF_XEN_CHECK_PVH);
     if ( rc == 0 )
         return -EINVAL;
 
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 5b9192ecc6..552448ce5d 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, ELF_XEN_CHECK_PVH)) != 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 e0801a9e6d..8bc77b0366 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, ELF_XEN_CHECK_PV)) != 0 )
         goto out;
 
     /* compatibility check */
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index b1f36866eb..13eb39ec52 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -359,7 +359,21 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
 /* ------------------------------------------------------------------------ */
 /* sanity checks                                                            */
 
-static elf_errorstatus elf_xen_note_check(struct elf_binary *elf,
+static elf_errorstatus elf_xen_note_check_pvh(struct elf_binary *elf,
+                              struct elf_dom_parms *parms)
+{
+    /* 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;
+    } else {
+        elf_err(elf, "ELF: Missing PVH PHYS32_ENTRY\n");
+        return -1;
+    }
+}
+
+static elf_errorstatus elf_xen_note_check_pv(struct elf_binary *elf,
                               struct elf_dom_parms *parms)
 {
     if ( (ELF_PTRVAL_INVALID(parms->elf_note_start)) &&
@@ -381,13 +395,6 @@ static elf_errorstatus elf_xen_note_check(struct elf_binary *elf,
          return 0;
     }
 
-    /* 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;
-    }
-
     /* Check the contents of the Xen notes or guest string. */
     if ( ((strlen(parms->loader) == 0) ||
           strncmp(parms->loader, "generic", 7)) &&
@@ -413,7 +420,36 @@ static elf_errorstatus elf_xen_note_check(struct elf_binary *elf,
     return 0;
 }
 
-static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
+static elf_errorstatus elf_xen_addr_calc_check_pvh(struct elf_binary *elf,
+                                                   struct elf_dom_parms *parms)
+{
+    parms->phys_kstart = elf->pstart;
+    parms->phys_kend   = elf->pend;
+
+    if ( parms->bsd_symtab )
+    {
+        elf_parse_bsdsyms(elf, elf->pend);
+        if ( elf->bsd_symtab_pend )
+            parms->phys_kend = elf->bsd_symtab_pend;
+    }
+
+    elf_msg(elf, "ELF: addresses:\n");
+    elf_msg(elf, "    phys_kstart      = 0x%" PRIx64 "\n", parms->phys_kstart);
+    elf_msg(elf, "    phys_kend        = 0x%" PRIx64 "\n", parms->phys_kend);
+    elf_msg(elf, "    phys_entry       = 0x%" PRIx32 "\n", parms->phys_entry);
+
+    if ( parms->phys_kstart > parms->phys_kend ||
+         parms->phys_entry < parms->phys_kstart ||
+         parms->phys_entry > parms->phys_kend )
+    {
+        elf_err(elf, "ERROR: ELF start or entries are out of bounds\n");
+        return -1;
+    }
+
+    return 0;
+}
+
+static elf_errorstatus elf_xen_addr_calc_check_pv(struct elf_binary *elf,
                                    struct elf_dom_parms *parms)
 {
     uint64_t virt_offset;
@@ -453,8 +489,6 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
     }
 
     virt_offset = parms->virt_base - parms->elf_paddr_offset;
-    parms->phys_kstart = elf->pstart;
-    parms->phys_kend   = elf->pend;
     parms->virt_kstart = elf->pstart + virt_offset;
     parms->virt_kend   = elf->pend   + virt_offset;
 
@@ -466,7 +500,6 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
         elf_parse_bsdsyms(elf, elf->pend);
         if ( elf->bsd_symtab_pend )
             parms->virt_kend = elf->bsd_symtab_pend + virt_offset;
-            parms->phys_kend = elf->bsd_symtab_pend;
     }
 
     elf_msg(elf, "ELF: addresses:\n");
@@ -500,9 +533,8 @@ 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)
+static elf_errorstatus elf_xen_parse_common(struct elf_binary *elf,
+                                            struct elf_dom_parms *parms)
 {
     ELF_HANDLE_DECL(elf_shdr) shdr;
     ELF_HANDLE_DECL(elf_phdr) phdr;
@@ -597,10 +629,35 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
         }
     }
 
-    if ( elf_xen_note_check(elf, parms) != 0 )
+    return 0;
+}
+
+elf_errorstatus elf_xen_parse(struct elf_binary *elf,
+                              struct elf_dom_parms *parms,
+                              unsigned int flags)
+{
+    if ( !flags ) {
+        elf_err(elf, "Must specify ELF_XEN_CHECK_{PV,PVH} flags to check");
         return -1;
-    if ( elf_xen_addr_calc_check(elf, parms) != 0 )
+    }
+
+    if ( elf_xen_parse_common(elf, parms) != 0 )
         return -1;
+
+    if ( flags & ELF_XEN_CHECK_PV ) {
+        if ( elf_xen_note_check_pv(elf, parms) != 0 )
+            return -1;
+        if ( elf_xen_addr_calc_check_pv(elf, parms) != 0 )
+            return -1;
+    }
+
+    if ( flags & ELF_XEN_CHECK_PVH ) {
+        if ( elf_xen_note_check_pvh(elf, parms) != 0 )
+            return -1;
+        if ( elf_xen_addr_calc_check_pvh(elf, parms) != 0 )
+            return -1;
+    }
+
     return 0;
 }
 
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 8d80d0812a..858f42cf07 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -455,8 +455,13 @@ int elf_xen_parse_note(struct elf_binary *elf,
                        ELF_HANDLE_DECL(elf_note) note);
 int elf_xen_parse_guest_info(struct elf_binary *elf,
                              struct elf_dom_parms *parms);
+
+#define ELF_XEN_CHECK_PV  (1 << 0)
+#define ELF_XEN_CHECK_PVH (1 << 1)
+
 int elf_xen_parse(struct elf_binary *elf,
-                  struct elf_dom_parms *parms);
+                  struct elf_dom_parms *parms,
+                  unsigned int flags);
 
 static inline void *elf_memcpy_unchecked(void *dest, const void *src, size_t n)
     { return memcpy(dest, src, n); }
-- 
2.31.1

[RFC PATCH 3/3] libelf: PVH: only allow elf_paddr_offset of 0

[Jason Andryuk]

Modern Linux and FreeBSD hardcode it to 0.  Just drop its use for PVH.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 xen/arch/x86/hvm/dom0_build.c      | 2 +-
 xen/common/libelf/libelf-dominfo.c | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 552448ce5d..335321ed3e 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -574,7 +574,7 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
     }
 
     /* Copy the OS image and free temporary buffer. */
-    elf.dest_base = (void *)parms.phys_kstart - parms.elf_paddr_offset;
+    elf.dest_base = (void *)parms.phys_kstart;
     elf.dest_size = parms.phys_kend - parms.phys_kstart;
 
     elf_set_vcpu(&elf, v);
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 13eb39ec52..12feb8755e 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -433,6 +433,12 @@ static elf_errorstatus elf_xen_addr_calc_check_pvh(struct elf_binary *elf,
             parms->phys_kend = elf->bsd_symtab_pend;
     }
 
+    if ( parms->elf_paddr_offset != 0 ) {
+        elf_err(elf, "ERROR: ELF elf_paddr_offset (0x" PRIx64 ") is non-zero\n",
+                parms->elf_paddr_offset);
+        return -1;
+    }
+
     elf_msg(elf, "ELF: addresses:\n");
     elf_msg(elf, "    phys_kstart      = 0x%" PRIx64 "\n", parms->phys_kstart);
     elf_msg(elf, "    phys_kend        = 0x%" PRIx64 "\n", parms->phys_kend);
-- 
2.31.1

Re: [PATCH] libelf: improve PVH elfnote parsing

[Jan Beulich]

On 14.05.2021 15:50, Roger Pau Monne wrote:
> @@ -426,7 +426,7 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
>      }
>  
>      /* Initial guess for virt_base is 0 if it is not explicitly defined. */
> -    if ( parms->virt_base == UNSET_ADDR )
> +    if ( parms->virt_base == UNSET_ADDR || hvm )
>      {
>          parms->virt_base = 0;
>          elf_msg(elf, "ELF: VIRT_BASE unset, using %#" PRIx64 "\n",
> @@ -442,7 +442,7 @@ 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.
>       */
> -    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;

Both of these would want their respective comments also updated, I
think: There's no defaulting or guessing really in PVH mode, is
there?

> @@ -456,8 +456,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 )

Don't you need "&& hvm" here to prevent ...

> +            parms->virt_entry = parms->phys_entry;

... this taking effect for a kernel capable of running in both
PV and PVH modes, instead of ...

> +        else
> +            parms->virt_entry = elf_uval(elf, elf->ehdr, e_entry);

... this (when actually in PV mode)?

Jan

Re: [PATCH] libelf: improve PVH elfnote parsing

[Roger Pau Monné]

On Mon, May 17, 2021 at 01:09:11PM +0200, Jan Beulich wrote:
> On 14.05.2021 15:50, Roger Pau Monne wrote:
> > @@ -426,7 +426,7 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
> >      }
> >  
> >      /* Initial guess for virt_base is 0 if it is not explicitly defined. */
> > -    if ( parms->virt_base == UNSET_ADDR )
> > +    if ( parms->virt_base == UNSET_ADDR || hvm )
> >      {
> >          parms->virt_base = 0;
> >          elf_msg(elf, "ELF: VIRT_BASE unset, using %#" PRIx64 "\n",
> > @@ -442,7 +442,7 @@ 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.
> >       */
> > -    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;
> 
> Both of these would want their respective comments also updated, I
> think: There's no defaulting or guessing really in PVH mode, is
> there?
> 
> > @@ -456,8 +456,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 )
> 
> Don't you need "&& hvm" here to prevent ...
> 
> > +            parms->virt_entry = parms->phys_entry;
> 
> ... this taking effect for a kernel capable of running in both
> PV and PVH modes, instead of ...
> 
> > +        else
> > +            parms->virt_entry = elf_uval(elf, elf->ehdr, e_entry);
> 
> ... this (when actually in PV mode)?

Oh, I somehow assumed that PV guests _must_ provide the entry point in
XEN_ELFNOTE_ENTRY, but I don't think that's the case. Will update and
send a new version.

Thanks, Roger.

Re: [PATCH] libelf: improve PVH elfnote parsing

[Roger Pau Monné]

On Fri, May 14, 2021 at 11:11:14AM -0400, Jason Andryuk wrote:
> On Fri, May 14, 2021 at 9:50 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
> >
> > 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.
> 
> Should elf_xen_addr_calc_check be changed so that PV operates on
> virtual addresses and HVM operates on physical addresses?

Right... I was aiming with getting away with something simpler and
just assume phys == virt on HVM in order to avoid more complicated
changes and the need to introduce new fields on the structure.

> I worked on some patches for this a while back, but lost track when
> other work pulled me away.  I'll send out what I had, but I think I
> had not tested many of the cases.  Also, I had other questions about
> the approach.  Fundamentally, what notes and limits need to be checked
> for PVH vs. PV?

Those are only sanity checks to assert that the image is kind of fine,
libelf also has checks when loading stuff to make sure a malicious elf
payload cannot fool the loader.

I'm unlikely to be able to do much work on this aside from this
current patch.

Thanks, Roger.