On 10/17/24 17:02, Andrew Cooper wrote:
On 17/10/2024 6:02 pm, Daniel P. Smith wrote:
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f7ea482920ef..d8ee5741740a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -284,6 +284,8 @@ static struct boot_info *__init
multiboot_fill_boot_info(unsigned long mbi_p)
{
struct boot_info *bi = &xen_boot_info;
const multiboot_info_t *mbi = __va(mbi_p);
+ module_t *mods = __va(mbi->mods_addr);
+ unsigned int i;
bi->nr_modules = (mbi->flags & MBI_MODULES) ? mbi->mods_count : 0;
@@ -302,6 +304,13 @@ static struct boot_info *__init multiboot_fill_boot_info(unsigned long mbi_p)
bi->memmap_length = mbi->mmap_length;
}
+ /*
+ * Iterate over all modules, including the extra one which should have been
+ * reserved for Xen itself
+ */
+ for ( i = 0; i <= bi->nr_modules; i++ )
+ bi->mods[i].mod = &mods[i];
I'm afraid you've got a bug here. bi->nr_modules is unsanitised from
firmware at this point.
It's checked/clamped later in __start_xen(), but not before you've
potentially scribbled past the end of bi->mod[] in this loop.
I think we want to retain the warning from clamping (which needs to be
after printk() is set up, so after parsing the cmdline), so to
compensate I think you want:
i < ARRAY_SIZE(bi->mods) && i <= bi->nr_modules
as the loop condition here, and a note to this effect. I'm not sure
what I think about passing exactly 64 modules, and this interacting with
the Xen slot.
Completely agree. I think Alejandro was trying to call that out and I
missed his point. Will fix.
However, you also want to move part of "x86/boot: convert create_dom0 to
use boot info" into this patch.
Specifically, the conversion from sizeof(module_map)*8 to
MAX_NR_BOOTMODS, or there's also a latent bug that depends Xen being
compiled as 64bit (unsigned long vs MAX_NR_BOOTMODS).
You are right, we should truncate to MAX_NR_BOOTMODS at this point and
not later.
v/r,
dps
