Re: [PATCH v3 4/4] x86/boot: Improve MBI2 structure check

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/09/2024 11:28 am, Frediano Ziglio wrote:
> Tag structure should contain at least the tag header.
> Entire tag structure must be contained inside MBI2 data.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> ---
>  xen/arch/x86/efi/parse-mbi2.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/efi/parse-mbi2.c b/xen/arch/x86/efi/parse-mbi2.c
> index 6038f35b16..7efda8fab2 100644
> --- a/xen/arch/x86/efi/parse-mbi2.c
> +++ b/xen/arch/x86/efi/parse-mbi2.c
> @@ -22,6 +22,7 @@ efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t 
> *mbi)
>      EFI_HANDLE ImageHandle = NULL;
>      EFI_SYSTEM_TABLE *SystemTable = NULL;
>      const char *cmdline = NULL;
> +    const void *const mbi_end = (const void *)mbi + mbi->total_size;
>      bool have_bs = false;
>  
>      if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC )
> @@ -30,7 +31,9 @@ efi_parse_mbi2(uint32_t magic, const multiboot2_fixed_t 
> *mbi)
>      /* Skip Multiboot2 information fixed part. */
>      tag = _p(ROUNDUP((unsigned long)(mbi + 1), MULTIBOOT2_TAG_ALIGN));
>  
> -    for ( ; (const void *)tag - (const void *)mbi < mbi->total_size
> +    for ( ; (const void *)(tag + 1) <= mbi_end
> +            && tag->size >= sizeof(*tag)
> +            && (const void *)tag + tag->size <= mbi_end
>              && tag->type != MULTIBOOT2_TAG_TYPE_END;
>            tag = _p(ROUNDUP((unsigned long)((const void *)tag + tag->size),
>                     MULTIBOOT2_TAG_ALIGN)) )

I'd merge this into the previous patch.  There's no reason to keep it
separate.

Also a minor style note I forgot, &&'s on the end of the previous line
please.

~Andrew