[PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

With the current ASM_FUNC() macro, there is no good way to declare an
alignment constraint for a function. As ASM_FUNC() switches sections,
declaring the constraint before the macro invocation applies it to the
current location in the previous section. Declaring the constraint after
the macro invocation lets the function label point to the location prior
to alignment. Depending on toolchain behaviour, this may cause the label
to point to alignment padding preceding the actual function definition.

To address these issues, introduce the ASM_FUNC_ALIGN() macro, which
declares the alignment constraint right before the function label.

Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
---
 ArmPkg/Include/AsmMacroIoLibV8.h | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/ArmPkg/Include/AsmMacroIoLibV8.h b/ArmPkg/Include/AsmMacroIoLibV8.h
index 135aaeca5d0b..919edc70384d 100644
--- a/ArmPkg/Include/AsmMacroIoLibV8.h
+++ b/ArmPkg/Include/AsmMacroIoLibV8.h
@@ -34,15 +34,29 @@
         cbnz   SAFE_XREG, 1f        ;\
         b      .                    ;// We should never get here
 
-#define _ASM_FUNC(Name, Section)    \
-  .global   Name                  ; \
-  .section  #Section, "ax"        ; \
-  .type     Name, %function       ; \
+#define _ASM_FUNC_HDR(Name, Section) \
+  .global   Name                   ; \
+  .section  #Section, "ax"         ; \
+  .type     Name, %function
+
+#define _ASM_FUNC_FTR(Name)         \
   Name:                           ; \
   AARCH64_BTI(c)
 
+#define _ASM_FUNC(Name, Section)    \
+  _ASM_FUNC_HDR(Name, Section)    ; \
+  _ASM_FUNC_FTR(Name)
+
+#define _ASM_FUNC_ALIGN(Name, Section, Align)       \
+  _ASM_FUNC_HDR(Name, Section)                    ; \
+  .balign Align                                   ; \
+  _ASM_FUNC_FTR(Name)
+
 #define ASM_FUNC(Name)  _ASM_FUNC(ASM_PFX(Name), .text. ## Name)
 
+#define ASM_FUNC_ALIGN(Name, Align)  \
+  _ASM_FUNC_ALIGN(ASM_PFX(Name), .text. ## Name, Align)
+
 #define MOV32(Reg, Val)                   \
   movz      Reg, (Val) >> 16, lsl #16   ; \
   movk      Reg, (Val) & 0xffff
-- 
2.40.0

[PATCH 2/2] ArmPkg/ArmMmuLib: Fix ArmReplaceLiveTranslationEntry() alignment

[Marvin Häuser]

As the ASM_FUNC() macro performs a section switch, the preceding
.balign directive applies the alignment constraint to the current
location in the previous section. As the linker may not merge the
sections in-order, ArmReplaceLiveTranslationEntry() may be left
unaligned.

Replace the explicit invocation of .balign with the ASM_FUNC_ALIGN()
macro, which guarantees the alignment constraint is applied correctly.

Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
index e936a5be4e11..7627aeb95464 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
@@ -69,17 +69,16 @@
 .L2_\@:
   .endm
 
-  // Align this routine to a log2 upper bound of its size, so that it is
-  // guaranteed not to cross a page or block boundary.
-  .balign 0x200
-
 //VOID
 //ArmReplaceLiveTranslationEntry (
 //  IN  UINT64  *Entry,
 //  IN  UINT64  Value,
 //  IN  UINT64  Address
 //  )
-ASM_FUNC(ArmReplaceLiveTranslationEntry)
+//
+// Align this routine to a log2 upper bound of its size, so that it is
+// guaranteed not to cross a page or block boundary.
+ASM_FUNC_ALIGN(ArmReplaceLiveTranslationEntry, 0x200)
 
   // disable interrupts
   mrs   x4, daif
-- 
2.40.0

Re: [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Leif Lindholm]

Hi Marvin,

First of all - many thanks for tracking down the bug that creates the
need for this.

On Mon, Apr 17, 2023 at 18:09:15 +0000, Marvin Häuser wrote:
> With the current ASM_FUNC() macro, there is no good way to declare an
> alignment constraint for a function. As ASM_FUNC() switches sections,
> declaring the constraint before the macro invocation applies it to the
> current location in the previous section. Declaring the constraint after
> the macro invocation lets the function label point to the location prior
> to alignment. Depending on toolchain behaviour, this may cause the label
> to point to alignment padding preceding the actual function definition.
> 
> To address these issues, introduce the ASM_FUNC_ALIGN() macro, which
> declares the alignment constraint right before the function label.
> 
> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> ---
>  ArmPkg/Include/AsmMacroIoLibV8.h | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmPkg/Include/AsmMacroIoLibV8.h b/ArmPkg/Include/AsmMacroIoLibV8.h
> index 135aaeca5d0b..919edc70384d 100644
> --- a/ArmPkg/Include/AsmMacroIoLibV8.h
> +++ b/ArmPkg/Include/AsmMacroIoLibV8.h
> @@ -34,15 +34,29 @@
>          cbnz   SAFE_XREG, 1f        ;\
>          b      .                    ;// We should never get here
>  
> -#define _ASM_FUNC(Name, Section)    \
> -  .global   Name                  ; \
> -  .section  #Section, "ax"        ; \
> -  .type     Name, %function       ; \
> +#define _ASM_FUNC_HDR(Name, Section) \
> +  .global   Name                   ; \
> +  .section  #Section, "ax"         ; \
> +  .type     Name, %function
> +
> +#define _ASM_FUNC_FTR(Name)         \
>    Name:                           ; \
>    AARCH64_BTI(c)
>  
> +#define _ASM_FUNC(Name, Section)    \
> +  _ASM_FUNC_HDR(Name, Section)    ; \
> +  _ASM_FUNC_FTR(Name)
> +
> +#define _ASM_FUNC_ALIGN(Name, Section, Align)       \

I like this solution, but I'd like to hear Ard's opinion.

I probably want to bikeshed some of the implementation details:
Although I generally dislike duplicate definitions, I think I would
prefer having _ASM_FUNC and _ASM_FUNC_ALIGN defined self-contained,
without _HDR and _FTR.
If we do keep the reused primitives, we need better language; the
footer of the header is not a footer of the function.

/
    Leif

> +  _ASM_FUNC_HDR(Name, Section)                    ; \
> +  .balign Align                                   ; \
> +  _ASM_FUNC_FTR(Name)
> +
>  #define ASM_FUNC(Name)  _ASM_FUNC(ASM_PFX(Name), .text. ## Name)
>  
> +#define ASM_FUNC_ALIGN(Name, Align)  \
> +  _ASM_FUNC_ALIGN(ASM_PFX(Name), .text. ## Name, Align)
> +
>  #define MOV32(Reg, Val)                   \
>    movz      Reg, (Val) >> 16, lsl #16   ; \
>    movk      Reg, (Val) & 0xffff
> -- 
> 2.40.0
> 

Re: [PATCH 2/2] ArmPkg/ArmMmuLib: Fix ArmReplaceLiveTranslationEntry() alignment

[Leif Lindholm]

On Mon, Apr 17, 2023 at 18:09:16 +0000, Marvin Häuser wrote:
> As the ASM_FUNC() macro performs a section switch, the preceding
> .balign directive applies the alignment constraint to the current
> location in the previous section. As the linker may not merge the
> sections in-order, ArmReplaceLiveTranslationEntry() may be left
> unaligned.
> 
> Replace the explicit invocation of .balign with the ASM_FUNC_ALIGN()
> macro, which guarantees the alignment constraint is applied correctly.
> 
> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>

Assuming we reach consensus on the preceding patch, this one is a no
brainer:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> index e936a5be4e11..7627aeb95464 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> @@ -69,17 +69,16 @@
>  .L2_\@:
>    .endm
>  
> -  // Align this routine to a log2 upper bound of its size, so that it is
> -  // guaranteed not to cross a page or block boundary.
> -  .balign 0x200
> -
>  //VOID
>  //ArmReplaceLiveTranslationEntry (
>  //  IN  UINT64  *Entry,
>  //  IN  UINT64  Value,
>  //  IN  UINT64  Address
>  //  )
> -ASM_FUNC(ArmReplaceLiveTranslationEntry)
> +//
> +// Align this routine to a log2 upper bound of its size, so that it is
> +// guaranteed not to cross a page or block boundary.
> +ASM_FUNC_ALIGN(ArmReplaceLiveTranslationEntry, 0x200)
>  
>    // disable interrupts
>    mrs   x4, daif
> -- 
> 2.40.0
> 

Re: [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Ard Biesheuvel]

On Mon, 17 Apr 2023 at 21:52, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> Hi Marvin,
>
> First of all - many thanks for tracking down the bug that creates the
> need for this.
>
> On Mon, Apr 17, 2023 at 18:09:15 +0000, Marvin Häuser wrote:
> > With the current ASM_FUNC() macro, there is no good way to declare an
> > alignment constraint for a function. As ASM_FUNC() switches sections,
> > declaring the constraint before the macro invocation applies it to the
> > current location in the previous section. Declaring the constraint after
> > the macro invocation lets the function label point to the location prior
> > to alignment. Depending on toolchain behaviour, this may cause the label
> > to point to alignment padding preceding the actual function definition.
> >
> > To address these issues, introduce the ASM_FUNC_ALIGN() macro, which
> > declares the alignment constraint right before the function label.
> >
> > Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> > ---
> >  ArmPkg/Include/AsmMacroIoLibV8.h | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/ArmPkg/Include/AsmMacroIoLibV8.h b/ArmPkg/Include/AsmMacroIoLibV8.h
> > index 135aaeca5d0b..919edc70384d 100644
> > --- a/ArmPkg/Include/AsmMacroIoLibV8.h
> > +++ b/ArmPkg/Include/AsmMacroIoLibV8.h
> > @@ -34,15 +34,29 @@
> >          cbnz   SAFE_XREG, 1f        ;\
> >          b      .                    ;// We should never get here
> >
> > -#define _ASM_FUNC(Name, Section)    \
> > -  .global   Name                  ; \
> > -  .section  #Section, "ax"        ; \
> > -  .type     Name, %function       ; \
> > +#define _ASM_FUNC_HDR(Name, Section) \
> > +  .global   Name                   ; \
> > +  .section  #Section, "ax"         ; \
> > +  .type     Name, %function
> > +
> > +#define _ASM_FUNC_FTR(Name)         \
> >    Name:                           ; \
> >    AARCH64_BTI(c)
> >
> > +#define _ASM_FUNC(Name, Section)    \
> > +  _ASM_FUNC_HDR(Name, Section)    ; \
> > +  _ASM_FUNC_FTR(Name)
> > +
> > +#define _ASM_FUNC_ALIGN(Name, Section, Align)       \
>
> I like this solution, but I'd like to hear Ard's opinion.
>
> I probably want to bikeshed some of the implementation details:
> Although I generally dislike duplicate definitions, I think I would
> prefer having _ASM_FUNC and _ASM_FUNC_ALIGN defined self-contained,
> without _HDR and _FTR.
> If we do keep the reused primitives, we need better language; the
> footer of the header is not a footer of the function.
>

Agree with all of this.

And thanks for tracking this down - must not have been fun :-)

Re: [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

> On 17. Apr 2023, at 23:18, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> Agree with all of this.
> 
> And thanks for tracking this down - must not have been fun :-)

No worries - it wasn’t. :) It was mere luck Vitaly discovered early it was an issue that triggered based on the binary layout rather than a bug in the mapping code itself…

Speaking of not fun to track down, I initially wanted to add ASSERTs (yes, runtime :( ) to check the alignment guarantee is actually met. Leif basically declined any form of regression-testing at runtime. Do you happen to have a simple(!) idea for how it could be checked at build-time? (It’s less about “which commands do I run?” and more about integration with the build process / BaseTools, cross-OS compatibility, etc.)

Thanks!

Best regards,
Marvin

Re: [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Ard Biesheuvel]

On Tue, 18 Apr 2023 at 08:40, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 17. Apr 2023, at 23:18, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Agree with all of this.
> >
> > And thanks for tracking this down - must not have been fun :-)
>
> No worries - it wasn’t. :) It was mere luck Vitaly discovered early it was an issue that triggered based on the binary layout rather than a bug in the mapping code itself…
>
> Speaking of not fun to track down, I initially wanted to add ASSERTs (yes, runtime :( ) to check the alignment guarantee is actually met. Leif basically declined any form of regression-testing at runtime. Do you happen to have a simple(!) idea for how it could be checked at build-time? (It’s less about “which commands do I run?” and more about integration with the build process / BaseTools, cross-OS compatibility, etc.)
>


I think we should just add another align to the code:

.align xx
func:

< code >

.align xx
.org func + xx

That way, if the code straddles a xx-aligned boundary, the .org will
move backwards and force an error.

Re: [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

> On 18. Apr 2023, at 10:10, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Tue, 18 Apr 2023 at 08:40, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> 
>>> On 17. Apr 2023, at 23:18, Ard Biesheuvel <ardb@kernel.org> wrote:
>>> 
>>> Agree with all of this.
>>> 
>>> And thanks for tracking this down - must not have been fun :-)
>> 
>> No worries - it wasn’t. :) It was mere luck Vitaly discovered early it was an issue that triggered based on the binary layout rather than a bug in the mapping code itself…
>> 
>> Speaking of not fun to track down, I initially wanted to add ASSERTs (yes, runtime :( ) to check the alignment guarantee is actually met. Leif basically declined any form of regression-testing at runtime. Do you happen to have a simple(!) idea for how it could be checked at build-time? (It’s less about “which commands do I run?” and more about integration with the build process / BaseTools, cross-OS compatibility, etc.)
>> 
> 
> 
> I think we should just add another align to the code:
> 
> .align xx
> func:
> 
> < code >
> 
> .align xx
> .org func + xx
> 
> That way, if the code straddles a xx-aligned boundary, the .org will
> move backwards and force an error.

Wow, that's pretty fucking smart... I didn't even know that directive was a thing. I will try to toy with it soon. Do you want it as a separate series, separate commit in the current series, or squashed into the fix?

Re: [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Ard Biesheuvel]

On Tue, 18 Apr 2023 at 10:18, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
>
> > On 18. Apr 2023, at 10:10, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Tue, 18 Apr 2023 at 08:40, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>
> >>
> >>> On 17. Apr 2023, at 23:18, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>
> >>> Agree with all of this.
> >>>
> >>> And thanks for tracking this down - must not have been fun :-)
> >>
> >> No worries - it wasn’t. :) It was mere luck Vitaly discovered early it was an issue that triggered based on the binary layout rather than a bug in the mapping code itself…
> >>
> >> Speaking of not fun to track down, I initially wanted to add ASSERTs (yes, runtime :( ) to check the alignment guarantee is actually met. Leif basically declined any form of regression-testing at runtime. Do you happen to have a simple(!) idea for how it could be checked at build-time? (It’s less about “which commands do I run?” and more about integration with the build process / BaseTools, cross-OS compatibility, etc.)
> >>
> >
> >
> > I think we should just add another align to the code:
> >
> > .align xx
> > func:
> >
> > < code >
> >
> > .align xx
> > .org func + xx
> >
> > That way, if the code straddles a xx-aligned boundary, the .org will
> > move backwards and force an error.
>
> Wow, that's pretty fucking smart... I didn't even know that directive was a thing. I will try to toy with it soon. Do you want it as a separate series, separate commit in the current series, or squashed into the fix?

I think it makes sense to just fold this into the second patch.

Re: [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 2595 bytes --]

Hi all,

While testing Ard's suggestion for V3, I noticed I got a broken FD where ArmReplaceLiveTranslationEntry() is misaligned, but does not cross a 4 KB boundary. To not just hide the issue via this patch, can someone please try to explain the exact requirements this function has (the comments read like 0x200 was just the lowest value to guarantee staying within a page)? Why would it be broken if misaligned, but not crossing a page?

Is there any chance the FD is somehow misaligned in memory, thus shifting the function across a page in the process? Or is the FD mapped to a fixed address like with x86? Is code after ArmReplaceLiveTranslationEntry() crossing page boundaries the actual issue (and is implicitly fixed by aligning it)?

For reference, I attached the broken FD. The problematic ArmReplaceLiveTranslationEntry() is located at MemoryInitPei 0x25C10 - 0x25D54.
This is from my arm_corruption-latest branch: https://github.com/mhaeuser/edk2/tree/arm_corruption-latest

Best regards,
Marvin



> On 18. Apr 2023, at 10:18, Marvin Häuser <mhaeuser@posteo.de> wrote:
> 
> 
> 
>> On 18. Apr 2023, at 10:10, Ard Biesheuvel <ardb@kernel.org> wrote:
>> 
>> On Tue, 18 Apr 2023 at 08:40, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>> 
>>> 
>>>> On 17. Apr 2023, at 23:18, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>> 
>>>> Agree with all of this.
>>>> 
>>>> And thanks for tracking this down - must not have been fun :-)
>>> 
>>> No worries - it wasn’t. :) It was mere luck Vitaly discovered early it was an issue that triggered based on the binary layout rather than a bug in the mapping code itself…
>>> 
>>> Speaking of not fun to track down, I initially wanted to add ASSERTs (yes, runtime :( ) to check the alignment guarantee is actually met. Leif basically declined any form of regression-testing at runtime. Do you happen to have a simple(!) idea for how it could be checked at build-time? (It’s less about “which commands do I run?” and more about integration with the build process / BaseTools, cross-OS compatibility, etc.)
>>> 
>> 
>> 
>> I think we should just add another align to the code:
>> 
>> .align xx
>> func:
>> 
>> < code >
>> 
>> .align xx
>> .org func + xx
>> 
>> That way, if the code straddles a xx-aligned boundary, the .org will
>> move backwards and force an error.
> 
> Wow, that's pretty fucking smart... I didn't even know that directive was a thing. I will try to toy with it soon. Do you want it as a separate series, separate commit in the current series, or squashed into the fix?


[-- Attachment #2.1: Type: text/html, Size: 3436 bytes --]

[-- Attachment #2.2: QEMU_EFI.fd --]
[-- Type: application/octet-stream, Size: 2097152 bytes --]

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Ard Biesheuvel]

On Wed, 19 Apr 2023 at 19:14, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hi all,
>
> While testing Ard's suggestion for V3, I noticed I got a broken FD where ArmReplaceLiveTranslationEntry() is misaligned, but does not cross a 4 KB boundary.

Which platform are you building?

> To not just hide the issue via this patch, can someone please try to explain the exact requirements this function has (the comments read like 0x200 was just the lowest value to guarantee staying within a page)? Why would it be broken if misaligned, but not crossing a page?
>

0x200 is a log2 upper bound for the size of the function, so it's just
the smallest value that fits that requirement, determined manually
iirc

And the only reason we have this is that we can cheaply decide whether
or not unmapping a page will unmap this function or not, but we could
actually just use the address and size to decide this.

In any case, if the FD is constructed in a way that violates the
alignment, there is something wrong with the build tools you are
using.

> Is there any chance the FD is somehow misaligned in memory, thus shifting the function across a page in the process? Or is the FD mapped to a fixed address like with x86? Is code after ArmReplaceLiveTranslationEntry() crossing page boundaries the actual issue (and is implicitly fixed by aligning it)?
>

If you are building ArmVirtQemu.dsc, the FD is mapped at address 0x0
and the FV is mapped at 0x1000


> For reference, I attached the broken FD. The problematic ArmReplaceLiveTranslationEntry() is located at MemoryInitPei 0x25C10 - 0x25D54.
> This is from my arm_corruption-latest branch: https://github.com/mhaeuser/edk2/tree/arm_corruption-latest
>
> Best regards,
> Marvin
>
>
> On 18. Apr 2023, at 10:18, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
>
> On 18. Apr 2023, at 10:10, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 18 Apr 2023 at 08:40, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
>
> On 17. Apr 2023, at 23:18, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Agree with all of this.
>
> And thanks for tracking this down - must not have been fun :-)
>
>
> No worries - it wasn’t. :) It was mere luck Vitaly discovered early it was an issue that triggered based on the binary layout rather than a bug in the mapping code itself…
>
> Speaking of not fun to track down, I initially wanted to add ASSERTs (yes, runtime :( ) to check the alignment guarantee is actually met. Leif basically declined any form of regression-testing at runtime. Do you happen to have a simple(!) idea for how it could be checked at build-time? (It’s less about “which commands do I run?†and more about integration with the build process / BaseTools, cross-OS compatibility, etc.)
>
>
>
> I think we should just add another align to the code:
>
> .align xx
> func:
>
> < code >
>
> .align xx
> .org func + xx
>
> That way, if the code straddles a xx-aligned boundary, the .org will
> move backwards and force an error.
>
>
> Wow, that's pretty fucking smart... I didn't even know that directive was a thing. I will try to toy with it soon. Do you want it as a separate series, separate commit in the current series, or squashed into the fix?
>
>
> Attachments:
>
> QEMU_EFI.fd
>
> 

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 4064 bytes --]


> On 19. Apr 2023, at 19:40, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Wed, 19 Apr 2023 at 19:14, Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>> wrote:
>> 
>> Hi all,
>> 
>> While testing Ard's suggestion for V3, I noticed I got a broken FD where ArmReplaceLiveTranslationEntry() is misaligned, but does not cross a 4 KB boundary.
> 
> Which platform are you building?

ArmVirtPkg / AARCH64 / DEBUG / GCC5 (GCC 12.2.0).

> 
>> To not just hide the issue via this patch, can someone please try to explain the exact requirements this function has (the comments read like 0x200 was just the lowest value to guarantee staying within a page)? Why would it be broken if misaligned, but not crossing a page?
>> 
> 
> 0x200 is a log2 upper bound for the size of the function, so it's just
> the smallest value that fits that requirement, determined manually
> iirc
> 
> And the only reason we have this is that we can cheaply decide whether
> or not unmapping a page will unmap this function or not, but we could
> actually just use the address and size to decide this.
> 
> In any case, if the FD is constructed in a way that violates the
> alignment, there is something wrong with the build tools you are
> using.

The tools are stock edk2, the only changes made are those in the latest commit of the linked branch.

> 
>> Is there any chance the FD is somehow misaligned in memory, thus shifting the function across a page in the process? Or is the FD mapped to a fixed address like with x86? Is code after ArmReplaceLiveTranslationEntry() crossing page boundaries the actual issue (and is implicitly fixed by aligning it)?
>> 
> 
> If you are building ArmVirtQemu.dsc, the FD is mapped at address 0x0
> and the FV is mapped at 0x1000

Then the function simply is not crossing a page boundary... which means the patch did fix a valid bug, but it wasn't what actually caused the corruption. Any help is appreciated. :)

Best regards,
Marvin

> 
> 
>> For reference, I attached the broken FD. The problematic ArmReplaceLiveTranslationEntry() is located at MemoryInitPei 0x25C10 - 0x25D54.
>> This is from my arm_corruption-latest branch: https://github.com/mhaeuser/edk2/tree/arm_corruption-latest
>> 
>> Best regards,
>> Marvin
>> 
>> 
>> On 18. Apr 2023, at 10:18, Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>> wrote:
>> 
>> 
>> 
>> On 18. Apr 2023, at 10:10, Ard Biesheuvel <ardb@kernel.org <mailto:ardb@kernel.org>> wrote:
>> 
>> On Tue, 18 Apr 2023 at 08:40, Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>> wrote:
>> 
>> 
>> 
>> On 17. Apr 2023, at 23:18, Ard Biesheuvel <ardb@kernel.org <mailto:ardb@kernel.org>> wrote:
>> 
>> Agree with all of this.
>> 
>> And thanks for tracking this down - must not have been fun :-)
>> 
>> 
>> No worries - it wasn’t. :) It was mere luck Vitaly discovered early it was an issue that triggered based on the binary layout rather than a bug in the mapping code itself…
>> 
>> Speaking of not fun to track down, I initially wanted to add ASSERTs (yes, runtime :( ) to check the alignment guarantee is actually met. Leif basically declined any form of regression-testing at runtime. Do you happen to have a simple(!) idea for how it could be checked at build-time? (It’s less about “which commands do I run?†and more about integration with the build process / BaseTools, cross-OS compatibility, etc.)
>> 
>> 
>> 
>> I think we should just add another align to the code:
>> 
>> .align xx
>> func:
>> 
>> < code >
>> 
>> .align xx
>> .org func + xx
>> 
>> That way, if the code straddles a xx-aligned boundary, the .org will
>> move backwards and force an error.
>> 
>> 
>> Wow, that's pretty fucking smart... I didn't even know that directive was a thing. I will try to toy with it soon. Do you want it as a separate series, separate commit in the current series, or squashed into the fix?
>> 
>> 
>> Attachments:
>> 
>> QEMU_EFI.fd
>> 
>> 


[-- Attachment #2: Type: text/html, Size: 18750 bytes --]

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Ard Biesheuvel]

On Wed, 19 Apr 2023 at 19:45, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> On 19. Apr 2023, at 19:40, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 19 Apr 2023 at 19:14, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> Hi all,
>
> While testing Ard's suggestion for V3, I noticed I got a broken FD where ArmReplaceLiveTranslationEntry() is misaligned, but does not cross a 4 KB boundary.
>
>
> Which platform are you building?
>
>
> ArmVirtPkg / AARCH64 / DEBUG / GCC5 (GCC 12.2.0).
>
>
> To not just hide the issue via this patch, can someone please try to explain the exact requirements this function has (the comments read like 0x200 was just the lowest value to guarantee staying within a page)? Why would it be broken if misaligned, but not crossing a page?
>
>
> 0x200 is a log2 upper bound for the size of the function, so it's just
> the smallest value that fits that requirement, determined manually
> iirc
>
> And the only reason we have this is that we can cheaply decide whether
> or not unmapping a page will unmap this function or not, but we could
> actually just use the address and size to decide this.
>
> In any case, if the FD is constructed in a way that violates the
> alignment, there is something wrong with the build tools you are
> using.
>
>
> The tools are stock edk2, the only changes made are those in the latest commit of the linked branch.
>
>
> Is there any chance the FD is somehow misaligned in memory, thus shifting the function across a page in the process? Or is the FD mapped to a fixed address like with x86? Is code after ArmReplaceLiveTranslationEntry() crossing page boundaries the actual issue (and is implicitly fixed by aligning it)?
>
>
> If you are building ArmVirtQemu.dsc, the FD is mapped at address 0x0
> and the FV is mapped at 0x1000
>
>
> Then the function simply is not crossing a page boundary... which means the patch did fix a valid bug, but it wasn't what actually caused the corruption. Any help is appreciated. :)
>

Your branch seems to be missing 16e0969ef775b898ac700f3261d76030b8ab9ef0

"ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs"

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]


> On 19. Apr 2023, at 20:03, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> Your branch seems to be missing 16e0969ef775b898ac700f3261d76030b8ab9ef0
> 
> "ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs"

That's correct (because that commit is after the last commit I managed to reproduce the issue with), but I don't see how this commit would fix the issue. As I said, the symptom is that PeiCore memory is badly corrupted and the stall happens due to executing said corruption, not due to jumping to NULL. Those broken branches I linked can all be made work by rolling back the change to MemoryAllocationLib (which changes the code size, thus misaligns *something*). In fact, using the broken variant only for MemoryInitPei is sufficient to reproduce the issue, other modules don't seem to be involved.

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 2871 bytes --]

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Ard Biesheuvel]

On Wed, 19 Apr 2023 at 20:25, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> On 19. Apr 2023, at 20:03, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Your branch seems to be missing 16e0969ef775b898ac700f3261d76030b8ab9ef0
>
> "ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs"
>
>
> That's correct (because that commit is after the last commit I managed to reproduce the issue with), but I don't see how this commit would fix the issue. As I said, the symptom is that PeiCore memory is badly corrupted and the stall happens due to executing said corruption, not due to jumping to NULL. Those broken branches I linked can all be made work by rolling back the change to MemoryAllocationLib (which changes the code size, thus misaligns *something*). In fact, using the broken variant only for MemoryInitPei is sufficient to reproduce the issue, other modules don't seem to be involved.
>

Applying that commit made your branch work for me.

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]


> On 19. Apr 2023, at 20:26, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Wed, 19 Apr 2023 at 20:25, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> 
>> On 19. Apr 2023, at 20:03, Ard Biesheuvel <ardb@kernel.org> wrote:
>> 
>> Your branch seems to be missing 16e0969ef775b898ac700f3261d76030b8ab9ef0
>> 
>> "ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs"
>> 
>> 
>> That's correct (because that commit is after the last commit I managed to reproduce the issue with), but I don't see how this commit would fix the issue. As I said, the symptom is that PeiCore memory is badly corrupted and the stall happens due to executing said corruption, not due to jumping to NULL. Those broken branches I linked can all be made work by rolling back the change to MemoryAllocationLib (which changes the code size, thus misaligns *something*). In fact, using the broken variant only for MemoryInitPei is sufficient to reproduce the issue, other modules don't seem to be involved.
>> 
> 
> Applying that commit made your branch work for me.

Yes, that might very well be - applying ae2c904 also "fixes" the issue as per https://github.com/mhaeuser/edk2/tree/arm_corruption-earliest-fixed

And technically, so does reverting this line :) https://github.com/mhaeuser/edk2/commit/7a96986e024f9c7ccf4774cc6f2ddb47a3abc86e#diff-1edfe01abdf8e4dcac640db4d9436e17b5f15d037714df7f365b58fcfc91e425R409

I don't understand how the changes would *fix* (rather than hide) the issue, so I'd attribute it to lucky codegen that doesn't misalign whatever is misaligned. I unfortunately have absolutely no time to get back to debugging this. :(

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 2388 bytes --]

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Ard Biesheuvel]

On Wed, 19 Apr 2023 at 20:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> On 19. Apr 2023, at 20:26, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 19 Apr 2023 at 20:25, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
>
> On 19. Apr 2023, at 20:03, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Your branch seems to be missing 16e0969ef775b898ac700f3261d76030b8ab9ef0
>
> "ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs"
>
>
> That's correct (because that commit is after the last commit I managed to reproduce the issue with), but I don't see how this commit would fix the issue. As I said, the symptom is that PeiCore memory is badly corrupted and the stall happens due to executing said corruption, not due to jumping to NULL. Those broken branches I linked can all be made work by rolling back the change to MemoryAllocationLib (which changes the code size, thus misaligns *something*). In fact, using the broken variant only for MemoryInitPei is sufficient to reproduce the issue, other modules don't seem to be involved.
>
>
> Applying that commit made your branch work for me.
>
>
> Yes, that might very well be - applying ae2c904 also "fixes" the issue as per https://github.com/mhaeuser/edk2/tree/arm_corruption-earliest-fixed
>
> And technically, so does reverting this line :) https://github.com/mhaeuser/edk2/commit/7a96986e024f9c7ccf4774cc6f2ddb47a3abc86e#diff-1edfe01abdf8e4dcac640db4d9436e17b5f15d037714df7f365b58fcfc91e425R409
>
> I don't understand how the changes would *fix* (rather than hide) the issue, so I'd attribute it to lucky codegen that doesn't misalign whatever is misaligned. I unfortunately have absolutely no time to get back to debugging this. :(
>

The issue is likely caused by

-Wl,--defsym=PECOFF_HEADER_SIZE=0

Why are you setting that? It breaks the ELF to PE conversion.

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

> On 19. Apr 2023, at 21:48, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> The issue is likely caused by
> 
> -Wl,--defsym=PECOFF_HEADER_SIZE=0
> 
> Why are you setting that? It breaks the ELF to PE conversion.

Where?

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

> On 19. Apr 2023, at 22:10, Marvin Häuser <mhaeuser@posteo.de> wrote:
> 
> 
>> On 19. Apr 2023, at 21:48, Ard Biesheuvel <ardb@kernel.org> wrote:
>> 
>> The issue is likely caused by
>> 
>> -Wl,--defsym=PECOFF_HEADER_SIZE=0
>> 
>> Why are you setting that? It breaks the ELF to PE conversion.
> 
> Where?

AUDK doesn’t use that macro, so you must mean my edk2 branch. In that case - I don’t know, that’s upstream code that isn’t mine? :D

It’s only done for ASLDLINK, which should not interfere here. Still, ASLC does generate PE binaries, so I’m not sure what the deal is with setting it to 0.

Best regards,
Marvin

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Ard Biesheuvel]

On Wed, 19 Apr 2023 at 22:10, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 19. Apr 2023, at 21:48, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > The issue is likely caused by
> >
> > -Wl,--defsym=PECOFF_HEADER_SIZE=0
> >
> > Why are you setting that? It breaks the ELF to PE conversion.
>
> Where?

It would, but you only appear to be setting that for ASLD_DLINK_FLAGS,
right? So that seems unrelated.

The only thing I am observing is that the store to memory in
ArmMmuBaseLibConstructor()

  Hob = GetFirstGuidHob (&gArmMmuReplaceLiveTranslationEntryFuncGuid);
  if (Hob != NULL) {
    mReplaceLiveEntryFunc = *(VOID **)GET_GUID_HOB_DATA (Hob);

is writing to the emulated NOR flash, and this switches it into NOR
programming mode, causing the firmware to crash immediately as it can
no longer fetch instructions.

FYI I am using GDB to step through the code, i.e.,

- run gdb (or 'gdb-multiarch' if you are cross-compiling)
- start qemu with -s -S
- connect using 'target remote :1234'
- paste the 'add-symbol-file' line, e.g.,
add-symbol-file
/home/ard/build/edk2-workspace/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.dll
0x30000
- set breakpoint
"hb _ModuleEntryPoint"
- start executing
"c"
- use 'ni' to advance to the 'str' instruction that sets mReplaceLiveEntryFunc

> 0x3553c <_ModuleEntryPoint+96>  str     x1, [x0, #224]

Now, as soon as I step over that instruction (using 's'), the entire
view of memory changes into

│  > 0x35540 <_ModuleEntryPoint+100> .inst   0x00800080 ; undefined
│    0x35544 <_ModuleEntryPoint+104> .inst   0x00800080 ; undefined

etc, and the next step generates an exception, but this cannot be
handled either. This is all related to the NOR flash emulation code in
QEMU, that stops working as a ROM and switches into programming mode.

I cannot explain why this only happens in this case, and why some
writes seem to be ignored. But it does explain why this particular
firmware build is misbehaving

Now, if you apply the following patches:

ArmPkg/Mmu: Remove handling of NONSECURE memory regions
ArmPkg/ArmMmuLib: Introduce region types for RO/XP WB cached memory
ArmVirtPkg/ArmVirtQemu: Use read-only memory region type for code flash

(from the edk2-devel list), your build still crashes, but it prints
one additional line

Synchronous Exception at 0x3553C

which is the exception caused by the write to NOR flash, which is now
mapped read-only in the page tables, and so it is caught by the
firmware itself.

If you subsequently apply

ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs

things work as expected.

https://github.com/ardbiesheuvel/edk2/tree/arm_corruption-latest-ardb

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Marvin Häuser]

[-- Attachment #1.1: Type: text/html, Size: 9666 bytes --]

[-- Attachment #1.2: 14ca435fb6c059eaeb7fe6eedbe4738ffaf336d0.png --]
[-- Type: image/png, Size: 92682 bytes --]

Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()

[Pedro Falcato]

On Wed, Apr 19, 2023 at 10:55 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 19 Apr 2023 at 22:10, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >
> >
> > > On 19. Apr 2023, at 21:48, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > The issue is likely caused by
> > >
> > > -Wl,--defsym=PECOFF_HEADER_SIZE=0
> > >
> > > Why are you setting that? It breaks the ELF to PE conversion.
> >
> > Where?
>
> It would, but you only appear to be setting that for ASLD_DLINK_FLAGS,
> right? So that seems unrelated.
>
> The only thing I am observing is that the store to memory in
> ArmMmuBaseLibConstructor()
>
>   Hob = GetFirstGuidHob (&gArmMmuReplaceLiveTranslationEntryFuncGuid);
>   if (Hob != NULL) {
>     mReplaceLiveEntryFunc = *(VOID **)GET_GUID_HOB_DATA (Hob);
>
> is writing to the emulated NOR flash, and this switches it into NOR
> programming mode, causing the firmware to crash immediately as it can
> no longer fetch instructions.
>
> FYI I am using GDB to step through the code, i.e.,
>
> - run gdb (or 'gdb-multiarch' if you are cross-compiling)
> - start qemu with -s -S
> - connect using 'target remote :1234'
> - paste the 'add-symbol-file' line, e.g.,
> add-symbol-file
> /home/ard/build/edk2-workspace/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.dll
> 0x30000
> - set breakpoint
> "hb _ModuleEntryPoint"
> - start executing
> "c"
> - use 'ni' to advance to the 'str' instruction that sets mReplaceLiveEntryFunc
>
> > 0x3553c <_ModuleEntryPoint+96>  str     x1, [x0, #224]
>
> Now, as soon as I step over that instruction (using 's'), the entire
> view of memory changes into
>
> │  > 0x35540 <_ModuleEntryPoint+100> .inst   0x00800080 ; undefined
> │    0x35544 <_ModuleEntryPoint+104> .inst   0x00800080 ; undefined
>
> etc, and the next step generates an exception, but this cannot be
> handled either. This is all related to the NOR flash emulation code in
> QEMU, that stops working as a ROM and switches into programming mode.
>
> I cannot explain why this only happens in this case, and why some
> writes seem to be ignored. But it does explain why this particular
> firmware build is misbehaving
>
> Now, if you apply the following patches:
>
> ArmPkg/Mmu: Remove handling of NONSECURE memory regions
> ArmPkg/ArmMmuLib: Introduce region types for RO/XP WB cached memory
> ArmVirtPkg/ArmVirtQemu: Use read-only memory region type for code flash
>
> (from the edk2-devel list), your build still crashes, but it prints
> one additional line
>
> Synchronous Exception at 0x3553C
>
> which is the exception caused by the write to NOR flash, which is now
> mapped read-only in the page tables, and so it is caught by the
> firmware itself.
>
> If you subsequently apply
>
> ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs
>
> things work as expected.
>
> https://github.com/ardbiesheuvel/edk2/tree/arm_corruption-latest-ardb

<random bystander approaches>

Hi Ard,

Marvin's emails keep getting caught on your spam filter, please see
https://edk2.groups.io/g/devel/message/103259

<random bystander leaves>
-- 
Pedro