* [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael
@ 2017-03-21 9:23 Ard Biesheuvel
2017-03-21 9:23 ` [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy Ard Biesheuvel
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2017-03-21 9:23 UTC (permalink / raw)
To: edk2-devel, lersek; +Cc: sigmaepsilon92, Ard Biesheuvel
This fixes two separate issues reported by Michael:
- setting EFI_MEMORY_XP via the GCD memory space map always fails,
- patchable PCD updates do not propagate to other modules.
Ard Biesheuvel (2):
ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect
policy
ArmVirtPkg/HighMemDxe: check new regions against GCD memory space map
ArmVirtPkg/HighMemDxe/HighMemDxe.c | 59 ++++++++++++++------
ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 2 +-
2 files changed, 43 insertions(+), 18 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy 2017-03-21 9:23 [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael Ard Biesheuvel @ 2017-03-21 9:23 ` Ard Biesheuvel 2017-03-21 10:36 ` Laszlo Ersek 2017-03-21 9:23 ` [PATCH 2/2] ArmVirtPkg/HighMemDxe: check new regions against GCD memory space map Ard Biesheuvel 2017-03-21 10:42 ` [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael Ard Biesheuvel 2 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2017-03-21 9:23 UTC (permalink / raw) To: edk2-devel, lersek; +Cc: sigmaepsilon92, Ard Biesheuvel Instead of invoking gDS->SetMemorySpaceAttributes to set the EFI_MEMORY_XP attribute on newly added regions, which is guaranteed to fail if the same attribute was not declared as a capability of the region when it as added, invoke the CPU arch protocol directly to set the EFI_MEMORY_XP attribute if our memory protection policy demands it. Reported-by: Michael Zimmermann <sigmaepsilon92@gmail.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmVirtPkg/HighMemDxe/HighMemDxe.c | 31 +++++++++++++++----- ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 1 + 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c index f70978f6414f..4e41120deff3 100644 --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c @@ -20,6 +20,7 @@ #include <Library/PcdLib.h> #include <Library/UefiBootServicesTableLib.h> +#include <Protocol/Cpu.h> #include <Protocol/FdtClient.h> EFI_STATUS @@ -30,6 +31,7 @@ InitializeHighMemDxe ( ) { FDT_CLIENT_PROTOCOL *FdtClient; + EFI_CPU_ARCH_PROTOCOL *Cpu; EFI_STATUS Status, FindNodeStatus; INT32 Node; CONST UINT32 *Reg; @@ -43,6 +45,10 @@ InitializeHighMemDxe ( (VOID **)&FdtClient); ASSERT_EFI_ERROR (Status); + Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, + (VOID **)&Cpu); + ASSERT_EFI_ERROR (Status); + // // Check for memory node and add the memory spaces except the lowest one // @@ -78,13 +84,24 @@ InitializeHighMemDxe ( continue; } + Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, + EFI_MEMORY_WB); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_WARN, + "%a: gDS->SetMemorySpaceAttributes() failed on region 0x%lx - 0x%lx (%r)\n", + __FUNCTION__, CurBase, CurBase + CurSize - 1, Status)); + } + + // + // Due to the ambiguous nature of the RO/XP GCD memory space attributes, + // it is impossible to add a memory space with the XP attribute in a way + // that does not result in the XP attribute being set on *all* UEFI + // memory map entries that are carved from it, including code regions + // that require executable permissions. // - // Take care not to strip any permission attributes that will have been - // set by DxeCore on the region we just added if a strict permission - // policy is in effect for EfiConventionalMemory regions. - // Unfortunately, we cannot interrogate the GCD memory space map for - // those permissions, since they are not recorded there (for historical - // reasons), so check the policy directly. + // So instead, we never set the RO/XP attributes in the GCD memory space + // capabilities or attribute fields, and apply any protections directly + // on the page table mappings by going through the cpu arch protocol. // Attributes = EFI_MEMORY_WB; if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & @@ -92,7 +109,7 @@ InitializeHighMemDxe ( Attributes |= EFI_MEMORY_XP; } - Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes); + Status = Cpu->SetMemoryAttributes (Cpu, CurBase, CurSize, Attributes); if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf index 89c743ebe058..ac1761974f52 100644 --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf @@ -41,6 +41,7 @@ [LibraryClasses] UefiDriverEntryPoint [Protocols] + gEfiCpuArchProtocolGuid ## CONSUMES gFdtClientProtocolGuid ## CONSUMES [Pcd] -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy 2017-03-21 9:23 ` [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy Ard Biesheuvel @ 2017-03-21 10:36 ` Laszlo Ersek 0 siblings, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2017-03-21 10:36 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel; +Cc: sigmaepsilon92 On 03/21/17 10:23, Ard Biesheuvel wrote: > Instead of invoking gDS->SetMemorySpaceAttributes to set the EFI_MEMORY_XP > attribute on newly added regions, which is guaranteed to fail if the same > attribute was not declared as a capability of the region when it as added, > invoke the CPU arch protocol directly to set the EFI_MEMORY_XP attribute > if our memory protection policy demands it. > > Reported-by: Michael Zimmermann <sigmaepsilon92@gmail.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmVirtPkg/HighMemDxe/HighMemDxe.c | 31 +++++++++++++++----- > ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 1 + > 2 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c > index f70978f6414f..4e41120deff3 100644 > --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c > +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c > @@ -20,6 +20,7 @@ > #include <Library/PcdLib.h> > #include <Library/UefiBootServicesTableLib.h> > > +#include <Protocol/Cpu.h> > #include <Protocol/FdtClient.h> > > EFI_STATUS > @@ -30,6 +31,7 @@ InitializeHighMemDxe ( > ) > { > FDT_CLIENT_PROTOCOL *FdtClient; > + EFI_CPU_ARCH_PROTOCOL *Cpu; > EFI_STATUS Status, FindNodeStatus; > INT32 Node; > CONST UINT32 *Reg; > @@ -43,6 +45,10 @@ InitializeHighMemDxe ( > (VOID **)&FdtClient); > ASSERT_EFI_ERROR (Status); > > + Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, > + (VOID **)&Cpu); > + ASSERT_EFI_ERROR (Status); > + > // > // Check for memory node and add the memory spaces except the lowest one > // > @@ -78,13 +84,24 @@ InitializeHighMemDxe ( > continue; > } > > + Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, > + EFI_MEMORY_WB); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, > + "%a: gDS->SetMemorySpaceAttributes() failed on region 0x%lx - 0x%lx (%r)\n", > + __FUNCTION__, CurBase, CurBase + CurSize - 1, Status)); > + } > + > + // > + // Due to the ambiguous nature of the RO/XP GCD memory space attributes, > + // it is impossible to add a memory space with the XP attribute in a way > + // that does not result in the XP attribute being set on *all* UEFI > + // memory map entries that are carved from it, including code regions > + // that require executable permissions. > // > - // Take care not to strip any permission attributes that will have been > - // set by DxeCore on the region we just added if a strict permission > - // policy is in effect for EfiConventionalMemory regions. > - // Unfortunately, we cannot interrogate the GCD memory space map for > - // those permissions, since they are not recorded there (for historical > - // reasons), so check the policy directly. > + // So instead, we never set the RO/XP attributes in the GCD memory space > + // capabilities or attribute fields, and apply any protections directly > + // on the page table mappings by going through the cpu arch protocol. > // > Attributes = EFI_MEMORY_WB; > if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & > @@ -92,7 +109,7 @@ InitializeHighMemDxe ( > Attributes |= EFI_MEMORY_XP; > } > > - Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes); > + Status = Cpu->SetMemoryAttributes (Cpu, CurBase, CurSize, Attributes); > > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_ERROR, > diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf > index 89c743ebe058..ac1761974f52 100644 > --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf > +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf > @@ -41,6 +41,7 @@ [LibraryClasses] > UefiDriverEntryPoint > > [Protocols] > + gEfiCpuArchProtocolGuid ## CONSUMES > gFdtClientProtocolGuid ## CONSUMES > > [Pcd] > gEfiCpuArchProtocolGuid is already part of the DEPEX, so: Reviewed-by: Laszlo Ersek <lersek@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] ArmVirtPkg/HighMemDxe: check new regions against GCD memory space map 2017-03-21 9:23 [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael Ard Biesheuvel 2017-03-21 9:23 ` [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy Ard Biesheuvel @ 2017-03-21 9:23 ` Ard Biesheuvel 2017-03-21 10:40 ` Laszlo Ersek 2017-03-21 10:42 ` [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael Ard Biesheuvel 2 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2017-03-21 9:23 UTC (permalink / raw) To: edk2-devel, lersek; +Cc: sigmaepsilon92, Ard Biesheuvel Instead of looking at the PCD gArmTokenSpaceGuid.PcdSystemMemoryBase to decide which DT node covers the memory we are already using, query the GCD memory space map, which is the authoritative source for this kind of information This fixes a problem observed by Michael on platforms where this PCD is of the 'Patchable' type, which means updates to its value do not propagate to other modules. Reported-by: Michael Zimmermann <sigmaepsilon92@gmail.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmVirtPkg/HighMemDxe/HighMemDxe.c | 30 +++++++++++++------- ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 1 - 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c index 4e41120deff3..aa3f5f6d8956 100644 --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c @@ -30,16 +30,17 @@ InitializeHighMemDxe ( IN EFI_SYSTEM_TABLE *SystemTable ) { - FDT_CLIENT_PROTOCOL *FdtClient; - EFI_CPU_ARCH_PROTOCOL *Cpu; - EFI_STATUS Status, FindNodeStatus; - INT32 Node; - CONST UINT32 *Reg; - UINT32 RegSize; - UINTN AddressCells, SizeCells; - UINT64 CurBase; - UINT64 CurSize; - UINT64 Attributes; + FDT_CLIENT_PROTOCOL *FdtClient; + EFI_CPU_ARCH_PROTOCOL *Cpu; + EFI_STATUS Status, FindNodeStatus; + INT32 Node; + CONST UINT32 *Reg; + UINT32 RegSize; + UINTN AddressCells, SizeCells; + UINT64 CurBase; + UINT64 CurSize; + UINT64 Attributes; + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, (VOID **)&FdtClient); @@ -73,7 +74,14 @@ InitializeHighMemDxe ( } RegSize -= (AddressCells + SizeCells) * sizeof (UINT32); - if (PcdGet64 (PcdSystemMemoryBase) != CurBase) { + Status = gDS->GetMemorySpaceDescriptor (CurBase, &GcdDescriptor); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_WARN, + "%a: Region 0x%lx - 0x%lx not found in the GCD memory space map\n", + __FUNCTION__, CurBase, CurBase + CurSize - 1)); + continue; + } + if (GcdDescriptor.GcdMemoryType == EfiGcdMemoryTypeNonExistent) { Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, CurBase, CurSize, EFI_MEMORY_WB); diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf index ac1761974f52..a7072e38d09d 100644 --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf @@ -45,7 +45,6 @@ [Protocols] gFdtClientProtocolGuid ## CONSUMES [Pcd] - gArmTokenSpaceGuid.PcdSystemMemoryBase gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy [Depex] -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ArmVirtPkg/HighMemDxe: check new regions against GCD memory space map 2017-03-21 9:23 ` [PATCH 2/2] ArmVirtPkg/HighMemDxe: check new regions against GCD memory space map Ard Biesheuvel @ 2017-03-21 10:40 ` Laszlo Ersek 0 siblings, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2017-03-21 10:40 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel; +Cc: sigmaepsilon92 On 03/21/17 10:23, Ard Biesheuvel wrote: > Instead of looking at the PCD gArmTokenSpaceGuid.PcdSystemMemoryBase > to decide which DT node covers the memory we are already using, query > the GCD memory space map, which is the authoritative source for this > kind of information > > This fixes a problem observed by Michael on platforms where this PCD > is of the 'Patchable' type, which means updates to its value do not > propagate to other modules. > > Reported-by: Michael Zimmermann <sigmaepsilon92@gmail.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmVirtPkg/HighMemDxe/HighMemDxe.c | 30 +++++++++++++------- > ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 1 - > 2 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c > index 4e41120deff3..aa3f5f6d8956 100644 > --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c > +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c > @@ -30,16 +30,17 @@ InitializeHighMemDxe ( > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > - FDT_CLIENT_PROTOCOL *FdtClient; > - EFI_CPU_ARCH_PROTOCOL *Cpu; > - EFI_STATUS Status, FindNodeStatus; > - INT32 Node; > - CONST UINT32 *Reg; > - UINT32 RegSize; > - UINTN AddressCells, SizeCells; > - UINT64 CurBase; > - UINT64 CurSize; > - UINT64 Attributes; > + FDT_CLIENT_PROTOCOL *FdtClient; > + EFI_CPU_ARCH_PROTOCOL *Cpu; > + EFI_STATUS Status, FindNodeStatus; > + INT32 Node; > + CONST UINT32 *Reg; > + UINT32 RegSize; > + UINTN AddressCells, SizeCells; > + UINT64 CurBase; > + UINT64 CurSize; > + UINT64 Attributes; > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; > > Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, > (VOID **)&FdtClient); > @@ -73,7 +74,14 @@ InitializeHighMemDxe ( > } > RegSize -= (AddressCells + SizeCells) * sizeof (UINT32); > > - if (PcdGet64 (PcdSystemMemoryBase) != CurBase) { > + Status = gDS->GetMemorySpaceDescriptor (CurBase, &GcdDescriptor); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, > + "%a: Region 0x%lx - 0x%lx not found in the GCD memory space map\n", > + __FUNCTION__, CurBase, CurBase + CurSize - 1)); > + continue; > + } > + if (GcdDescriptor.GcdMemoryType == EfiGcdMemoryTypeNonExistent) { > Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, CurBase, > CurSize, EFI_MEMORY_WB); > > diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf > index ac1761974f52..a7072e38d09d 100644 > --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf > +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf > @@ -45,7 +45,6 @@ [Protocols] > gFdtClientProtocolGuid ## CONSUMES > > [Pcd] > - gArmTokenSpaceGuid.PcdSystemMemoryBase > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy > > [Depex] > Reviewed-by: Laszlo Ersek <lersek@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael 2017-03-21 9:23 [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael Ard Biesheuvel 2017-03-21 9:23 ` [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy Ard Biesheuvel 2017-03-21 9:23 ` [PATCH 2/2] ArmVirtPkg/HighMemDxe: check new regions against GCD memory space map Ard Biesheuvel @ 2017-03-21 10:42 ` Ard Biesheuvel 2 siblings, 0 replies; 6+ messages in thread From: Ard Biesheuvel @ 2017-03-21 10:42 UTC (permalink / raw) To: edk2-devel@lists.01.org, Laszlo Ersek; +Cc: Michael Zimmermann, Ard Biesheuvel On 21 March 2017 at 09:23, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > This fixes two separate issues reported by Michael: > - setting EFI_MEMORY_XP via the GCD memory space map always fails, > - patchable PCD updates do not propagate to other modules. > > Ard Biesheuvel (2): > ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect > policy > ArmVirtPkg/HighMemDxe: check new regions against GCD memory space map > > ArmVirtPkg/HighMemDxe/HighMemDxe.c | 59 ++++++++++++++------ > ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 2 +- > 2 files changed, 43 insertions(+), 18 deletions(-) > Pushed as 60bd1e1269ff ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy 5d5a19028a55 ArmVirtPkg/HighMemDxe: check new regions against GCD memory space map Thanks, ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-21 10:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-21 9:23 [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael Ard Biesheuvel 2017-03-21 9:23 ` [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy Ard Biesheuvel 2017-03-21 10:36 ` Laszlo Ersek 2017-03-21 9:23 ` [PATCH 2/2] ArmVirtPkg/HighMemDxe: check new regions against GCD memory space map Ard Biesheuvel 2017-03-21 10:40 ` Laszlo Ersek 2017-03-21 10:42 ` [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox