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
