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

["Marvin Häuser" <mhaeuser@posteo.de>]

[-- 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 --]