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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Tue, Oct 1, 2024 at 5:02 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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

Hi,
  personally, I don't care much about checking for overflows here.
It's not that we are in a higher security level, and we need to check
malicious intentions (like when user calls the kernel or a VM calls
the hypervisor), if the loader (which is at the same security level)
is passing garbage we are going to crash.

Saying that with this commit Marek checks are failing, I was thinking
about dropping this commit completely. Yes, in theory better checks
are better, but if we cause regression and boot failures maybe it's
better to allow some wrong data. That's one reason I wanted to keep
this commit separate from the other, which is just translating what
assembly code was doing, not introducing any regression (good or bad)
in behavior. I think it would be worth investigating what Marek
machine is passing in order to make sure we accept whatever wrong data
we are receiving (or understanding why more checks cause the issue).

Frediano