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.6396.1675881143891131986 for ; Wed, 08 Feb 2023 10:32:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=FvtSw74Y; 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 379A724036F for ; Wed, 8 Feb 2023 19:32:21 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1675881142; bh=bOg6NWGOiYdQCHr79COSGOKtZVixdYt+9Y9G1zZdkPI=; h=Subject:From:Date:Cc:To:From; b=FvtSw74YS1XI386lPdJ9EP6HnmSzBaFd8vFVQNgoTfNid8T+2BaiILnM8KatkrKrZ zM/Nqn19PmtvUs9vsVOh1/zA8yxO7Hafqdkpq7SDetaNGPElAABZWfejzrPzuepUOB dlcx1ZcyKPHWkeQqkNUIz6o5Gn9zyPqkHa8AuHpLg9mCmJ2sAy51DYdKqhHZB0LRUT HxKaMtXAJhecSeUEqcv9bpKmK/IhMcwV7Jbo1p4FhfSveq1QyTZkFlMdqGqA0ju1bN 3jIJvnxUftGDfix1qd9cJw6XW8gN0JqMlnlIvqVoieJZZvIQZwBSejluD+JhjB54EU sPZjxVZcNJBIg== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4PBpWR3Mggz9rxD; Wed, 8 Feb 2023 19:32:19 +0100 (CET) Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\)) Subject: Re: [PATCH 2/3] ArmPkg/CpuDxe: Perform preliminary NX remap of free memory From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= In-Reply-To: <20230208175812.700129-3-ardb@kernel.org> Date: Wed, 8 Feb 2023 18:32:08 +0000 Cc: edk2-devel-groups-io , Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar , Taylor Beebe Message-Id: <97EEE177-AE23-47CD-946F-F8578988C5EF@posteo.de> References: <20230208175812.700129-1-ardb@kernel.org> <20230208175812.700129-3-ardb@kernel.org> To: Ard Biesheuvel Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Thanks!! :) Comments inline. > On 8. Feb 2023, at 18:58, Ard Biesheuvel wrote: >=20 > The DXE core implementation of PcdDxeNxMemoryProtectionPolicy already > contains an assertion that EfiConventionalMemory and = EfiBootServicesData > 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 = page > 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. >=20 > 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, = we > 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. >=20 > 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. >=20 > Signed-off-by: Ard Biesheuvel > --- > ArmPkg/Drivers/CpuDxe/CpuDxe.c | 77 ++++++++++++++++++++ > ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 + > 2 files changed, 79 insertions(+) >=20 > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c = b/ArmPkg/Drivers/CpuDxe/CpuDxe.c > index d04958e79e52..83fd6fd4e476 100644 > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c > @@ -11,6 +11,8 @@ >=20 > #include >=20 > +#include > + > BOOLEAN mIsFlushingGCD; >=20 > /** > @@ -227,6 +229,69 @@ InitializeDma ( > CpuArchProtocol->DmaBufferAlignment =3D ArmCacheWritebackGranule (); > } >=20 > +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 = rather 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 = directly within *Core? I know there is code that doesn't, but the former = should 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 = entry will already be accounted for. However, I saw downstream code that = tried something like this (they actually added a constant overhead ahead = of time) bounded by something like 5 iterations. Might just have been = defensive programming - 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 = GetMemoryMap() should be bigger then the value returned in = MemoryMapSize" It appears the spec did not define a canonical way to call = GetMemoryMap(), did it? :( > + > + ASSERT_EFI_ERROR (Status); > + > + MemoryMapEntry =3D MemoryMap; > + MemoryMapEnd =3D (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + = MemoryMapSize); > + 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, = DescriptorSize); > + } > +} > + > EFI_STATUS > CpuDxeInitialize ( > IN EFI_HANDLE ImageHandle, > @@ -240,6 +305,18 @@ CpuDxeInitialize ( >=20 > InitializeDma (&mCpu); >=20 > + // > + // 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 = unused > + // memory is mapped with the same permissions as boot services data > + // regions. Otherwise, we may end up with unbounded recursion, due = to the > + // fact that updating permissions on a newly allocated page table = may 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 = InitializeDxeNxMemoryProtectionPolicy(), assuming BS_Data and ConvMem = have the same XP configuration? This reassures me that the CPU Arch protocol producers should be linked = into DxeCore rather than loaded at some arbitrary point in time... = Unrelated to the patch, of course. Best regards, Marvin > + > Status =3D gBS->InstallMultipleProtocolInterfaces ( > &mCpuHandle, > &gEfiCpuArchProtocolGuid, > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf = b/ArmPkg/Drivers/CpuDxe/CpuDxe.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] >=20 > [Pcd.common] > gArmTokenSpaceGuid.PcdVFPEnabled > + gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy >=20 > [FeaturePcd.common] > gArmTokenSpaceGuid.PcdDebuggerExceptionSupport > --=20 > 2.39.1 >=20