On 01.10.2024 10:41, Frediano Ziglio wrote: > On Mon, Sep 30, 2024 at 4:55 PM Jan Beulich <[email protected]> wrote: >> >> On 26.09.2024 11:21, Frediano Ziglio wrote: >>> --- a/xen/arch/x86/efi/parse-mbi2.c >>> +++ b/xen/arch/x86/efi/parse-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_end = (const void *)mbi + mbi->total_size; >>> 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_end && >> >> It may be possible to argue away overflow concerns here. I'm not so sure >> though >> for the case ... > > Do you mean tag + 1 ?
Yes. > For the same reason, computing tag above could have an overflow. > If the caller pass invalid data range, we will have overflows in either case. Indeed. Yet as said, for these "+ 1" it may be okay to keep them like that; the check below is certainly more risky. >>> + tag->size >= sizeof(*tag) && >>> + (const void *)tag + tag->size <= mbi_end && >> >> ... here. The earlier logic subtracting pointers wasn't susceptible to such. > > Can you suggest a change ? Keep comparing (pointer differences) against mbi->total_size. Jan
