Re: [PATCH v7 2/2] x86/boot: Improve MBI2 structure check

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2024 12:22, Frediano Ziglio wrote:
> --- a/xen/arch/x86/efi/mbi2.c
> +++ b/xen/arch/x86/efi/mbi2.c
> @@ -13,6 +13,7 @@ efi_multiboot2_prelude(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_raw = (const void *)mbi;
>      bool have_bs = false;
>  
>      if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC )
> @@ -21,7 +22,9 @@ efi_multiboot2_prelude(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_raw <= mbi->total_size &&
> +            tag->size >= sizeof(*tag) &&
> +            (const void *)tag + tag->size - mbi_raw <= mbi->total_size &&
>              tag->type != MULTIBOOT2_TAG_TYPE_END;
>            tag = _p(ROUNDUP((unsigned long)tag + tag->size,
>                     MULTIBOOT2_TAG_ALIGN)) )

Hmm, looks like what I said on the earlier version still wasn't unambiguous
enough; I'm sorry. There is still potential for intermediate overflows in
the calculations. _If_ we care about avoiding overflows, we need to avoid
all of that. Even more so that Misra may not like this sort of pointer
calculations. You know tag >= mbi_raw, so tag - mbi_raw is always valid to
calculate. tag->size can further be checked to be less than mbi->total_size,
at which point mbi->total_size - tag->size can also be calculated without
risking {over,under}flow. (Similar then for the earlier (tag + 1) check.)

Jan