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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Tue, Sep 24, 2024 at 3:15 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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