| Message ID | 20241023105756.641695-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: Yet more PVH module handling fixes | expand |
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>
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.
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; }