Re: [PATCH v6 3/3] x86/boot: Improve MBI2 structure check

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Sep 30, 2024 at 4:55 PM Jan Beulich <jbeulich@xxxxxxxx> 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 ?
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.

> > +            tag->size >= sizeof(*tag) &&
> > +            (const void *)tag + tag->size <= mbi_end &&
>
> ... here. The earlier logic subtracting pointers wasn't susceptible to such.
>
> Jan

Can you suggest a change ?

Frediano