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

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.

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

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>