From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.79410.1675760212673823809 for ; Tue, 07 Feb 2023 00:56:53 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=ZTQpB9dt; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id E07C12403BA for ; Tue, 7 Feb 2023 09:56:50 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1675760210; bh=P8CSFMyre8IVADRyBBx1C26uR9BWgs2sGa1l+oUFh5g=; h=From:Subject:Date:Cc:To:From; b=ZTQpB9dtdj+aN0wFM94G4wP9jsQ60N5rZYJyoUwXw6F5IyWkumS/aj7myoe90JErq eVK7fiQAV1JYTtMOoY+8I7P1lLE8Sc93579Sw7TspbmI/Y85UtFNg1FY2np7/jNWhI /hCQg16/iU/zEJL7xmSrKEirVs47IsKUf1bFT5lnXC6r4XrleoxJ5HmWGRzh0Jkxvl bRGm3AdRL+XvKj9VqwM+m2rgy5nhOmCcMoz67raoVVE7ghsOEKkUu3qAFg2afZ2L6I l2kWaMFPBLW+05EV/VYOyoOm+zZxlMtrHV6ijjjZmZvk27K48L7FCW17sXIpJ7+YJv fYyLp6mwMC5Dw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4P9xns5sTkz6tmJ; Tue, 7 Feb 2023 09:56:49 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Date: Tue, 7 Feb 2023 08:56:48 +0000 Message-Id: References: Cc: devel@edk2.groups.io, t@taylorbeebe.com In-Reply-To: To: Ard Biesheuvel Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Taylor and Ard, > On 7. Feb 2023, at 09:29, Ard Biesheuvel wrote: >=20 > =EF=BB=BFOn Tue, 7 Feb 2023 at 02:18, Taylor Beebe wr= ote: >>=20 >> I can't see the Bugzilla you referenced so I requested security Bugzilla >> access. But, yes, that's the bug to which I was referring :) >>=20 >=20 > I cannot see that bugzilla entry either. I CC=E2=80=99d you both. >=20 >> Once Ard's change to add Memory Attribute Protocol support to ARM >> platforms is in, the change you linked may be palatable for the >> upstream. However, ARM platforms could run into the infinite loop I >> outlined if that change is pushed upstream because of the lack of >> per-allocated page tables and a software semaphore to prevent looping. >>=20 >=20 > I still don't see how this happens. There is an ASSERT in > CoreInitializeMemoryProtection() to ensure that EfiConventialMemory > and EfiBootServicesData have the same memory type, and I added that > (in commit 7eb927db3e25a) for precisely this reason, i.e., to ensure > that the plumbing of this feature wouldn't recurse. >=20 > Could this be related to heap guard, perhaps? I could see how changing > the boundaries of allocations might trigger a split even if the old > and new type should have the exact same type, and perhaps we should > use unguarded pages for page tables. I know you meant recursing, but that might be related to the BZ, if I under= stood Taylor correctly. The only scenario I first-hand experienced this bug= with was unloading a PE image. I don=E2=80=99t have the time right now to = check the guarding page code in detail, but from what I just saw, I can ver= y well imagine it can trigger the BZ bug (and thus potentially the recursio= n issue?). >=20 >=20 >> I implemented pre-allocated pages for ARM a while back in a private repo >> but never committed it to Mu. Maybe that would also be worth committing >> and pushing upstream. >>=20 >=20 > I'd like to understand better whether or not there is a way to avoid > the need for this, but if not, I'd be happy to review your solution. > Does the issue only exist on ARM? Does it still happen after I rewrote > the MMU library? (in 2020) Sorry to interject with no contribution, but for x86 platforms, our downstr= eam fork removed the requirement that BSData and ConvMem need to have the s= ame permissions. In fact, ideally ConvMem is just unmapped. Can this be ena= bled for ARM without a solution like Taylor=E2=80=99s? You said you added t= he requirement as a mitigation. Unrelated FYI, we also removed the XP checks for code downstream and all ty= pes but ConvMem (which is unmapped or read-only, I forgot) have default per= missions of RW. The reason for that is that unlike an OS, we don=E2=80=99t = have a fully-featured VM system and especially things like mmap are absent.= Thus, any data or code must first be written to the memory before it can b= e executed. The execute flag is added after loading the code to ensure W^X. Best regards, Marvin >=20 > Thanks, > Ard. >=20 >=20 >>> On 2/3/2023 11:58 AM, Marvin H=C3=A4user wrote: >>> Hi Taylor, >>>=20 >>> Do you by any chance mean this bug? >>> https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModuleP= kg/Core/Dxe/Misc/MemoryProtection.c#L1544 >>>=20 >>> I reported this a while ago at >>> https://bugzilla.tianocore.org/show_bug.cgi?id=3D3316 >>> >>>=20 >>> The Mu fix is by no means a workaround and actually fixes this issue in >>> a sane way. It should have been fixed upstream ages ago. >>>=20 >>> Best regards, >>> Marvin >>>=20 >>=20 >>=20 >>=20 >>=20 >>=20 >>=20