diff mbox series

[v3,4/4] x86/boot: Improve MBI2 structure check

Message ID 20240924102811.86884-5-frediano.ziglio@cloud.com (mailing list archive)
State Superseded
Headers show
Series x86/boot: Reduce assembly code | expand

Commit Message

Frediano Ziglio Sept. 24, 2024, 10:28 a.m. UTC 
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(-)

Comments

Andrew Cooper Sept. 24, 2024, 2:15 p.m. UTC | #1 
On 24/09/2024 11:28 am, Frediano Ziglio wrote:
> 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 6038f35b16..7efda8fab2 100644
> --- a/xen/arch/x86/efi/parse-mbi2.c
> +++ b/xen/arch/x86/efi/parse-mbi2.c
> @@ -22,6 +22,7 @@ efi_parse_mbi2(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 )
> @@ -30,7 +31,9 @@ efi_parse_mbi2(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)((const void *)tag + tag->size),
>                     MULTIBOOT2_TAG_ALIGN)) )

I'd merge this into the previous patch.  There's no reason to keep it
separate.

Also a minor style note I forgot, &&'s on the end of the previous line
please.

~Andrew
Frediano Ziglio Sept. 24, 2024, 2:20 p.m. UTC | #2 
On Tue, Sep 24, 2024 at 3:15 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 24/09/2024 11:28 am, Frediano Ziglio wrote:
> > 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 6038f35b16..7efda8fab2 100644
> > --- a/xen/arch/x86/efi/parse-mbi2.c
> > +++ b/xen/arch/x86/efi/parse-mbi2.c
> > @@ -22,6 +22,7 @@ efi_parse_mbi2(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 )
> > @@ -30,7 +31,9 @@ efi_parse_mbi2(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)((const void *)tag + tag->size),
> >                     MULTIBOOT2_TAG_ALIGN)) )
>
> I'd merge this into the previous patch.  There's no reason to keep it
> separate.
>
> Also a minor style note I forgot, &&'s on the end of the previous line
> please.
>

The reason is that the rationale is different.
The previous patch is converting assembly to C, this is improving.
Merging would make the conversion point void.

Frediano
diff mbox series

Patch

diff --git a/xen/arch/x86/efi/parse-mbi2.c b/xen/arch/x86/efi/parse-mbi2.c
index 6038f35b16..7efda8fab2 100644
--- a/xen/arch/x86/efi/parse-mbi2.c
+++ b/xen/arch/x86/efi/parse-mbi2.c
@@ -22,6 +22,7 @@  efi_parse_mbi2(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 )
@@ -30,7 +31,9 @@  efi_parse_mbi2(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)((const void *)tag + tag->size),
                    MULTIBOOT2_TAG_ALIGN)) )