From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.44615.1681926031234287956 for ; Wed, 19 Apr 2023 10:40:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=pXQshIJ0; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 7E82564145 for ; Wed, 19 Apr 2023 17:40:30 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DE3A0C4339B for ; Wed, 19 Apr 2023 17:40:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1681926029; bh=1e+E7SdoIHr67ZkY3m7Mi3KS5RSDKkM2h69hRz+Xj+k=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=pXQshIJ051PGX3cnEQpjYm2q5gSJnyiXeRwDT+cxMoFUNnYnfRSu2IFoAPQsbPIOe LHjjXfp5/BCmX3aIdvLvGWCodRxytskzP5KaD/M4ZSrb4Txy3j7+I5HSsa19tdOEvl RnCUZw2yPsC320ictYjvhiWtUlG1h4bYEBDeKOm7g13mAQKcvv38UeIYV501IcPr4Y 478ruyB3aja938Rhmq8ZEv0q4LjCd2hfZd7wgVpvZbW0GGFVobwbekKwRHLjAg+dKT 4SlgcwsAaepQh2O2U9s6znDtLp3SQmD47umtN5BmGKi2oGZjj1czvxhpIUQO+J1igk LlimT0Vxe0K/Q== Received: by mail-lj1-f174.google.com with SMTP id r9so21441814ljp.9 for ; Wed, 19 Apr 2023 10:40:29 -0700 (PDT) X-Gm-Message-State: AAQBX9c04p9nOTfN+Zb339KvGiqk5SRgqLxcLtHmXLD02Yf5dYFtyHl8 TMVBN1e8YCxZPBuuv9eHiQMQQGnPc75yRRQcrxU= X-Google-Smtp-Source: AKy350YEDUwFbpdArrfm9r3JqvJQgTT57vJZnRMI18pU42xFJyprKwIM5HigHbLMFWqT0TXa1qJVmf4slqRxUJ3TkmM= X-Received: by 2002:a2e:9550:0:b0:2a7:b1de:3ff7 with SMTP id t16-20020a2e9550000000b002a7b1de3ff7mr2016046ljh.16.1681926027861; Wed, 19 Apr 2023 10:40:27 -0700 (PDT) MIME-Version: 1.0 References: <46CED01C-BEA6-49F3-9634-051DC63D248C@posteo.de> In-Reply-To: From: "Ard Biesheuvel" Date: Wed, 19 Apr 2023 19:40:16 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN() To: devel@edk2.groups.io, mhaeuser@posteo.de Cc: Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Vitaly Cheptsov Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 19 Apr 2023 at 19:14, Marvin H=C3=A4user 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 b= oundary. Which platform are you building? > To not just hide the issue via this patch, can someone please try to expl= ain 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() cross= ing page boundaries the actual issue (and is implicitly fixed by aligning i= t)? > 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 ArmReplaceLiveTr= anslationEntry() 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=C3=83=C2=A4user = wrote: > > > > On 18. Apr 2023, at 10:10, Ard Biesheuvel wrote: > > On Tue, 18 Apr 2023 at 08:40, Marvin H=C3=83=C2=A4user wrote: > > > > On 17. Apr 2023, at 23:18, Ard Biesheuvel wrote: > > Agree with all of this. > > And thanks for tracking 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 was 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 the alignment guarantee is actually met. Leif basic= ally declined any form of regression-testing at runtime. Do you happen to h= ave 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.) > > > > 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 seri= es, separate commit in the current series, or squashed into the fix? > > > Attachments: > > QEMU_EFI.fd > >=20