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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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