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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Oct 23, 2024 at 11:57:55AM +0100, Andrew Cooper wrote:
> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxx>

Thanks, Roger.