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

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

On Wed, Oct 23, 2024 at 11:57:56AM +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.  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@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/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@xxxxxxxxxx>

Thanks, Roger.