[edk2-devel] [PATCH 1/1] OvmfPkg: Align the SEC module within OvmfPkgX64

[edk2-devel] [PATCH 1/1] OvmfPkg: Align the SEC module within OvmfPkgX64

[Glenn Griffin]

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

Prior to this change the alignment of the SEC module would be 4 bytes.
This is inconsistent with the expectations of the compiler and can lead
to unexpected behavior.

For example a modern version of clang with size optimizations enabled
(-Oz) can break the ALIGN_POINTER macro in the SEC module.

Here is a minimal example:

https://godbolt.org/z/fzW67ndTY

```
unsigned char mArray[112];

#define ALIGN_VALUE(Value, Alignment)  ((Value) + (((Alignment) - (Value)) & ((Alignment) - 1)))
#define ALIGN_POINTER(Pointer, Alignment)  ((void *) (ALIGN_VALUE ((uint64_t)(Pointer), (Alignment))))

int main() {
printf("0x%llx\n", mArray);
printf("0x%llx\n", ALIGN_POINTER(mArray, 64));
return 0;
}
```

The above code compiles down to:

```
main:                                   # @main
push    r14
push    rbx
push    rax
lea     rbx, [rip + .L.str]
lea     r14, [rip + mArray]
mov     rdi, rbx
mov     rsi, r14
xor     eax, eax
call    printf@PLT
push    64
pop     rsi
sub     esi, r14d
and     esi, 48
add     rsi, r14
mov     rdi, rbx
xor     eax, eax
call    printf@PLT
xor     eax, eax
add     rsp, 8
pop     rbx
pop     r14
ret
mArray:
.zero   112

.L.str:
.asciz  "0x%llx\n"
```

Note that the `value & 63` in the ALIGN_VALUE implementation gets
transformed into `and esi, 48` in the assembly. The compiler knows
the array address is already aligned so it believes there is no need to
clear the last 4 bits of the address.

However by mapping the data section that contains mArray onto a
4-byte-aligned base address we violate the compiler's expectations. The
last 4 bits of the mArray address are no longer zeroes leading to an
ALIGN_POINTER macro that doesn't work.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>

Signed-off-by: Glenn Griffin <ggriffiniii@gmail.com>
---
OvmfPkg/OvmfPkgX64.fdf | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index eb3fb90cb8b6..8b60355de40b 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -434,7 +434,7 @@ [FV.FVMAIN_COMPACT]

[Rule.Common.SEC]
FILE SEC = $(NAMED_GUID) {
-    PE32     PE32           $(INF_OUTPUT)/$(MODULE_NAME).efi
+    PE32     PE32   Align=Auto    $(INF_OUTPUT)/$(MODULE_NAME).efi
UI       STRING ="$(MODULE_NAME)" Optional
VERSION  STRING ="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER)
}
--
2.44.0.396.g6e790dbe36-goog


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117138): https://edk2.groups.io/g/devel/message/117138
Mute This Topic: https://groups.io/mt/105165329/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

[edk2-devel] [PATCH 1/1] OvmfPkg: Align the SEC module within OvmfPkgX64

[Glenn Griffin]

Prior to this change the alignment of the SEC module would be 4 bytes.
This is inconsistent with the expectations of the compiler and can lead
to unexpected behavior.

For example a modern version of clang with size optimizations enabled
(-Oz) can break the ALIGN_POINTER macro in the SEC module.

Here is a minimal example:

https://godbolt.org/z/fzW67ndTY

```
unsigned char mArray[112];

#define ALIGN_VALUE(Value, Alignment)  ((Value) + (((Alignment) -
(Value)) & ((Alignment) - 1)))
#define ALIGN_POINTER(Pointer, Alignment)  ((void *) (ALIGN_VALUE
((uint64_t)(Pointer), (Alignment))))

int main() {
    printf("0x%llx\n", mArray);
    printf("0x%llx\n", ALIGN_POINTER(mArray, 64));
    return 0;
}
```

The above code compiles down to:

```
main:                                   # @main
        push    r14
        push    rbx
        push    rax
        lea     rbx, [rip + .L.str]
        lea     r14, [rip + mArray]
        mov     rdi, rbx
        mov     rsi, r14
        xor     eax, eax
        call    printf@PLT
        push    64
        pop     rsi
        sub     esi, r14d
        and     esi, 48
        add     rsi, r14
        mov     rdi, rbx
        xor     eax, eax
        call    printf@PLT
        xor     eax, eax
        add     rsp, 8
        pop     rbx
        pop     r14
        ret
mArray:
        .zero   112

.L.str:
        .asciz  "0x%llx\n"
```

Note that the `value & 63` in the ALIGN_VALUE implementation gets
transformed into `and esi, 48` in the assembly. The compiler knows
the array address is already aligned so it believes there is no need to
clear the last 4 bits of the address.

However by mapping the data section that contains mArray onto a
4-byte-aligned base address we violate the compiler's expectations. The
last 4 bits of the mArray address are no longer zeroes leading to an
ALIGN_POINTER macro that doesn't work.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>

Signed-off-by: Glenn Griffin <ggriffiniii@gmail.com>
---
 OvmfPkg/OvmfPkgX64.fdf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index eb3fb90cb8b6..8b60355de40b 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -434,7 +434,7 @@ [FV.FVMAIN_COMPACT]

 [Rule.Common.SEC]
   FILE SEC = $(NAMED_GUID) {
-    PE32     PE32           $(INF_OUTPUT)/$(MODULE_NAME).efi
+    PE32     PE32   Align=Auto    $(INF_OUTPUT)/$(MODULE_NAME).efi
     UI       STRING ="$(MODULE_NAME)" Optional
     VERSION  STRING ="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER)
   }
-- 
2.44.0.396.g6e790dbe36-goog


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117139): https://edk2.groups.io/g/devel/message/117139
Mute This Topic: https://groups.io/mt/105165329/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Align the SEC module within OvmfPkgX64

[Ard Biesheuvel]

On Tue, 26 Mar 2024 at 22:01, Glenn Griffin <ggriffiniii@gmail.com> wrote:
>
> Prior to this change the alignment of the SEC module would be 4 bytes.
> This is inconsistent with the expectations of the compiler and can lead
> to unexpected behavior.
>
> For example a modern version of clang with size optimizations enabled
> (-Oz) can break the ALIGN_POINTER macro in the SEC module.
>

...
>
> However by mapping the data section that contains mArray onto a
> 4-byte-aligned base address we violate the compiler's expectations. The
> last 4 bits of the mArray address are no longer zeroes leading to an
> ALIGN_POINTER macro that doesn't work.
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>
> Signed-off-by: Glenn Griffin <ggriffiniii@gmail.com>
> ---
>  OvmfPkg/OvmfPkgX64.fdf | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index eb3fb90cb8b6..8b60355de40b 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -434,7 +434,7 @@ [FV.FVMAIN_COMPACT]
>
>  [Rule.Common.SEC]
>    FILE SEC = $(NAMED_GUID) {
> -    PE32     PE32           $(INF_OUTPUT)/$(MODULE_NAME).efi
> +    PE32     PE32   Align=Auto    $(INF_OUTPUT)/$(MODULE_NAME).efi
>      UI       STRING ="$(MODULE_NAME)" Optional
>      VERSION  STRING ="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER)
>    }

Thanks Glenn. As you have mentioned to me off-list, this is the same
issue Andrew reported here

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=3887

Your fix is obviously correct, and I intend to merge it unless anyone
has any concerns with this.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117140): https://edk2.groups.io/g/devel/message/117140
Mute This Topic: https://groups.io/mt/105165329/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-