* [PATCH 1/3] ArmPkg/UncachedMemoryAllocationLib: restore mapping attributes after free
@ 2017-02-28 12:13 Ard Biesheuvel
2017-02-28 12:13 ` [PATCH 2/3] ArmPkg/UncachedMemoryAllocationLib: use CWG value to align pool allocations Ard Biesheuvel
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2017-02-28 12:13 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: Ard Biesheuvel
In order to play nice with platforms that use strict memory permission
policies, restore the original mapping attributes when freeing uncached
allocations.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
index f6c692f9a403..cd13a7da92e0 100644
--- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
+++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
@@ -42,11 +42,6 @@ UncachedInternalAllocateAlignedPages (
-//
-// Assume all of memory has the same cache attributes, unless we do our magic
-//
-UINT64 gAttributes;
-
typedef struct {
EFI_PHYSICAL_ADDRESS Base;
VOID *Allocation;
@@ -54,6 +49,7 @@ typedef struct {
EFI_MEMORY_TYPE MemoryType;
BOOLEAN Allocated;
LIST_ENTRY Link;
+ UINT64 Attributes;
} FREE_PAGE_NODE;
STATIC LIST_ENTRY mPageList = INITIALIZE_LIST_HEAD_VARIABLE (mPageList);
@@ -153,10 +149,7 @@ AllocatePagesFromList (
}
Status = gDS->GetMemorySpaceDescriptor (Memory, &Descriptor);
- if (!EFI_ERROR (Status)) {
- // We are making an assumption that all of memory has the same default attributes
- gAttributes = Descriptor.Attributes;
- } else {
+ if (EFI_ERROR (Status)) {
gBS->FreePages (Memory, Pages);
return Status;
}
@@ -181,6 +174,7 @@ AllocatePagesFromList (
NewNode->Pages = Pages;
NewNode->Allocated = TRUE;
NewNode->MemoryType = MemoryType;
+ NewNode->Attributes = Descriptor.Attributes;
InsertTailList (&mPageList, &NewNode->Link);
@@ -266,6 +260,10 @@ UncachedMemoryAllocationLibDestructor (
// We only free the non-allocated buffer
if (OldNode->Allocated == FALSE) {
gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)OldNode->Base, OldNode->Pages);
+
+ gDS->SetMemorySpaceAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)OldNode->Base,
+ EFI_PAGES_TO_SIZE (OldNode->Pages), OldNode->Attributes);
+
RemoveEntryList (&OldNode->Link);
FreePool (OldNode);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] ArmPkg/UncachedMemoryAllocationLib: use CWG value to align pool allocations
2017-02-28 12:13 [PATCH 1/3] ArmPkg/UncachedMemoryAllocationLib: restore mapping attributes after free Ard Biesheuvel
@ 2017-02-28 12:13 ` Ard Biesheuvel
2017-03-07 16:49 ` Leif Lindholm
2017-02-28 12:13 ` [PATCH 3/3] ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable Ard Biesheuvel
2017-03-07 16:42 ` [PATCH 1/3] ArmPkg/UncachedMemoryAllocationLib: restore mapping attributes after free Leif Lindholm
2 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2017-02-28 12:13 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: Ard Biesheuvel
Uncached pool allocations are aligned to the data cache line length under
the assumption that this is sufficient to prevent cache maintenance from
corrupting adjacent allocations. However, the value to use in such cases
is architecturally called the Cache Writeback Granule (CWG), which is
essentially the maximum Dcache line length rather than the minimum.
Note that this is mostly a cosmetical fix, given that the pool allocation
is turned into a page allocation later, and rounded up accordingly.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
index cd13a7da92e0..0d8abad23433 100644
--- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
+++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
@@ -545,7 +545,7 @@ UncachedInternalAllocatePool (
IN UINTN AllocationSize
)
{
- UINTN CacheLineLength = ArmDataCacheLineLength ();
+ UINTN CacheLineLength = ArmCacheWritebackGranule ();
return UncachedInternalAllocateAlignedPool (MemoryType, AllocationSize, CacheLineLength);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable
2017-02-28 12:13 [PATCH 1/3] ArmPkg/UncachedMemoryAllocationLib: restore mapping attributes after free Ard Biesheuvel
2017-02-28 12:13 ` [PATCH 2/3] ArmPkg/UncachedMemoryAllocationLib: use CWG value to align pool allocations Ard Biesheuvel
@ 2017-02-28 12:13 ` Ard Biesheuvel
2017-03-07 16:49 ` Leif Lindholm
2017-03-07 16:42 ` [PATCH 1/3] ArmPkg/UncachedMemoryAllocationLib: restore mapping attributes after free Leif Lindholm
2 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2017-02-28 12:13 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: Ard Biesheuvel
The primary use case for UncachedMemoryAllocationLib is non-coherent DMA,
which implies that such regions are not used to fetch instructions from.
So let's map them as non-executable, to avoid creating a security hole
when the rest of the platform may be enforcing strict memory permissions
on ordinary allocations.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
index 0d8abad23433..b4fbfbcb362b 100644
--- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
+++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
@@ -154,7 +154,8 @@ AllocatePagesFromList (
return Status;
}
- Status = gDS->SetMemorySpaceAttributes (Memory, EFI_PAGES_TO_SIZE (Pages), EFI_MEMORY_WC);
+ Status = gDS->SetMemorySpaceAttributes (Memory, EFI_PAGES_TO_SIZE (Pages),
+ EFI_MEMORY_WC | EFI_MEMORY_XP);
if (EFI_ERROR (Status)) {
gBS->FreePages (Memory, Pages);
return Status;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] ArmPkg/UncachedMemoryAllocationLib: restore mapping attributes after free
2017-02-28 12:13 [PATCH 1/3] ArmPkg/UncachedMemoryAllocationLib: restore mapping attributes after free Ard Biesheuvel
2017-02-28 12:13 ` [PATCH 2/3] ArmPkg/UncachedMemoryAllocationLib: use CWG value to align pool allocations Ard Biesheuvel
2017-02-28 12:13 ` [PATCH 3/3] ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable Ard Biesheuvel
@ 2017-03-07 16:42 ` Leif Lindholm
2 siblings, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2017-03-07 16:42 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
On Tue, Feb 28, 2017 at 12:13:10PM +0000, Ard Biesheuvel wrote:
> In order to play nice with platforms that use strict memory permission
> policies, restore the original mapping attributes when freeing uncached
> allocations.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> index f6c692f9a403..cd13a7da92e0 100644
> --- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> +++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> @@ -42,11 +42,6 @@ UncachedInternalAllocateAlignedPages (
>
>
>
> -//
> -// Assume all of memory has the same cache attributes, unless we do our magic
> -//
> -UINT64 gAttributes;
> -
> typedef struct {
> EFI_PHYSICAL_ADDRESS Base;
> VOID *Allocation;
> @@ -54,6 +49,7 @@ typedef struct {
> EFI_MEMORY_TYPE MemoryType;
> BOOLEAN Allocated;
> LIST_ENTRY Link;
> + UINT64 Attributes;
> } FREE_PAGE_NODE;
>
> STATIC LIST_ENTRY mPageList = INITIALIZE_LIST_HEAD_VARIABLE (mPageList);
> @@ -153,10 +149,7 @@ AllocatePagesFromList (
> }
>
> Status = gDS->GetMemorySpaceDescriptor (Memory, &Descriptor);
> - if (!EFI_ERROR (Status)) {
> - // We are making an assumption that all of memory has the same default attributes
> - gAttributes = Descriptor.Attributes;
> - } else {
> + if (EFI_ERROR (Status)) {
> gBS->FreePages (Memory, Pages);
> return Status;
> }
> @@ -181,6 +174,7 @@ AllocatePagesFromList (
> NewNode->Pages = Pages;
> NewNode->Allocated = TRUE;
> NewNode->MemoryType = MemoryType;
> + NewNode->Attributes = Descriptor.Attributes;
>
> InsertTailList (&mPageList, &NewNode->Link);
>
> @@ -266,6 +260,10 @@ UncachedMemoryAllocationLibDestructor (
> // We only free the non-allocated buffer
> if (OldNode->Allocated == FALSE) {
> gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)OldNode->Base, OldNode->Pages);
> +
> + gDS->SetMemorySpaceAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)OldNode->Base,
> + EFI_PAGES_TO_SIZE (OldNode->Pages), OldNode->Attributes);
> +
> RemoveEntryList (&OldNode->Link);
> FreePool (OldNode);
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] ArmPkg/UncachedMemoryAllocationLib: use CWG value to align pool allocations
2017-02-28 12:13 ` [PATCH 2/3] ArmPkg/UncachedMemoryAllocationLib: use CWG value to align pool allocations Ard Biesheuvel
@ 2017-03-07 16:49 ` Leif Lindholm
0 siblings, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2017-03-07 16:49 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
On Tue, Feb 28, 2017 at 12:13:11PM +0000, Ard Biesheuvel wrote:
> Uncached pool allocations are aligned to the data cache line length under
> the assumption that this is sufficient to prevent cache maintenance from
> corrupting adjacent allocations. However, the value to use in such cases
> is architecturally called the Cache Writeback Granule (CWG), which is
> essentially the maximum Dcache line length rather than the minimum.
>
> Note that this is mostly a cosmetical fix, given that the pool allocation
> is turned into a page allocation later, and rounded up accordingly.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> index cd13a7da92e0..0d8abad23433 100644
> --- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> +++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> @@ -545,7 +545,7 @@ UncachedInternalAllocatePool (
> IN UINTN AllocationSize
> )
> {
> - UINTN CacheLineLength = ArmDataCacheLineLength ();
> + UINTN CacheLineLength = ArmCacheWritebackGranule ();
> return UncachedInternalAllocateAlignedPool (MemoryType, AllocationSize, CacheLineLength);
> }
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable
2017-02-28 12:13 ` [PATCH 3/3] ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable Ard Biesheuvel
@ 2017-03-07 16:49 ` Leif Lindholm
0 siblings, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2017-03-07 16:49 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
On Tue, Feb 28, 2017 at 12:13:12PM +0000, Ard Biesheuvel wrote:
> The primary use case for UncachedMemoryAllocationLib is non-coherent DMA,
> which implies that such regions are not used to fetch instructions from.
>
> So let's map them as non-executable, to avoid creating a security hole
> when the rest of the platform may be enforcing strict memory permissions
> on ordinary allocations.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> index 0d8abad23433..b4fbfbcb362b 100644
> --- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> +++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> @@ -154,7 +154,8 @@ AllocatePagesFromList (
> return Status;
> }
>
> - Status = gDS->SetMemorySpaceAttributes (Memory, EFI_PAGES_TO_SIZE (Pages), EFI_MEMORY_WC);
> + Status = gDS->SetMemorySpaceAttributes (Memory, EFI_PAGES_TO_SIZE (Pages),
> + EFI_MEMORY_WC | EFI_MEMORY_XP);
> if (EFI_ERROR (Status)) {
> gBS->FreePages (Memory, Pages);
> return Status;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-07 16:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-28 12:13 [PATCH 1/3] ArmPkg/UncachedMemoryAllocationLib: restore mapping attributes after free Ard Biesheuvel
2017-02-28 12:13 ` [PATCH 2/3] ArmPkg/UncachedMemoryAllocationLib: use CWG value to align pool allocations Ard Biesheuvel
2017-03-07 16:49 ` Leif Lindholm
2017-02-28 12:13 ` [PATCH 3/3] ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable Ard Biesheuvel
2017-03-07 16:49 ` Leif Lindholm
2017-03-07 16:42 ` [PATCH 1/3] ArmPkg/UncachedMemoryAllocationLib: restore mapping attributes after free Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox