[PATCH v8 0/2] x86/boot: Improve MBI2 structure check (was: Reduce assembly code)

[PATCH v8 0/2] x86/boot: Improve MBI2 structure check (was: Reduce assembly code)

[Frediano Ziglio]

This series came from part of the work of removing duplications between
boot code and rewriting part of code from assembly to C.

Changes since v1, more details in specific commits:
- style updates;
- comments and descriptions improvements;
- other improvements.

Changes since v2:
- rebased on master, resolved conflicts;
- add comment on trampoline section.

Changes since v3:
- changed new function name;
- declare efi_multiboot2 in a separate header;
- distinguish entry point from using magic number;
- other minor changes (see commens in commits).

Changes since v4:
- rebase on staging;
- set %fs and %gs as other segment registers;
- style and other changes.

Changes since v5:
- fixed a typo.

Changes since v6:
- remove merged patch;
- comment and style;
- change some pointer checks to avoid overflows;
- rename parse-mbi2.c to mbi2.c.

Changes since v7:
- removed merged parts;
- add required stack alignment.

Frediano Ziglio (2):
  x86/boot: Align mbi2.c stack to 16 bytes
  x86/boot: Improve MBI2 structure check

 xen/arch/x86/efi/Makefile | 2 ++
 xen/arch/x86/efi/mbi2.c   | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v8 1/2] x86/boot: Align mbi2.c stack to 16 bytes

[Frediano Ziglio]

Doing previous testing with an Adler Lake Intel machine the following
patch (improving MBI structure checking) started to fail.
Excluding it makes the tests succeed however there was not apparent
reason (looking at the code) for the failure.
So I instrumented code to output the structure and tested code with
this extracted data with and without the following patch and results
were the same.
Compiled assembly code from lab was also fine beside not keeping
the 16-byte alignment for the stack.
Turning on stack alignment solve the problem on Adler Lake machine.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/efi/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 7e2b5c07de..c2cad86856 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -11,6 +11,8 @@ $(obj)/boot.init.o: $(obj)/buildid.o
 $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
 $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-boundary)
 
+$(obj)/mbi2.o: CFLAGS_stack_boundary := $(cflags-stack-boundary)
+
 obj-y := common-stub.o stub.o
 obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))
 obj-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y))
-- 
2.34.1

Re: [PATCH v8 1/2] x86/boot: Align mbi2.c stack to 16 bytes

[Jan Beulich]

On 09.10.2024 10:04, Frediano Ziglio wrote:
> Doing previous testing with an Adler Lake Intel machine the following
> patch (improving MBI structure checking) started to fail.

In patch descriptions please don't refer to "this patch" or "the following
patch"; describe a commit in a self-contained way, with references to
what's already committed mentioning commit hash and title, whereas
references to what hasn't been committed using merely the title (and maybe
a link to its most recent posting). I'm not sure though that the other
patch really matters here beyond having exposed an issue that was there
(latently) anyway.

> Excluding it makes the tests succeed however there was not apparent
> reason (looking at the code) for the failure.
> So I instrumented code to output the structure and tested code with
> this extracted data with and without the following patch and results
> were the same.
> Compiled assembly code from lab was also fine beside not keeping
> the 16-byte alignment for the stack.
> Turning on stack alignment solve the problem on Adler Lake machine.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>

This really wants a Fixes: tag then.

> --- a/xen/arch/x86/efi/Makefile
> +++ b/xen/arch/x86/efi/Makefile
> @@ -11,6 +11,8 @@ $(obj)/boot.init.o: $(obj)/buildid.o
>  $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
>  $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-boundary)
>  
> +$(obj)/mbi2.o: CFLAGS_stack_boundary := $(cflags-stack-boundary)
> +
>  obj-y := common-stub.o stub.o
>  obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))
>  obj-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y))

You're duplicating code, which is better to avoid when possible. Is there
a reason the earlier commit didn't simply add mbi2.o to $(EFIOBJ-y)? That
way the existing logic would have covered that file as well. And really I
think it should have been mbi2.init.o (or else adding it into $(obj-bin-y)
is wrong), which probably wants correcting at the same time (ISTR actually
having requested that during an earlier review round).

Jan

Re: [PATCH v8 1/2] x86/boot: Align mbi2.c stack to 16 bytes

[Frediano Ziglio]

On Wed, Oct 9, 2024 at 9:20 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.10.2024 10:04, Frediano Ziglio wrote:
> > Doing previous testing with an Adler Lake Intel machine the following
> > patch (improving MBI structure checking) started to fail.
>
> In patch descriptions please don't refer to "this patch" or "the following
> patch"; describe a commit in a self-contained way, with references to
> what's already committed mentioning commit hash and title, whereas
> references to what hasn't been committed using merely the title (and maybe
> a link to its most recent posting). I'm not sure though that the other
> patch really matters here beyond having exposed an issue that was there
> (latently) anyway.
>

In this case it's referring to a not merged commit, so I cannot put
the hash, but I changed to state the subject.

> > Excluding it makes the tests succeed however there was not apparent
> > reason (looking at the code) for the failure.
> > So I instrumented code to output the structure and tested code with
> > this extracted data with and without the following patch and results
> > were the same.
> > Compiled assembly code from lab was also fine beside not keeping
> > the 16-byte alignment for the stack.
> > Turning on stack alignment solve the problem on Adler Lake machine.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
>
> This really wants a Fixes: tag then.
>

Done.

> > --- a/xen/arch/x86/efi/Makefile
> > +++ b/xen/arch/x86/efi/Makefile
> > @@ -11,6 +11,8 @@ $(obj)/boot.init.o: $(obj)/buildid.o
> >  $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
> >  $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-boundary)
> >
> > +$(obj)/mbi2.o: CFLAGS_stack_boundary := $(cflags-stack-boundary)
> > +
> >  obj-y := common-stub.o stub.o
> >  obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))
> >  obj-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y))
>
> You're duplicating code, which is better to avoid when possible. Is there
> a reason the earlier commit didn't simply add mbi2.o to $(EFIOBJ-y)? That
> way the existing logic would have covered that file as well. And really I
> think it should have been mbi2.init.o (or else adding it into $(obj-bin-y)
> is wrong), which probably wants correcting at the same time (ISTR actually
> having requested that during an earlier review round).
 >
> Jan

This was my first attempt, but it fails poorly, as EFIOBJ-y comes with
the addition of creating some file links that causes mbi2.c to be
overridden.
If I remember, you suggested changing to obj-bin-y. Still, maybe is
not the best place. It was added to obj-bin-y because it should be
included either if XEN_BUILD_EFI is "y" or not.

Frediano

Re: [PATCH v8 1/2] x86/boot: Align mbi2.c stack to 16 bytes

[Jan Beulich]

On 09.10.2024 12:15, Frediano Ziglio wrote:
> On Wed, Oct 9, 2024 at 9:20 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 09.10.2024 10:04, Frediano Ziglio wrote:
>>> --- a/xen/arch/x86/efi/Makefile
>>> +++ b/xen/arch/x86/efi/Makefile
>>> @@ -11,6 +11,8 @@ $(obj)/boot.init.o: $(obj)/buildid.o
>>>  $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
>>>  $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-boundary)
>>>
>>> +$(obj)/mbi2.o: CFLAGS_stack_boundary := $(cflags-stack-boundary)
>>> +
>>>  obj-y := common-stub.o stub.o
>>>  obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))
>>>  obj-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y))
>>
>> You're duplicating code, which is better to avoid when possible. Is there
>> a reason the earlier commit didn't simply add mbi2.o to $(EFIOBJ-y)? That
>> way the existing logic would have covered that file as well. And really I
>> think it should have been mbi2.init.o (or else adding it into $(obj-bin-y)
>> is wrong), which probably wants correcting at the same time (ISTR actually
>> having requested that during an earlier review round).
> 
> This was my first attempt, but it fails poorly, as EFIOBJ-y comes with
> the addition of creating some file links that causes mbi2.c to be
> overridden.

I can't see $(EFIOBJ-y) affecting symlink creation. What I can see is that
the variable is used in the setting of clean-files, which indeed is a problem.
Still imo the solution then is to introduce another variable to substitute the
uses of $(EFIOBJ-y) in arch/x86/efi/Makefile. E.g.

EFIOBJ-all := $(EFIOBJ-y) mbi2.init.o

> If I remember, you suggested changing to obj-bin-y. Still, maybe is
> not the best place. It was added to obj-bin-y because it should be
> included either if XEN_BUILD_EFI is "y" or not.

No, that doesn't explain the addition to obj-bin-y; this would equally be
achieved by adding to obj-y. The difference between the two variables is
whether objects are to be subject to LTO. And the typical case then is that
init-only objects aren't worth that extra build overhead. Hence the common
pattern is (besides files with assembly sources) for *.init.o to be added to
obj-bin-*.

Jan

Re: [PATCH v8 1/2] x86/boot: Align mbi2.c stack to 16 bytes

[Frediano Ziglio]

On Wed, Oct 9, 2024 at 12:13 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.10.2024 12:15, Frediano Ziglio wrote:
> > On Wed, Oct 9, 2024 at 9:20 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 09.10.2024 10:04, Frediano Ziglio wrote:
> >>> --- a/xen/arch/x86/efi/Makefile
> >>> +++ b/xen/arch/x86/efi/Makefile
> >>> @@ -11,6 +11,8 @@ $(obj)/boot.init.o: $(obj)/buildid.o
> >>>  $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
> >>>  $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-boundary)
> >>>
> >>> +$(obj)/mbi2.o: CFLAGS_stack_boundary := $(cflags-stack-boundary)
> >>> +
> >>>  obj-y := common-stub.o stub.o
> >>>  obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))
> >>>  obj-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y))
> >>
> >> You're duplicating code, which is better to avoid when possible. Is there
> >> a reason the earlier commit didn't simply add mbi2.o to $(EFIOBJ-y)? That
> >> way the existing logic would have covered that file as well. And really I
> >> think it should have been mbi2.init.o (or else adding it into $(obj-bin-y)
> >> is wrong), which probably wants correcting at the same time (ISTR actually
> >> having requested that during an earlier review round).
> >
> > This was my first attempt, but it fails poorly, as EFIOBJ-y comes with
> > the addition of creating some file links that causes mbi2.c to be
> > overridden.
>
> I can't see $(EFIOBJ-y) affecting symlink creation. What I can see is that
> the variable is used in the setting of clean-files, which indeed is a problem.
> Still imo the solution then is to introduce another variable to substitute the
> uses of $(EFIOBJ-y) in arch/x86/efi/Makefile. E.g.
>
> EFIOBJ-all := $(EFIOBJ-y) mbi2.init.o
>

what about simply

diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 7e2b5c07de..f2ce739f57 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -9,7 +9,7 @@ $(obj)/%.o: $(src)/%.ihex FORCE
$(obj)/boot.init.o: $(obj)/buildid.o

$(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
-$(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary :=
$(cflags-stack-boundary)
+$(addprefix $(obj)/,$(EFIOBJ-y) mbi2.o): CFLAGS_stack_boundary :=
$(cflags-stack-boundary)

obj-y := common-stub.o stub.o
obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))


> > If I remember, you suggested changing to obj-bin-y. Still, maybe is
> > not the best place. It was added to obj-bin-y because it should be
> > included either if XEN_BUILD_EFI is "y" or not.
>
> No, that doesn't explain the addition to obj-bin-y; this would equally be
> achieved by adding to obj-y. The difference between the two variables is
> whether objects are to be subject to LTO. And the typical case then is that
> init-only objects aren't worth that extra build overhead. Hence the common
> pattern is (besides files with assembly sources) for *.init.o to be added to
> obj-bin-*.
>

Then I would stick to obj-bin-y.

Frediano

Re: [PATCH v8 1/2] x86/boot: Align mbi2.c stack to 16 bytes

[Jan Beulich]

On 10.10.2024 10:34, Frediano Ziglio wrote:
> On Wed, Oct 9, 2024 at 12:13 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 09.10.2024 12:15, Frediano Ziglio wrote:
>>> On Wed, Oct 9, 2024 at 9:20 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 09.10.2024 10:04, Frediano Ziglio wrote:
>>>>> --- a/xen/arch/x86/efi/Makefile
>>>>> +++ b/xen/arch/x86/efi/Makefile
>>>>> @@ -11,6 +11,8 @@ $(obj)/boot.init.o: $(obj)/buildid.o
>>>>>  $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
>>>>>  $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-boundary)
>>>>>
>>>>> +$(obj)/mbi2.o: CFLAGS_stack_boundary := $(cflags-stack-boundary)
>>>>> +
>>>>>  obj-y := common-stub.o stub.o
>>>>>  obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))
>>>>>  obj-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y))
>>>>
>>>> You're duplicating code, which is better to avoid when possible. Is there
>>>> a reason the earlier commit didn't simply add mbi2.o to $(EFIOBJ-y)? That
>>>> way the existing logic would have covered that file as well. And really I
>>>> think it should have been mbi2.init.o (or else adding it into $(obj-bin-y)
>>>> is wrong), which probably wants correcting at the same time (ISTR actually
>>>> having requested that during an earlier review round).
>>>
>>> This was my first attempt, but it fails poorly, as EFIOBJ-y comes with
>>> the addition of creating some file links that causes mbi2.c to be
>>> overridden.
>>
>> I can't see $(EFIOBJ-y) affecting symlink creation. What I can see is that
>> the variable is used in the setting of clean-files, which indeed is a problem.
>> Still imo the solution then is to introduce another variable to substitute the
>> uses of $(EFIOBJ-y) in arch/x86/efi/Makefile. E.g.
>>
>> EFIOBJ-all := $(EFIOBJ-y) mbi2.init.o
>>
> 
> what about simply
> 
> diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
> index 7e2b5c07de..f2ce739f57 100644
> --- a/xen/arch/x86/efi/Makefile
> +++ b/xen/arch/x86/efi/Makefile
> @@ -9,7 +9,7 @@ $(obj)/%.o: $(src)/%.ihex FORCE
> $(obj)/boot.init.o: $(obj)/buildid.o
> 
> $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
> -$(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary :=
> $(cflags-stack-boundary)
> +$(addprefix $(obj)/,$(EFIOBJ-y) mbi2.o): CFLAGS_stack_boundary :=
> $(cflags-stack-boundary)
> 
> obj-y := common-stub.o stub.o
> obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))

Yes, but see below for the other adjustment to make.

>>> If I remember, you suggested changing to obj-bin-y. Still, maybe is
>>> not the best place. It was added to obj-bin-y because it should be
>>> included either if XEN_BUILD_EFI is "y" or not.
>>
>> No, that doesn't explain the addition to obj-bin-y; this would equally be
>> achieved by adding to obj-y. The difference between the two variables is
>> whether objects are to be subject to LTO. And the typical case then is that
>> init-only objects aren't worth that extra build overhead. Hence the common
>> pattern is (besides files with assembly sources) for *.init.o to be added to
>> obj-bin-*.
> 
> Then I would stick to obj-bin-y.

Correct, yet it wants to be mbi2.init.o there.

Jan

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

[Frediano Ziglio]

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.

Changes since v7:
- check for tag type being MULTIBOOT2_TAG_TYPE_END before checking
  tag size. This allows caller to not fill full tag (type field is
  before size field).
---
 xen/arch/x86/efi/mbi2.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/efi/mbi2.c b/xen/arch/x86/efi/mbi2.c
index 55a1777483..d2f0f0ee61 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,8 +22,10 @@ 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 &&
-            tag->type != MULTIBOOT2_TAG_TYPE_END;
+    for ( ; (const void *)(tag + 1) - mbi_raw <= mbi->total_size &&
+            tag->type != MULTIBOOT2_TAG_TYPE_END &&
+            tag->size >= sizeof(*tag) &&
+            (const void *)tag + tag->size - mbi_raw <= mbi->total_size;
           tag = _p(ROUNDUP((unsigned long)tag + tag->size,
                    MULTIBOOT2_TAG_ALIGN)) )
     {
-- 
2.34.1

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

[Jan Beulich]

On 09.10.2024 10:04, 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>