Tag structure should contain at least the tag header.
Entire tag structure must be contained inside MBI2 data.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since v6:
- compare against total_size every time to avoid overflows.
---
 xen/arch/x86/efi/mbi2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/efi/mbi2.c b/xen/arch/x86/efi/mbi2.c
index 55a1777483..935f3ae5d0 100644
--- 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)) )
-- 
2.34.1

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

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