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.6924.1675882187545146561 for ; Wed, 08 Feb 2023 10:49:47 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=fRWrlPRQ; 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 DB04C6174F for ; Wed, 8 Feb 2023 18:49:46 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4C459C4339B for ; Wed, 8 Feb 2023 18:49:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1675882186; bh=eE6px6PsUcKt2ABjWeUAPT3fUQSOJizklvkn/ylmf88=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=fRWrlPRQn6mxa6ew7LEs+R5QY7oIeiowz6GrXlgfLvpHBIQ5Q+aJKP6aZr2xWq+Yg xUk1gusFtPzqdmp5JzeKXg9lZb5+SmiSJz5SCVo7Tddw6GMO+iiRZ7uWbO6SE5KEf4 hVihITImRncJMYr1R42Tt2xaehgqNE9Tc/E6rKLsZiqgHriyybA3v7yoo6CONN9Tv8 afy05aUckAXGSbtTUmhAobuegkg7ajDpq4HZTDudz1giNICB7hqe3gdI8euFBuHKmW Xeuzhw5LdjLcR4gDKi1saTF1MZKVONuuKV/PvcACEWJE8MbbYzQTJ21CxfV7c12Uim /R84kQtPq1uyQ== Received: by mail-lj1-f174.google.com with SMTP id h4so20461897lja.2 for ; Wed, 08 Feb 2023 10:49:46 -0800 (PST) X-Gm-Message-State: AO0yUKVHgZ1vX5lFsiG+tku+XYXKN0PYUwmIA1JUIvvKAKge8oJiAq3m ADoqsojQDST7GAflEoj8kCXKal5/uOO4x/+E+gw= X-Google-Smtp-Source: AK7set80cnTfDFvnh/BtQuA1A9pJ7QBfCBkq2V37yyN2fVT0iyb1ayPlK5yckAR8H7D33IC6V/WP+bHgRW9IsvQ+Nfs= X-Received: by 2002:a2e:8206:0:b0:290:5b9d:e97 with SMTP id w6-20020a2e8206000000b002905b9d0e97mr1340874ljg.187.1675882184288; Wed, 08 Feb 2023 10:49:44 -0800 (PST) MIME-Version: 1.0 References: <20230208175812.700129-1-ardb@kernel.org> <20230208175812.700129-3-ardb@kernel.org> <97EEE177-AE23-47CD-946F-F8578988C5EF@posteo.de> In-Reply-To: <97EEE177-AE23-47CD-946F-F8578988C5EF@posteo.de> From: "Ard Biesheuvel" Date: Wed, 8 Feb 2023 19:49:31 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 2/3] ArmPkg/CpuDxe: Perform preliminary NX remap of free memory To: devel@edk2.groups.io, mhaeuser@posteo.de Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar , Taylor Beebe Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 8 Feb 2023 at 19:32, Marvin H=C3=A4user wrote: > > Thanks!! :) Comments inline. > > > On 8. Feb 2023, at 18:58, Ard Biesheuvel wrote: > > > > The DXE core implementation of PcdDxeNxMemoryProtectionPolicy already > > contains an assertion that EfiConventionalMemory and EfiBootServicesDat= a > > are subjected to the same policy when it comes to the use of NX > > permissions. The reason for this is that we may otherwise end up with > > unbounded recursion in the page table code, given that allocating a pag= e > > table would then involve a permission attribute change, and this could > > result in the need for a block entry to be split, which would trigger > > the allocation of a page table recursively. > > > > For the same reason, a shortcut exists in ApplyMemoryProtectionPolicy() > > where, instead of setting the memory attributes unconditionally, we > > compare the NX policies and avoid touching the page tables if they are > > the same for the old and the new memory types. Without this shortcut, w= e > > may end up in a situation where, as the CPU arch protocol DXE driver is > > ramping up, the same unbounded recursion is triggered, due to the fact > > that the NX policy for EfiConventionalMemory has not been applied yet. > > > > To break this cycle, let's remap all EfiConventionalMemory regions > > according to the NX policy for EfiBootServicesData before exposing the > > CPU arch protocol to the DXE core and other drivers. This ensures that > > creating EfiBootServicesData allocations does not result in memory > > attribute changes, and therefore no recursion. > > > > Signed-off-by: Ard Biesheuvel > > --- > > ArmPkg/Drivers/CpuDxe/CpuDxe.c | 77 ++++++++++++++++++++ > > ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 + > > 2 files changed, 79 insertions(+) > > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/Cpu= Dxe.c > > index d04958e79e52..83fd6fd4e476 100644 > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c > > @@ -11,6 +11,8 @@ > > > > #include > > > > +#include > > + > > BOOLEAN mIsFlushingGCD; > > > > /** > > @@ -227,6 +229,69 @@ InitializeDma ( > > CpuArchProtocol->DmaBufferAlignment =3D ArmCacheWritebackGranule (); > > } > > > > +STATIC > > +VOID > > +RemapUnusedMemoryNx ( > > + VOID > > + ) > > +{ > > + UINT64 TestBit; > > + UINTN MemoryMapSize; > > + UINTN MapKey; > > + UINTN DescriptorSize; > > + UINT32 DescriptorVersion; > > + EFI_MEMORY_DESCRIPTOR *MemoryMap; > > + EFI_MEMORY_DESCRIPTOR *MemoryMapEntry; > > + EFI_MEMORY_DESCRIPTOR *MemoryMapEnd; > > + EFI_STATUS Status; > > + > > + TestBit =3D LShiftU64 (1, EfiBootServicesData); > > + if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & TestBit) =3D=3D 0) = { > > + return; > > + } > > + > > + MemoryMapSize =3D 0; > > + MemoryMap =3D NULL; > > + > > + Status =3D gBS->GetMemoryMap ( > > + &MemoryMapSize, > > + MemoryMap, > > + &MapKey, > > + &DescriptorSize, > > + &DescriptorVersion > > + ); > > + ASSERT (Status =3D=3D EFI_BUFFER_TOO_SMALL); > > + do { > > + MemoryMap =3D (EFI_MEMORY_DESCRIPTOR *)AllocatePool (MemoryMapSize= ); > > nitpick: *If* there is a V2, maybe drop the cast? > > > + ASSERT (MemoryMap !=3D NULL); > > I know ASSERTing for NULL is a common pattern for some reason, but I'd ra= ther not have more code with this added. > > > + Status =3D gBS->GetMemoryMap ( > > + &MemoryMapSize, > > + MemoryMap, > > + &MapKey, > > + &DescriptorSize, > > + &DescriptorVersion > > + ); > > Another nitpick, isn't it common practice to call the Core* functions dir= ectly within *Core? I know there is code that doesn't, but the former shoul= d be more efficient. > > > + if (EFI_ERROR (Status)) { > > + FreePool (MemoryMap); > > + } > > + } while (Status =3D=3D EFI_BUFFER_TOO_SMALL); > > Is this guaranteed to terminate? I mean, I get the idea - try again with = the larger size and when allocating the bigger buffer, its potential new en= try will already be accounted for. However, I saw downstream code that trie= d something like this (they actually added a constant overhead ahead of tim= e) bounded by something like 5 iterations. Might just have been defensive p= rogramming - probably was -, but it's not trivial to verify, I think, is it= ? > > Regarding the added constant, the spec actually says the following, which= obviously is just to shortcut a second round of GetMemoryMap(), but still: > "The actual size of the buffer allocated for the consequent call to GetMe= moryMap() should be bigger then the value returned in MemoryMapSize" > > It appears the spec did not define a canonical way to call GetMemoryMap()= , did it? :( > This is all copy-pasted from MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > > + > > + ASSERT_EFI_ERROR (Status); > > + > > + MemoryMapEntry =3D MemoryMap; > > + MemoryMapEnd =3D (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + Me= moryMapSize); > > + while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) { > > + if (MemoryMapEntry->Type =3D=3D EfiConventionalMemory) { > > + ArmSetMemoryRegionNoExec ( > > + MemoryMapEntry->PhysicalStart, > > + EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages) > > + ); > > + } > > + > > + MemoryMapEntry =3D NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, Descrip= torSize); > > + } > > +} > > + > > EFI_STATUS > > CpuDxeInitialize ( > > IN EFI_HANDLE ImageHandle, > > @@ -240,6 +305,18 @@ CpuDxeInitialize ( > > > > InitializeDma (&mCpu); > > > > + // > > + // Once we install the CPU arch protocol, the DXE core's memory > > + // protection routines will invoke them to manage the permissions of= page > > + // allocations as they are created. Given that this includes pages > > + // allocated for page tables by this driver, we must ensure that unu= sed > > + // memory is mapped with the same permissions as boot services data > > + // regions. Otherwise, we may end up with unbounded recursion, due t= o the > > + // fact that updating permissions on a newly allocated page table ma= y trigger > > + // a block entry split, which triggers a page table allocation, etc = etc > > + // > > + RemapUnusedMemoryNx (); > > Hmm. I might be too tired, but why is this not already done by Initialize= DxeNxMemoryProtectionPolicy(), assuming BS_Data and ConvMem have the same X= P configuration? > > This reassures me that the CPU Arch protocol producers should be linked i= nto DxeCore rather than loaded at some arbitrary point in time... Unrelated= to the patch, of course. > The ordering here is a bit tricky. As soon as the CPU arch protocol is exposed, every allocation will be remapped individually, resulting in page table splits and therefore recursion. > > > + > > Status =3D gBS->InstallMultipleProtocolInterfaces ( > > &mCpuHandle, > > &gEfiCpuArchProtocolGuid, > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/C= puDxe.inf > > index e732e21cb94a..8fd0f4133088 100644 > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf > > @@ -48,6 +48,7 @@ [LibraryClasses] > > DefaultExceptionHandlerLib > > DxeServicesTableLib > > HobLib > > + MemoryAllocationLib > > PeCoffGetEntryPointLib > > UefiDriverEntryPoint > > UefiLib > > @@ -64,6 +65,7 @@ [Guids] > > > > [Pcd.common] > > gArmTokenSpaceGuid.PcdVFPEnabled > > + gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy > > > > [FeaturePcd.common] > > gArmTokenSpaceGuid.PcdDebuggerExceptionSupport > > -- > > 2.39.1 > > > > > >=20 > >