[v6,05/44] x86/boot: introduce struct boot_module

Commit Message

Daniel P. Smith Oct. 17, 2024, 5:02 p.m. UTC

This will introduce a new struct boot_module to provide a rich state
representation around modules provided by the boot loader. Support is for 64
boot modules, one held in reserve for Xen, and up to 63 can be provided by the
boot loader. The array of struct boot_modules will be accessible via a
reference held in struct boot_info.

A temporary `mod` parameter is included in struct boot_module to ease the
transition from using Multiboot v1 structures over to struct boot_module. Once
the transition is complete, the parameter will be dropped from the structure.

Signed-off-by: Daniel P. Smith <[e-mail]>
Reviewed-by: Jason Andryuk <[e-mail]>
---
Changes since v5:
- reword comment
---
 xen/arch/x86/include/asm/bootinfo.h | 10 ++++++++++
 xen/arch/x86/setup.c                |  9 +++++++++
 2 files changed, 19 insertions(+)

Comments

Andrew Cooper Oct. 17, 2024, 9:02 p.m. UTC | #1

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.


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).

~Andrew

Daniel P. Smith Oct. 18, 2024, 12:15 a.m. UTC | #2

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

Patch

diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index f75a7e72a700..d19473d8941e 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -8,8 +8,17 @@ 
 #ifndef __XEN_X86_BOOTINFO_H__
 #define __XEN_X86_BOOTINFO_H__
 
+#include <xen/multiboot.h>
 #include <xen/types.h>
 
+/* Max number of boot modules a bootloader can provide in addition to Xen */
+#define MAX_NR_BOOTMODS 63
+
+struct boot_module {
+    /* Transitionary only */
+    module_t *mod;
+};
+
 /*
  * Xen internal representation of information provided by the
  * bootloader/environment, or derived from the information.
@@ -22,6 +31,7 @@  struct boot_info {
     size_t memmap_length;
 
     unsigned int nr_modules;
+    struct boot_module mods[MAX_NR_BOOTMODS + 1];
 };
 
 #endif /* __XEN_X86_BOOTINFO_H__ */
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];
+
     return bi;
 }