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>
---
 xen/arch/x86/efi/parse-mbi2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/efi/parse-mbi2.c b/xen/arch/x86/efi/parse-mbi2.c
index 55a1777483..cae26d5e10 100644
--- 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 &&
+            tag->size >= sizeof(*tag) &&
+            (const void *)tag + tag->size <= mbi_end &&
             tag->type != MULTIBOOT2_TAG_TYPE_END;
           tag = _p(ROUNDUP((unsigned long)tag + tag->size,
                    MULTIBOOT2_TAG_ALIGN)) )
-- 
2.34.1

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 ...

> +            tag->size >= sizeof(*tag) &&
> +            (const void *)tag + tag->size <= mbi_end &&

... here. The earlier logic subtracting pointers wasn't susceptible to such.

Jan

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

On 01.10.2024 10:41, Frediano Ziglio wrote:
> On Mon, Sep 30, 2024 at 4:55 PM Jan Beulich <jbeulich@suse.com> 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