[PATCH 0/3] x86/boot: Yet more PVH module handling fixes

[PATCH 0/3] x86/boot: Yet more PVH module handling fixes

[Andrew Cooper]

It turns out microcode and XSM module handling has been broken ever since PVH
boot was introduced.  Both appear to have have gone unnoticed because their
functionality isn't typically used in PVH guests.

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1508716668

Andrew Cooper (1):
  x86/boot: Add a temporary module_map pointer to boot_image

Daniel P. Smith (2):
  x86/boot: Fix microcode module handling during PVH boot
  x86/boot: Fix XSM module handling during PVH boot

 xen/arch/x86/cpu/microcode/core.c    | 40 +++++++++++-----------------
 xen/arch/x86/include/asm/bootinfo.h  |  1 +
 xen/arch/x86/include/asm/microcode.h |  8 +++---
 xen/arch/x86/setup.c                 | 10 ++++---
 xen/include/xsm/xsm.h                | 12 ++++-----
 xen/xsm/xsm_core.c                   |  7 +++--
 xen/xsm/xsm_policy.c                 | 16 +++++------
 7 files changed, 43 insertions(+), 51 deletions(-)

-- 
2.39.5

[PATCH 1/3] x86/boot: Add a temporary module_map pointer to boot_image

[Andrew Cooper]

... in order to untangle parameter handling independently from other other
logic changes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/include/asm/bootinfo.h | 1 +
 xen/arch/x86/setup.c                | 1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index ffa443406747..6237da7e4d86 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -31,6 +31,7 @@ struct boot_info {
     size_t memmap_length;
 
     unsigned int nr_modules;
+    unsigned long *module_map; /* Temporary */
     struct boot_module mods[MAX_NR_BOOTMODS + 1];
 };
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c5b37bd2112e..d8001867c925 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1086,6 +1086,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
     }
 
     bi = multiboot_fill_boot_info(mbi, mod);
+    bi->module_map = module_map; /* Temporary */
 
     /* Parse the command-line options. */
     if ( (kextra = strstr(bi->cmdline, " -- ")) != NULL )
-- 
2.39.5

Re: [PATCH 1/3] x86/boot: Add a temporary module_map pointer to boot_image

[Daniel P. Smith]

On 10/23/24 06:57, Andrew Cooper wrote:
> ... in order to untangle parameter handling independently from other other
> logic changes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com

[PATCH 2/3] x86/boot: Fix microcode module handling during PVH boot

[Andrew Cooper]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

As detailed in commit 0fe607b2a144 ("x86/boot: Fix PVH boot during boot_info
transition period"), the use of __va(mbi->mods_addr) constitutes a
use-after-free on the PVH boot path.

This pattern has been in use since before PVH support was added.  Inside a PVH
VM, it will go unnoticed as long as the microcode container parser doesn't
choke on the random data it finds.

The use within early_microcode_init() happens to be safe because it's prior to
move_xen().  microcode_init_cache() is after move_xen(), and therefore unsafe.

Plumb the boot_info pointer down, replacing module_map and mbi.  Importantly,
bi->mods[].mod is a safe way to access the module list during PVH boot.

Note: microcode_scan_module() is still bogusly stashing a bootstrap_map()'d
      pointer in ucode_blob.data, which constitutes a different
      use-after-free, and only works in general because of a second bug.  This
      is unrelated to PVH, and needs untangling differently.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>

Refectored/extracted from the hyperlaunch series.

There's no good Fixes tag for this, because it can't reasonably be "introduce
PVH", nor can the fix as-is reasonably be backported.  If we want to fix the
bug in older trees, we need to plumb the mod pointer down alongside mbi.
---
 xen/arch/x86/cpu/microcode/core.c    | 40 +++++++++++-----------------
 xen/arch/x86/include/asm/microcode.h |  8 +++---
 xen/arch/x86/setup.c                 |  4 +--
 3 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 8564e4d2c94c..1d58cb0f3bc1 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -35,6 +35,7 @@
 #include <xen/watchdog.h>
 
 #include <asm/apic.h>
+#include <asm/bootinfo.h>
 #include <asm/cpu-policy.h>
 #include <asm/nmi.h>
 #include <asm/processor.h>
@@ -152,11 +153,8 @@ static int __init cf_check parse_ucode(const char *s)
 }
 custom_param("ucode", parse_ucode);
 
-static void __init microcode_scan_module(
-    unsigned long *module_map,
-    const multiboot_info_t *mbi)
+static void __init microcode_scan_module(struct boot_info *bi)
 {
-    module_t *mod = (module_t *)__va(mbi->mods_addr);
     uint64_t *_blob_start;
     unsigned long _blob_size;
     struct cpio_data cd;
@@ -178,13 +176,13 @@ static void __init microcode_scan_module(
     /*
      * Try all modules and see whichever could be the microcode blob.
      */
-    for ( i = 1 /* Ignore dom0 kernel */; i < mbi->mods_count; i++ )
+    for ( i = 1 /* Ignore dom0 kernel */; i < bi->nr_modules; i++ )
     {
-        if ( !test_bit(i, module_map) )
+        if ( !test_bit(i, bi->module_map) )
             continue;
 
-        _blob_start = bootstrap_map(&mod[i]);
-        _blob_size = mod[i].mod_end;
+        _blob_start = bootstrap_map(bi->mods[i].mod);
+        _blob_size = bi->mods[i].mod->mod_end;
         if ( !_blob_start )
         {
             printk("Could not map multiboot module #%d (size: %ld)\n",
@@ -204,21 +202,17 @@ static void __init microcode_scan_module(
     }
 }
 
-static void __init microcode_grab_module(
-    unsigned long *module_map,
-    const multiboot_info_t *mbi)
+static void __init microcode_grab_module(struct boot_info *bi)
 {
-    module_t *mod = (module_t *)__va(mbi->mods_addr);
-
     if ( ucode_mod_idx < 0 )
-        ucode_mod_idx += mbi->mods_count;
-    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count ||
-         !__test_and_clear_bit(ucode_mod_idx, module_map) )
+        ucode_mod_idx += bi->nr_modules;
+    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
+         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
         goto scan;
-    ucode_mod = mod[ucode_mod_idx];
+    ucode_mod = *bi->mods[ucode_mod_idx].mod;
 scan:
     if ( ucode_scan )
-        microcode_scan_module(module_map, mbi);
+        microcode_scan_module(bi);
 }
 
 static struct microcode_ops __ro_after_init ucode_ops;
@@ -822,8 +816,7 @@ static int __init early_update_cache(const void *data, size_t len)
     return rc;
 }
 
-int __init microcode_init_cache(unsigned long *module_map,
-                                const struct multiboot_info *mbi)
+int __init microcode_init_cache(struct boot_info *bi)
 {
     int rc = 0;
 
@@ -832,7 +825,7 @@ int __init microcode_init_cache(unsigned long *module_map,
 
     if ( ucode_scan )
         /* Need to rescan the modules because they might have been relocated */
-        microcode_scan_module(module_map, mbi);
+        microcode_scan_module(bi);
 
     if ( ucode_mod.mod_end )
         rc = early_update_cache(bootstrap_map(&ucode_mod),
@@ -878,8 +871,7 @@ static int __init early_microcode_update_cpu(void)
     return microcode_update_cpu(patch, 0);
 }
 
-int __init early_microcode_init(unsigned long *module_map,
-                                const struct multiboot_info *mbi)
+int __init early_microcode_init(struct boot_info *bi)
 {
     const struct cpuinfo_x86 *c = &boot_cpu_data;
     int rc = 0;
@@ -922,7 +914,7 @@ int __init early_microcode_init(unsigned long *module_map,
         return -ENODEV;
     }
 
-    microcode_grab_module(module_map, mbi);
+    microcode_grab_module(bi);
 
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();
diff --git a/xen/arch/x86/include/asm/microcode.h b/xen/arch/x86/include/asm/microcode.h
index 57c08205d475..a278773f8b5d 100644
--- a/xen/arch/x86/include/asm/microcode.h
+++ b/xen/arch/x86/include/asm/microcode.h
@@ -24,10 +24,10 @@ DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 void microcode_set_module(unsigned int idx);
 int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
                      unsigned long len, unsigned int flags);
-int early_microcode_init(unsigned long *module_map,
-                         const struct multiboot_info *mbi);
-int microcode_init_cache(unsigned long *module_map,
-                         const struct multiboot_info *mbi);
 int microcode_update_one(void);
 
+struct boot_info;
+int early_microcode_init(struct boot_info *bi);
+int microcode_init_cache(struct boot_info *bi);
+
 #endif /* ASM_X86__MICROCODE_H */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d8001867c925..c75b8f15fa5d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1392,7 +1392,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
      * TODO: load ucode earlier once multiboot modules become accessible
      * at an earlier stage.
      */
-    early_microcode_init(module_map, mbi);
+    early_microcode_init(bi);
 
     if ( xen_phys_start )
     {
@@ -1936,7 +1936,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
 
     timer_init();
 
-    microcode_init_cache(module_map, mbi); /* Needs xmalloc() */
+    microcode_init_cache(bi); /* Needs xmalloc() */
 
     tsx_init(); /* Needs microcode.  May change HLE/RTM feature bits. */
 
-- 
2.39.5

Re: [PATCH 2/3] x86/boot: Fix microcode module handling during PVH boot

[Daniel P. Smith]

On 10/23/24 06:57, Andrew Cooper wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> 
> As detailed in commit 0fe607b2a144 ("x86/boot: Fix PVH boot during boot_info
> transition period"), the use of __va(mbi->mods_addr) constitutes a
> use-after-free on the PVH boot path.
> 
> This pattern has been in use since before PVH support was added.  Inside a PVH
> VM, it will go unnoticed as long as the microcode container parser doesn't
> choke on the random data it finds.
> 
> The use within early_microcode_init() happens to be safe because it's prior to
> move_xen().  microcode_init_cache() is after move_xen(), and therefore unsafe.
> 
> Plumb the boot_info pointer down, replacing module_map and mbi.  Importantly,
> bi->mods[].mod is a safe way to access the module list during PVH boot.
> 
> Note: microcode_scan_module() is still bogusly stashing a bootstrap_map()'d
>        pointer in ucode_blob.data, which constitutes a different
>        use-after-free, and only works in general because of a second bug.  This
>        is unrelated to PVH, and needs untangling differently.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Re: [PATCH 2/3] x86/boot: Fix microcode module handling during PVH boot

[Roger Pau Monné]

On Wed, Oct 23, 2024 at 11:57:55AM +0100, Andrew Cooper wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> 
> As detailed in commit 0fe607b2a144 ("x86/boot: Fix PVH boot during boot_info
> transition period"), the use of __va(mbi->mods_addr) constitutes a
> use-after-free on the PVH boot path.
> 
> This pattern has been in use since before PVH support was added.  Inside a PVH
> VM, it will go unnoticed as long as the microcode container parser doesn't
> choke on the random data it finds.
> 
> The use within early_microcode_init() happens to be safe because it's prior to
> move_xen().  microcode_init_cache() is after move_xen(), and therefore unsafe.
> 
> Plumb the boot_info pointer down, replacing module_map and mbi.  Importantly,
> bi->mods[].mod is a safe way to access the module list during PVH boot.
> 
> Note: microcode_scan_module() is still bogusly stashing a bootstrap_map()'d
>       pointer in ucode_blob.data, which constitutes a different
>       use-after-free, and only works in general because of a second bug.  This
>       is unrelated to PVH, and needs untangling differently.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> Refectored/extracted from the hyperlaunch series.
> 
> There's no good Fixes tag for this, because it can't reasonably be "introduce
> PVH", nor can the fix as-is reasonably be backported.  If we want to fix the
> bug in older trees, we need to plumb the mod pointer down alongside mbi.
> ---
>  xen/arch/x86/cpu/microcode/core.c    | 40 +++++++++++-----------------
>  xen/arch/x86/include/asm/microcode.h |  8 +++---
>  xen/arch/x86/setup.c                 |  4 +--
>  3 files changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index 8564e4d2c94c..1d58cb0f3bc1 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -35,6 +35,7 @@
>  #include <xen/watchdog.h>
>  
>  #include <asm/apic.h>
> +#include <asm/bootinfo.h>
>  #include <asm/cpu-policy.h>
>  #include <asm/nmi.h>
>  #include <asm/processor.h>
> @@ -152,11 +153,8 @@ static int __init cf_check parse_ucode(const char *s)
>  }
>  custom_param("ucode", parse_ucode);
>  
> -static void __init microcode_scan_module(
> -    unsigned long *module_map,
> -    const multiboot_info_t *mbi)
> +static void __init microcode_scan_module(struct boot_info *bi)

Sorry to be pedantic, can't you keep bi as const?

>  {
> -    module_t *mod = (module_t *)__va(mbi->mods_addr);
>      uint64_t *_blob_start;
>      unsigned long _blob_size;
>      struct cpio_data cd;
> @@ -178,13 +176,13 @@ static void __init microcode_scan_module(
>      /*
>       * Try all modules and see whichever could be the microcode blob.
>       */
> -    for ( i = 1 /* Ignore dom0 kernel */; i < mbi->mods_count; i++ )
> +    for ( i = 1 /* Ignore dom0 kernel */; i < bi->nr_modules; i++ )
>      {
> -        if ( !test_bit(i, module_map) )
> +        if ( !test_bit(i, bi->module_map) )
>              continue;
>  
> -        _blob_start = bootstrap_map(&mod[i]);
> -        _blob_size = mod[i].mod_end;
> +        _blob_start = bootstrap_map(bi->mods[i].mod);
> +        _blob_size = bi->mods[i].mod->mod_end;
>          if ( !_blob_start )
>          {
>              printk("Could not map multiboot module #%d (size: %ld)\n",
> @@ -204,21 +202,17 @@ static void __init microcode_scan_module(
>      }
>  }
>  
> -static void __init microcode_grab_module(
> -    unsigned long *module_map,
> -    const multiboot_info_t *mbi)
> +static void __init microcode_grab_module(struct boot_info *bi)

Same here re bi being const?

There are some further below, that I think all should keep the const
keywoard?

Otherwise looks good:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 2/3] x86/boot: Fix microcode module handling during PVH boot

[Andrew Cooper]

On 23/10/2024 6:01 pm, Roger Pau Monné wrote:
> On Wed, Oct 23, 2024 at 11:57:55AM +0100, Andrew Cooper wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> As detailed in commit 0fe607b2a144 ("x86/boot: Fix PVH boot during boot_info
>> transition period"), the use of __va(mbi->mods_addr) constitutes a
>> use-after-free on the PVH boot path.
>>
>> This pattern has been in use since before PVH support was added.  Inside a PVH
>> VM, it will go unnoticed as long as the microcode container parser doesn't
>> choke on the random data it finds.
>>
>> The use within early_microcode_init() happens to be safe because it's prior to
>> move_xen().  microcode_init_cache() is after move_xen(), and therefore unsafe.
>>
>> Plumb the boot_info pointer down, replacing module_map and mbi.  Importantly,
>> bi->mods[].mod is a safe way to access the module list during PVH boot.
>>
>> Note: microcode_scan_module() is still bogusly stashing a bootstrap_map()'d
>>       pointer in ucode_blob.data, which constitutes a different
>>       use-after-free, and only works in general because of a second bug.  This
>>       is unrelated to PVH, and needs untangling differently.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>>
>> Refectored/extracted from the hyperlaunch series.
>>
>> There's no good Fixes tag for this, because it can't reasonably be "introduce
>> PVH", nor can the fix as-is reasonably be backported.  If we want to fix the
>> bug in older trees, we need to plumb the mod pointer down alongside mbi.
>> ---
>>  xen/arch/x86/cpu/microcode/core.c    | 40 +++++++++++-----------------
>>  xen/arch/x86/include/asm/microcode.h |  8 +++---
>>  xen/arch/x86/setup.c                 |  4 +--
>>  3 files changed, 22 insertions(+), 30 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>> index 8564e4d2c94c..1d58cb0f3bc1 100644
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -35,6 +35,7 @@
>>  #include <xen/watchdog.h>
>>  
>>  #include <asm/apic.h>
>> +#include <asm/bootinfo.h>
>>  #include <asm/cpu-policy.h>
>>  #include <asm/nmi.h>
>>  #include <asm/processor.h>
>> @@ -152,11 +153,8 @@ static int __init cf_check parse_ucode(const char *s)
>>  }
>>  custom_param("ucode", parse_ucode);
>>  
>> -static void __init microcode_scan_module(
>> -    unsigned long *module_map,
>> -    const multiboot_info_t *mbi)
>> +static void __init microcode_scan_module(struct boot_info *bi)
> Sorry to be pedantic, can't you keep bi as const?

Not really, no.

Yes technically in this patch, but no by the end of the hyperlaunch series.

I'm uninterested in taking extra churn just to have a pointer be const
for a few more patches.

[For the list, I conferred with Roger IRL and he was happy.]

~Andrew

[PATCH 3/3] x86/boot: Fix XSM module handling during PVH boot

[Andrew Cooper]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

As detailed in commit 0fe607b2a144 ("x86/boot: Fix PVH boot during boot_info
transition period"), the use of __va(mbi->mods_addr) constitutes a
use-after-free on the PVH boot path.

This pattern has been in use since before PVH support was added.  This has
most likely gone unnoticed because no-one's tried using a detached Flask
policy in a PVH VM before.

Plumb the boot_info pointer down, replacing module_map and mbi.  Importantly,
bi->mods[].mod is a safe way to access the module list during PVH boot.

As this is the final non-bi use of mbi in __start_xen(), make the pointer
unusable once bi has been established, to prevent new uses creeping back in.
This is a stopgap until mbi can be fully removed.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>

Refectored/extracted from the hyperlaunch series.

There's no good Fixes tag for this, because it can't reasonably be "introduce
PVH", nor can the fix as-is reasonably be backported.  If we want to fix the
bug in older trees, we need to plumb the mod pointer down alongside mbi.
---
 xen/arch/x86/setup.c  |  5 ++++-
 xen/include/xsm/xsm.h | 12 +++++-------
 xen/xsm/xsm_core.c    |  7 +++----
 xen/xsm/xsm_policy.c  | 16 +++++++---------
 4 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c75b8f15fa5d..8974b0c6ede6 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1088,6 +1088,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
     bi = multiboot_fill_boot_info(mbi, mod);
     bi->module_map = module_map; /* Temporary */
 
+    /* Use bi-> instead */
+#define mbi DO_NOT_USE
+
     /* Parse the command-line options. */
     if ( (kextra = strstr(bi->cmdline, " -- ")) != NULL )
     {
@@ -1862,7 +1865,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
     mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
                                   RANGESETF_prettyprint_hex);
 
-    xsm_multiboot_init(module_map, mbi);
+    xsm_multiboot_init(bi);
 
     /*
      * IOMMU-related ACPI table parsing may require some of the system domains
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 627c0d2731af..4dbff9d866e0 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -17,7 +17,6 @@
 
 #include <xen/alternative-call.h>
 #include <xen/sched.h>
-#include <xen/multiboot.h>
 
 /* policy magic number (defined by XSM_MAGIC) */
 typedef uint32_t xsm_magic_t;
@@ -778,11 +777,10 @@ static inline int xsm_argo_send(const struct domain *d, const struct domain *t)
 #endif /* XSM_NO_WRAPPERS */
 
 #ifdef CONFIG_MULTIBOOT
-int xsm_multiboot_init(
-    unsigned long *module_map, const multiboot_info_t *mbi);
+struct boot_info;
+int xsm_multiboot_init(struct boot_info *bi);
 int xsm_multiboot_policy_init(
-    unsigned long *module_map, const multiboot_info_t *mbi,
-    void **policy_buffer, size_t *policy_size);
+    struct boot_info *bi, void **policy_buffer, size_t *policy_size);
 #endif
 
 #ifdef CONFIG_HAS_DEVICE_TREE
@@ -828,8 +826,8 @@ static const inline struct xsm_ops *silo_init(void)
 #include <xsm/dummy.h>
 
 #ifdef CONFIG_MULTIBOOT
-static inline int xsm_multiboot_init (
-    unsigned long *module_map, const multiboot_info_t *mbi)
+struct boot_info;
+static inline int xsm_multiboot_init(struct boot_info *bi)
 {
     return 0;
 }
diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index eaa028109bde..6e3fac68c057 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -21,6 +21,7 @@
 #ifdef CONFIG_XSM
 
 #ifdef CONFIG_MULTIBOOT
+#include <asm/bootinfo.h>
 #include <asm/setup.h>
 #endif
 
@@ -139,8 +140,7 @@ static int __init xsm_core_init(const void *policy_buffer, size_t policy_size)
 }
 
 #ifdef CONFIG_MULTIBOOT
-int __init xsm_multiboot_init(
-    unsigned long *module_map, const multiboot_info_t *mbi)
+int __init xsm_multiboot_init(struct boot_info *bi)
 {
     int ret = 0;
     void *policy_buffer = NULL;
@@ -150,8 +150,7 @@ int __init xsm_multiboot_init(
 
     if ( XSM_MAGIC )
     {
-        ret = xsm_multiboot_policy_init(module_map, mbi, &policy_buffer,
-                                        &policy_size);
+        ret = xsm_multiboot_policy_init(bi, &policy_buffer, &policy_size);
         if ( ret )
         {
             bootstrap_map(NULL);
diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
index 8dafbc93810f..6f799dd28f5b 100644
--- a/xen/xsm/xsm_policy.c
+++ b/xen/xsm/xsm_policy.c
@@ -20,7 +20,7 @@
 
 #include <xsm/xsm.h>
 #ifdef CONFIG_MULTIBOOT
-#include <xen/multiboot.h>
+#include <asm/bootinfo.h>
 #include <asm/setup.h>
 #endif
 #include <xen/bitops.h>
@@ -31,11 +31,9 @@
 
 #ifdef CONFIG_MULTIBOOT
 int __init xsm_multiboot_policy_init(
-    unsigned long *module_map, const multiboot_info_t *mbi,
-    void **policy_buffer, size_t *policy_size)
+    struct boot_info *bi, void **policy_buffer, size_t *policy_size)
 {
     int i;
-    module_t *mod = (module_t *)__va(mbi->mods_addr);
     int rc = 0;
     u32 *_policy_start;
     unsigned long _policy_len;
@@ -44,13 +42,13 @@ int __init xsm_multiboot_policy_init(
      * Try all modules and see whichever could be the binary policy.
      * Adjust module_map for the module that is the binary policy.
      */
-    for ( i = mbi->mods_count-1; i >= 1; i-- )
+    for ( i = bi->nr_modules - 1; i >= 1; i-- )
     {
-        if ( !test_bit(i, module_map) )
+        if ( !test_bit(i, bi->module_map) )
             continue;
 
-        _policy_start = bootstrap_map(mod + i);
-        _policy_len   = mod[i].mod_end;
+        _policy_start = bootstrap_map(bi->mods[i].mod);
+        _policy_len   = bi->mods[i].mod->mod_end;
 
         if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC )
         {
@@ -60,7 +58,7 @@ int __init xsm_multiboot_policy_init(
             printk("Policy len %#lx, start at %p.\n",
                    _policy_len,_policy_start);
 
-            __clear_bit(i, module_map);
+            __clear_bit(i, bi->module_map);
             break;
 
         }
-- 
2.39.5

Re: [PATCH 3/3] x86/boot: Fix XSM module handling during PVH boot

[Daniel P. Smith]

On 10/23/24 06:57, Andrew Cooper wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> 
> As detailed in commit 0fe607b2a144 ("x86/boot: Fix PVH boot during boot_info
> transition period"), the use of __va(mbi->mods_addr) constitutes a
> use-after-free on the PVH boot path.
> 
> This pattern has been in use since before PVH support was added.  This has
> most likely gone unnoticed because no-one's tried using a detached Flask
> policy in a PVH VM before.
> 
> Plumb the boot_info pointer down, replacing module_map and mbi.  Importantly,
> bi->mods[].mod is a safe way to access the module list during PVH boot.
> 
> As this is the final non-bi use of mbi in __start_xen(), make the pointer
> unusable once bi has been established, to prevent new uses creeping back in.
> This is a stopgap until mbi can be fully removed.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Re: [PATCH 3/3] x86/boot: Fix XSM module handling during PVH boot

[Roger Pau Monné]

On Wed, Oct 23, 2024 at 11:57:56AM +0100, Andrew Cooper wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> 
> As detailed in commit 0fe607b2a144 ("x86/boot: Fix PVH boot during boot_info
> transition period"), the use of __va(mbi->mods_addr) constitutes a
> use-after-free on the PVH boot path.
> 
> This pattern has been in use since before PVH support was added.  This has
> most likely gone unnoticed because no-one's tried using a detached Flask
> policy in a PVH VM before.
> 
> Plumb the boot_info pointer down, replacing module_map and mbi.  Importantly,
> bi->mods[].mod is a safe way to access the module list during PVH boot.
> 
> As this is the final non-bi use of mbi in __start_xen(), make the pointer
> unusable once bi has been established, to prevent new uses creeping back in.
> This is a stopgap until mbi can be fully removed.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> Refectored/extracted from the hyperlaunch series.
> 
> There's no good Fixes tag for this, because it can't reasonably be "introduce
> PVH", nor can the fix as-is reasonably be backported.  If we want to fix the
> bug in older trees, we need to plumb the mod pointer down alongside mbi.
> ---
>  xen/arch/x86/setup.c  |  5 ++++-
>  xen/include/xsm/xsm.h | 12 +++++-------
>  xen/xsm/xsm_core.c    |  7 +++----
>  xen/xsm/xsm_policy.c  | 16 +++++++---------
>  4 files changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index c75b8f15fa5d..8974b0c6ede6 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1088,6 +1088,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>      bi = multiboot_fill_boot_info(mbi, mod);
>      bi->module_map = module_map; /* Temporary */
>  
> +    /* Use bi-> instead */
> +#define mbi DO_NOT_USE
> +
>      /* Parse the command-line options. */
>      if ( (kextra = strstr(bi->cmdline, " -- ")) != NULL )
>      {
> @@ -1862,7 +1865,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
>                                    RANGESETF_prettyprint_hex);
>  
> -    xsm_multiboot_init(module_map, mbi);
> +    xsm_multiboot_init(bi);
>  
>      /*
>       * IOMMU-related ACPI table parsing may require some of the system domains
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 627c0d2731af..4dbff9d866e0 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -17,7 +17,6 @@
>  
>  #include <xen/alternative-call.h>
>  #include <xen/sched.h>
> -#include <xen/multiboot.h>
>  
>  /* policy magic number (defined by XSM_MAGIC) */
>  typedef uint32_t xsm_magic_t;
> @@ -778,11 +777,10 @@ static inline int xsm_argo_send(const struct domain *d, const struct domain *t)
>  #endif /* XSM_NO_WRAPPERS */
>  
>  #ifdef CONFIG_MULTIBOOT
> -int xsm_multiboot_init(
> -    unsigned long *module_map, const multiboot_info_t *mbi);
> +struct boot_info;
> +int xsm_multiboot_init(struct boot_info *bi);

This one seems to also drop a const?

This also propagates to the functions below.

With that:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH 4/3] x86/boot: Remove the mbi_p parameter from __start_xen()

[Andrew Cooper]

The use of physical addresses in __start_xen() has proved to be fertile soure
of bugs.

The MB1/2 path stashes the MBI pointer in multiboot_ptr (a setup.c variable
even), then re-loads it immediately before calling __start_xen().  For this,
we can just drop the function parameter and read multiboot_ptr in the one
place it's used.

The EFI path also passes this parameter into __start_xen().  Have the EFI path
set up multiboot_ptr too, and move the explanation of phyiscal-mode pointers.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1509072662
---
 xen/arch/x86/boot/x86_64.S       |  2 --
 xen/arch/x86/efi/efi-boot.h      |  9 +++++++--
 xen/arch/x86/include/asm/setup.h |  1 +
 xen/arch/x86/setup.c             | 14 +++++---------
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 04bb62ae8680..26b9d1c2df9a 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -77,8 +77,6 @@ ENTRY(__high_start)
         tailcall start_secondary
 
 .L_bsp:
-        /* Pass off the Multiboot info structure to C land (if applicable). */
-        mov     multiboot_ptr(%rip),%edi
         tailcall __start_xen
 
         .section .data.page_aligned, "aw", @progbits
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 94f34433640f..3b26f0b0f500 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -248,6 +248,12 @@ static void __init noreturn efi_arch_post_exit_boot(void)
     efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start);
     memcpy((void *)trampoline_phys, trampoline_start, cfg.size);
 
+    /*
+     * We're in physical mode right now (i.e. identity map), so a regular
+     * pointer is also a phyiscal address to the rest of Xen.
+     */
+    multiboot_ptr = (unsigned long)&mbi;
+
     /* Set system registers and transfer control. */
     asm volatile("pushq $0\n\tpopfq");
     rdmsrl(MSR_EFER, efer);
@@ -279,8 +285,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
                      [cr4] "+&r" (cr4)
                    : [cr3] "r" (idle_pg_table),
                      [cs] "i" (__HYPERVISOR_CS),
-                     [ds] "r" (__HYPERVISOR_DS),
-                     "D" (&mbi)
+                     [ds] "r" (__HYPERVISOR_DS)
                    : "memory" );
     unreachable();
 }
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 3d189521189d..811855e57478 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -14,6 +14,7 @@ extern unsigned long xenheap_initial_phys_start;
 extern uint64_t boot_tsc_stamp;
 
 extern void *stack_start;
+extern unsigned int multiboot_ptr;
 
 void early_cpu_init(bool verbose);
 void early_time_init(void);
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c5b37bd2112e..c9129c21787b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -157,8 +157,8 @@ char asmlinkage __section(".init.bss.stack_aligned") __aligned(STACK_SIZE)
 /* Used by the BSP/AP paths to find the higher half stack mapping to use. */
 void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info);
 
-/* Used by the boot asm to stash the relocated multiboot info pointer. */
-unsigned int asmlinkage __initdata multiboot_ptr;
+/* Used by the boot asm and EFI to stash the multiboot_info paddr. */
+unsigned int __initdata multiboot_ptr;
 
 struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
 
@@ -1014,7 +1014,7 @@ static struct domain *__init create_dom0(const module_t *image,
 /* How much of the directmap is prebuilt at compile time. */
 #define PREBUILT_MAP_LIMIT (1 << L2_PAGETABLE_SHIFT)
 
-void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
+void asmlinkage __init noreturn __start_xen(void)
 {
     const char *memmap_type = NULL;
     char *kextra;
@@ -1059,7 +1059,6 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
 
     if ( pvh_boot )
     {
-        ASSERT(mbi_p == 0);
         pvh_init(&mbi, &mod);
         /*
          * mbi and mod are regular pointers to .initdata.  These remain valid
@@ -1068,7 +1067,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
     }
     else
     {
-        mbi = __va(mbi_p);
+        mbi = __va(multiboot_ptr);
         mod = __va(mbi->mods_addr);
 
         /*
@@ -1078,11 +1077,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
          * For EFI, these are directmap pointers into the Xen image.  They do
          * not remain valid across move_xen().  EFI boot only functions
          * because a non-zero xen_phys_start inhibits move_xen().
-         *
-         * Don't be fooled by efi_arch_post_exit_boot() passing "D" (&mbi).
-         * This is a EFI physical-mode (i.e. identity map) pointer.
          */
-        ASSERT(mbi_p < MB(1) || xen_phys_start);
+        ASSERT(multiboot_ptr < MB(1) || xen_phys_start);
     }
 
     bi = multiboot_fill_boot_info(mbi, mod);

base-commit: be84e7fe58b51f6b6dd907a038f0ef998a1e281e
prerequisite-patch-id: 795f6e9425cc6a953166b530ae68df466a7a3c2b
-- 
2.39.5

Re: [PATCH 4/3] x86/boot: Remove the mbi_p parameter from __start_xen()

[Daniel P. Smith]

On 10/23/24 10:44, Andrew Cooper wrote:
> The use of physical addresses in __start_xen() has proved to be fertile soure
> of bugs.
> 
> The MB1/2 path stashes the MBI pointer in multiboot_ptr (a setup.c variable
> even), then re-loads it immediately before calling __start_xen().  For this,
> we can just drop the function parameter and read multiboot_ptr in the one
> place it's used.
> 
> The EFI path also passes this parameter into __start_xen().  Have the EFI path
> set up multiboot_ptr too, and move the explanation of phyiscal-mode pointers.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>

For EFI:
Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>

For remainder:
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Re: [PATCH 4/3] x86/boot: Remove the mbi_p parameter from __start_xen()

[Andrew Cooper]

On 23/10/2024 3:58 pm, Daniel P. Smith wrote:
> On 10/23/24 10:44, Andrew Cooper wrote:
>> The use of physical addresses in __start_xen() has proved to be
>> fertile soure
>> of bugs.
>>
>> The MB1/2 path stashes the MBI pointer in multiboot_ptr (a setup.c
>> variable
>> even), then re-loads it immediately before calling __start_xen(). 
>> For this,
>> we can just drop the function parameter and read multiboot_ptr in the
>> one
>> place it's used.
>>
>> The EFI path also passes this parameter into __start_xen().  Have the
>> EFI path
>> set up multiboot_ptr too, and move the explanation of phyiscal-mode
>> pointers.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>
> For EFI:
> Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>
> For remainder:
> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Thanks.  That's usually just R-by then.

In Xen, we take A-by to mean "sure, whatever", and R-by to mean "I've
looked at this in detail and think it's good".

R-by implies A-by, and either are fine from a maintainership point of view.

~Andrew