[Xen-devel] [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

panic() doesn't contain any caller information, so the sum output of:

  (d1) (XEN)
  (d1) (XEN) ****************************************
  (d1) (XEN) Panic on CPU 0:
  (d1) (XEN) Magic value is wrong: 336ec568
  (d1) (XEN) ****************************************
  (d1) (XEN)

isn't helpful at identifying what went wrong.  Update the panic() strings to
identify PVH and aid with diagnostics.

The BUG_ON() check for ARRAY_SIZE(pvh_mbi_mods) is off-by-one, and redundant
with the earlier panic() which explains things in more detail.  Drop it.

Finally, Xen takes nr_modules != 0 to mean that modlist_paddr is valid, but a
cleverly crafterd PVH start info layout can cause Xen to use modlist_paddr at
gaddr 0, in violation of the PVH spec.

As such a start info would be a domain builder bug, crosscheck and ignore
modules in such a case, rather than fixing up all of Xen to check both values.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
CC: Juergen Gross <jgross@xxxxxxxx>
---
 xen/arch/x86/guest/pvh-boot.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
index 544775e..01f1376 100644
--- a/xen/arch/x86/guest/pvh-boot.c
+++ b/xen/arch/x86/guest/pvh-boot.c
@@ -38,12 +38,20 @@ static const char *__initdata pvh_loader = "PVH Directboot";
 static void __init convert_pvh_info(multiboot_info_t **mbi,
                                     module_t **mod)
 {
-    const struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
+    struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
     const struct hvm_modlist_entry *entry;
     unsigned int i;
 
     if ( pvh_info->magic != XEN_HVM_START_MAGIC_VALUE )
-        panic("Magic value is wrong: %x\n", pvh_info->magic);
+        panic("PVH magic value is wrong: %x\n", pvh_info->magic);
+
+    /* Check consistency between the modlist and number of modules. */
+    if ( (pvh_info->modlist_paddr == 0) != (pvh_info->nr_modules == 0) )
+    {
+        printk("PVH module mismatch: pa %08"PRIx64", nr %u - Ignoring\n",
+               pvh_info->modlist_paddr, pvh_info->nr_modules);
+        pvh_info->modlist_paddr = pvh_info->nr_modules = 0;
+    }
 
     /*
      * Temporary module array needs to be at least one element bigger than
@@ -51,8 +59,8 @@ static void __init convert_pvh_info(multiboot_info_t **mbi,
      * arch/x86/setup.c:__start_xen().
      */
     if ( ARRAY_SIZE(pvh_mbi_mods) <= pvh_info->nr_modules )
-        panic("The module array is too small, size %zu, requested %u\n",
-              ARRAY_SIZE(pvh_mbi_mods), pvh_info->nr_modules);
+        panic("Insufficient PVH module array entries.  Have %zu, need %u\n",
+              ARRAY_SIZE(pvh_mbi_mods), pvh_info->nr_modules + 1);
 
     /*
      * Turn hvm_start_info into mbi. Luckily all modules are placed under 4GB
@@ -64,7 +72,6 @@ static void __init convert_pvh_info(multiboot_info_t **mbi,
     pvh_mbi.cmdline = pvh_info->cmdline_paddr;
     pvh_mbi.boot_loader_name = __pa(pvh_loader);
 
-    BUG_ON(pvh_info->nr_modules >= ARRAY_SIZE(pvh_mbi_mods));
     pvh_mbi.mods_count = pvh_info->nr_modules;
     pvh_mbi.mods_addr = __pa(pvh_mbi_mods);
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel