From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web10.44806.1681926350234915848 for ; Wed, 19 Apr 2023 10:45:50 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=I+ke54IZ; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 55950240303 for ; Wed, 19 Apr 2023 19:45:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1681926348; bh=kRprAdppo25wPaZmQTcOYjGhP8vO67QoqAXaxSYG1n0=; h=From:Subject:Date:Cc:To:From; b=I+ke54IZB+Uf17qLCC3SzSKRnv6WUpZpabv+F1+zZfzK6TelgU9Y/EDw5/MWXFV7V JIjWGV9vAJJJzeABabzDyyXELJU074HMspHpo3fUwWF+UDhl7cbZaJPKMEVwXWRiiE k06Al9wdVRxxCZ7qp2gL2HLHVz2/yZNDERqLepZ8rv+SbQtDkDG8FBns+NeItB+iHf C/lPmRmmia9p+4/T4mlRG9XlUd8iFhFpAwSQMgPuqIyWdTeaWhg9lfZxmn1zW1Bmh8 1EsviRnekNfk0bhcrr/2jNhr4rNjxV/sxclSGqMgRbJwBn9IK+cxF1QCeBIXdxEfvC NotUCzyCd0Ncw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4Q1p9Q5tWgz6tw4; Wed, 19 Apr 2023 19:45:46 +0200 (CEST) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-Id: <696924B3-EF5B-4799-AAD9-E090C97D9AA9@posteo.de> Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.500.231\)) Subject: Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN() Date: Wed, 19 Apr 2023 17:45:36 +0000 In-Reply-To: Cc: edk2-devel-groups-io , Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Vitaly Cheptsov To: Ard Biesheuvel References: <46CED01C-BEA6-49F3-9634-051DC63D248C@posteo.de> Content-Type: multipart/alternative; boundary="Apple-Mail=_65386F1B-D318-4DAD-B05A-3D139D17AF54" --Apple-Mail=_65386F1B-D318-4DAD-B05A-3D139D17AF54 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 19. Apr 2023, at 19:40, Ard Biesheuvel wrote: >=20 > On Wed, 19 Apr 2023 at 19:14, Marvin H=C3=A4user > wrote: >>=20 >> Hi all, >>=20 >> 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. >=20 > Which platform are you building? ArmVirtPkg / AARCH64 / DEBUG / GCC5 (GCC 12.2.0). >=20 >> To not just hide the issue via this patch, can someone please try to exp= lain the exact requirements this function has (the comments read like 0x200= was just the lowest value to guarantee staying within a page)? Why would i= t be broken if misaligned, but not crossing a page? >>=20 >=20 > 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 >=20 > 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. >=20 > 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 com= mit of the linked branch. >=20 >> Is there any chance the FD is somehow misaligned in memory, thus shiftin= g the function across a page in the process? Or is the FD mapped to a fixed= address like with x86? Is code after ArmReplaceLiveTranslationEntry() cros= sing page boundaries the actual issue (and is implicitly fixed by aligning = it)? >>=20 >=20 > 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 corrupti= on. Any help is appreciated. :) Best regards, Marvin >=20 >=20 >> For reference, I attached the broken FD. The problematic ArmReplaceLiveT= ranslationEntry() is located at MemoryInitPei 0x25C10 - 0x25D54. >> This is from my arm_corruption-latest branch: https://github.com/mhaeuse= r/edk2/tree/arm_corruption-latest >>=20 >> Best regards, >> Marvin >>=20 >>=20 >> On 18. Apr 2023, at 10:18, Marvin H=C3=83=C2=A4user > wrote: >>=20 >>=20 >>=20 >> On 18. Apr 2023, at 10:10, Ard Biesheuvel > wrote: >>=20 >> On Tue, 18 Apr 2023 at 08:40, Marvin H=C3=83=C2=A4user > wrote: >>=20 >>=20 >>=20 >> On 17. Apr 2023, at 23:18, Ard Biesheuvel > wrote: >>=20 >> Agree with all of this. >>=20 >> And thanks for tracking this down - must not have been fun :-) >>=20 >>=20 >> No worries - it wasn=C3=A2=E2=82=AC=E2=84=A2t. :) It was mere luck Vital= y discovered early it was an issue that triggered based on the binary layou= t rather than a bug in the mapping code itself=C3=A2=E2=82=AC=C2=A6 >>=20 >> Speaking of not fun to track down, I initially wanted to add ASSERTs (ye= s, runtime :( ) to check the alignment guarantee is actually met. Leif basi= cally 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=C3=A2= =E2=82=AC=E2=84=A2s less about =C3=A2=E2=82=AC=C5=93which commands do I run= ?=C3=A2=E2=82=AC and more about integration with the build process / BaseTo= ols, cross-OS compatibility, etc.) >>=20 >>=20 >>=20 >> I think we should just add another align to the code: >>=20 >> .align xx >> func: >>=20 >> < code > >>=20 >> .align xx >> .org func + xx >>=20 >> That way, if the code straddles a xx-aligned boundary, the .org will >> move backwards and force an error. >>=20 >>=20 >> Wow, that's pretty fucking smart... I didn't even know that directive wa= s a thing. I will try to toy with it soon. Do you want it as a separate ser= ies, separate commit in the current series, or squashed into the fix? >>=20 >>=20 >> Attachments: >>=20 >> QEMU_EFI.fd >>=20 >>=20 --Apple-Mail=_65386F1B-D318-4DAD-B05A-3D139D17AF54 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
On 19. Apr 2023, at 19:40, Ard Biesheuvel <ardb@kernel.org> wr= ote:

On = Wed, 19 Apr 2023 at 19:14, Marvin H=C3=A4user <mhaeuser@posteo.de> wrote:

Hi all,

Whil= e testing Ard's suggestion for V3, I noticed I got a broken FD where ArmRep= laceLiveTranslationEntry() is misaligned, but does not cross a 4 KB boundar= y.

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 ex= plain the exact requirements this function has (the comments read like 0x20= 0 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, s= o it's just
the smallest value that fits that requi= rement, 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
actua= lly 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 w= ith the build tools you are
using.

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


Is there any chance the FD is = somehow misaligned in memory, thus shifting the function across a page in t= he process? Or is the FD mapped to a fixed address like with x86? Is code a= fter ArmReplaceLiveTranslationEntry() crossing page boundaries the actual i= ssue (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 si= mply is not crossing a page boundary... which means the patch did fix a val= id bug, but it wasn't what actually caused the corruption. Any help is appr= eciated. :)

Best regards,
Marvin


For reference, I a= ttached the broken FD. The problematic ArmReplaceLiveTranslationEntry() is = located at MemoryInitPei 0x25C10 - 0x25D54.
This is from my arm_corrupti= on-latest branch: https://gi= thub.com/mhaeuser/edk2/tree/arm_corruption-latest

Best regards,<= br>Marvin


On 18. Apr 2023, at 10:18, Marvin H=C3=83=C2=A4user &l= t;mhaeuser@posteo.de> wrote:


On 18. Apr 2023, at 10:10, Ard Biesheuvel <ardb@kernel.org> wrote:

On Tue, 18 Apr 20= 23 at 08:40, Marvin H=C3=83=C2=A4user <mhaeuser@posteo.de> wrote:



On 17. Apr 2023, at 2= 3:18, Ard Biesheuvel <ardb@kernel.org= > wrote:

Agree with all of this.

And thanks for tracki= ng this down - must not have been fun :-)


No worries - it wasn= =C3=A2=E2=82=AC=E2=84=A2t. :) It was mere luck Vitaly discovered early it w= as an issue that triggered based on the binary layout rather than a bug in = the mapping code itself=C3=A2=E2=82=AC=C2=A6

Speaking of not fun to = track down, I initially wanted to add ASSERTs (yes, runtime :( ) to check t= he 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=C3=A2=E2=82=AC=E2=84=A2s less ab= out =C3=A2=E2=82=AC=C5=93which commands do I run?=C3=A2=E2=82=AC and more a= bout 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 pre= tty 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 comm= it in the current series, or squashed into the fix?


Attachments:=

QEMU_EFI.fd

--Apple-Mail=_65386F1B-D318-4DAD-B05A-3D139D17AF54--