[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 05/44] x86/boot: introduce struct boot_module
- To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
- Date: Thu, 17 Oct 2024 20:15:07 -0400
- Arc-authentication-results: i=1; mx.zohomail.com; dkim=pass header.i=apertussolutions.com; spf=pass smtp.mailfrom=dpsmith@xxxxxxxxxxxxxxxxxxxx; dmarc=pass header.from=<dpsmith@xxxxxxxxxxxxxxxxxxxx>
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1729210509; h=Content-Type:Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To; bh=2WBY1d784sRwXYzUn8u8sRCxIsLl8d/a1Fy5wqmbP/w=; b=UdN2ZxMNMoWSpDO76qjAf6t3zyWb9SOq1VYJD1G2mtuGupZYG+wXICQ2GjV2RIm5k8JqgZ8hpYyrlfOeFN90drbrT+Th30EQTwyhcKzJ+rhiRDaHE4aczGsfQCXn9zGaXdk5KGUMpSQp0ijkAKpD1mi9IAsmZ/tpiol5KmTc4bU=
- Arc-seal: i=1; a=rsa-sha256; t=1729210509; cv=none; d=zohomail.com; s=zohoarc; b=S04kpg88lILvWbNs1dVe9R8mwkeIL1TJb3O4IQtHjZMIhVLVil64DUd0DuOKYP+q87GbhOI0y/N7L5UCR5yNIhygXtVS6V2wpmHeotCS73+zBfn8zG3p9IbYg0gKrHUgwpZoFQ+wJggBIuk06t5oiafGtM24a4LSJmod6OVVqJs=
- Cc: jason.andryuk@xxxxxxx, christopher.w.clark@xxxxxxxxx, stefano.stabellini@xxxxxxx, Jan Beulich <jbeulich@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
- Delivery-date: Fri, 18 Oct 2024 00:15:24 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
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
|
