* [PATCH 1/4] MdePkg: Add Memory Attribute Protocol definition
2023-01-31 22:35 [PATCH 0/4] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
@ 2023-01-31 22:35 ` Ard Biesheuvel
2023-02-02 3:19 ` 回复: " gaoliming
2023-01-31 22:35 ` [PATCH 2/4] MdePkg: Bump implemented UEFI version to v2.10 Ard Biesheuvel
` (2 subsequent siblings)
3 siblings, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2023-01-31 22:35 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar
From: Sean Brogan <sean.brogan@microsoft.com>
Add the Memory Attribute Protocol definition, which was adopted and
included in version 2.10 of the UEFI specification.
Taken from Project Mu's mu_basecore repository.
Link: https://bugzilla.tianocore.org/show_bug.cgi?id=3519
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdePkg/Include/Protocol/MemoryAttribute.h | 131 ++++++++++++++++++++
MdePkg/MdePkg.dec | 3 +
2 files changed, 134 insertions(+)
diff --git a/MdePkg/Include/Protocol/MemoryAttribute.h b/MdePkg/Include/Protocol/MemoryAttribute.h
new file mode 100644
index 000000000000..b33138732ad1
--- /dev/null
+++ b/MdePkg/Include/Protocol/MemoryAttribute.h
@@ -0,0 +1,131 @@
+/** @file
+
+ EFI Memory Attribute Protocol provides retrieval and update service
+ for memory attributes in EFI environment.
+
+ Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __EFI_MEMORY_ATTRIBUTE_H__
+#define __EFI_MEMORY_ATTRIBUTE_H__
+
+#define EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID \
+ { \
+ 0xf4560cf6, 0x40ec, 0x4b4a, { 0xa1, 0x92, 0xbf, 0x1d, 0x57, 0xd0, 0xb1, 0x89 } \
+ }
+
+typedef struct _EFI_MEMORY_ATTRIBUTE_PROTOCOL EFI_MEMORY_ATTRIBUTE_PROTOCOL;
+
+/**
+ This function set given attributes of the memory region specified by
+ BaseAddress and Length.
+
+ The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes The bit mask of attributes to set for the memory
+ region.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes specified an illegal combination of
+ attributes that cannot be set together.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+ The bit mask of attributes is not supported for
+ the memory resource range specified by
+ BaseAddress and Length.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_SET_MEMORY_ATTRIBUTES)(
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ );
+
+/**
+ This function clears given attributes of the memory region specified by
+ BaseAddress and Length.
+
+ The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes The bit mask of attributes to clear for the memory
+ region.
+
+ @retval EFI_SUCCESS The attributes were cleared for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes specified an illegal combination of
+ attributes that cannot be cleared together.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+ The bit mask of attributes is not supported for
+ the memory resource range specified by
+ BaseAddress and Length.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_CLEAR_MEMORY_ATTRIBUTES)(
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ );
+
+/**
+ This function retrieves the attributes of the memory region specified by
+ BaseAddress and Length. If different attributes are got from different part
+ of the memory region, EFI_NO_MAPPING will be returned.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes Pointer to attributes returned.
+
+ @retval EFI_SUCCESS The attributes got for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes is NULL.
+ @retval EFI_NO_MAPPING Attributes are not consistent cross the memory
+ region.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_GET_MEMORY_ATTRIBUTES)(
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ OUT UINT64 *Attributes
+ );
+
+///
+/// EFI Memory Attribute Protocol provides services to retrieve or update
+/// attribute of memory in the EFI environment.
+///
+struct _EFI_MEMORY_ATTRIBUTE_PROTOCOL {
+ EFI_GET_MEMORY_ATTRIBUTES GetMemoryAttributes;
+ EFI_SET_MEMORY_ATTRIBUTES SetMemoryAttributes;
+ EFI_CLEAR_MEMORY_ATTRIBUTES ClearMemoryAttributes;
+};
+
+extern EFI_GUID gEfiMemoryAttributeProtocolGuid;
+
+#endif
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 3d08f20d15b0..a8658403c8fd 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -1915,6 +1915,9 @@ [Protocols]
## Include/Protocol/RedfishDiscover.h
gEfiRedfishDiscoverProtocolGuid = { 0x5db12509, 0x4550, 0x4347, { 0x96, 0xb3, 0x73, 0xc0, 0xff, 0x6e, 0x86, 0x9f }}
+ ## Include/Protocol/MemoryAttribute.h
+ gEfiMemoryAttributeProtocolGuid = { 0xf4560cf6, 0x40ec, 0x4b4a, { 0xa1, 0x92, 0xbf, 0x1d, 0x57, 0xd0, 0xb1, 0x89 }}
+
#
# Protocols defined in Shell2.0
#
--
2.39.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* 回复: [PATCH 1/4] MdePkg: Add Memory Attribute Protocol definition
2023-01-31 22:35 ` [PATCH 1/4] MdePkg: Add Memory Attribute Protocol definition Ard Biesheuvel
@ 2023-02-02 3:19 ` gaoliming
2023-02-02 9:25 ` [edk2-devel] " Ard Biesheuvel
0 siblings, 1 reply; 24+ messages in thread
From: gaoliming @ 2023-02-02 3:19 UTC (permalink / raw)
To: 'Ard Biesheuvel', devel
Cc: 'Michael Kinney', 'Jiewen Yao',
'Michael Kubacki', 'Sean Brogan',
'Rebecca Cran', 'Leif Lindholm',
'Sami Mujawar'
Ard:
I check this protocol definition in UEFI2.10 spec. GetMemoryAttributes and
SetMemoryAttributes API return status include EFI_OUT_OF_RESOURCES and
EFI_ACCESS_DENIED. But, they are missing in this patch. Can you help
confirm?
Thanks
Liming
> -----邮件原件-----
> 发件人: Ard Biesheuvel <ardb@kernel.org>
> 发送时间: 2023年2月1日 6:36
> 收件人: devel@edk2.groups.io
> 抄送: Ard Biesheuvel <ardb@kernel.org>; Michael Kinney
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Jiewen Yao <jiewen.yao@intel.com>; Michael Kubacki
> <michael.kubacki@microsoft.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Rebecca Cran <quic_rcran@quicinc.com>;
> Leif Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar
> <sami.mujawar@arm.com>
> 主题: [PATCH 1/4] MdePkg: Add Memory Attribute Protocol definition
>
> From: Sean Brogan <sean.brogan@microsoft.com>
>
> Add the Memory Attribute Protocol definition, which was adopted and
> included in version 2.10 of the UEFI specification.
>
> Taken from Project Mu's mu_basecore repository.
>
> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=3519
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdePkg/Include/Protocol/MemoryAttribute.h | 131
> ++++++++++++++++++++
> MdePkg/MdePkg.dec | 3 +
> 2 files changed, 134 insertions(+)
>
> diff --git a/MdePkg/Include/Protocol/MemoryAttribute.h
> b/MdePkg/Include/Protocol/MemoryAttribute.h
> new file mode 100644
> index 000000000000..b33138732ad1
> --- /dev/null
> +++ b/MdePkg/Include/Protocol/MemoryAttribute.h
> @@ -0,0 +1,131 @@
> +/** @file
>
> +
>
> + EFI Memory Attribute Protocol provides retrieval and update service
>
> + for memory attributes in EFI environment.
>
> +
>
> + Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
>
> + SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +
>
> +**/
>
> +
>
> +#ifndef __EFI_MEMORY_ATTRIBUTE_H__
>
> +#define __EFI_MEMORY_ATTRIBUTE_H__
>
> +
>
> +#define EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID \
>
> + { \
>
> + 0xf4560cf6, 0x40ec, 0x4b4a, { 0xa1, 0x92, 0xbf, 0x1d, 0x57, 0xd0,
0xb1,
> 0x89 } \
>
> + }
>
> +
>
> +typedef struct _EFI_MEMORY_ATTRIBUTE_PROTOCOL
> EFI_MEMORY_ATTRIBUTE_PROTOCOL;
>
> +
>
> +/**
>
> + This function set given attributes of the memory region specified by
>
> + BaseAddress and Length.
>
> +
>
> + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and
> EFI_MEMORY_RO.
>
> +
>
> + @param This The
> EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
>
> + @param BaseAddress The physical address that is the start
> address of
>
> + a memory region.
>
> + @param Length The size in bytes of the memory region.
>
> + @param Attributes The bit mask of attributes to set for the
> memory
>
> + region.
>
> +
>
> + @retval EFI_SUCCESS The attributes were set for the
> memory region.
>
> + @retval EFI_INVALID_PARAMETER Length is zero.
>
> + Attributes specified an illegal
> combination of
>
> + attributes that cannot be set
> together.
>
> + @retval EFI_UNSUPPORTED The processor does not support one
> or more
>
> + bytes of the memory resource range
> specified
>
> + by BaseAddress and Length.
>
> + The bit mask of attributes is not
> supported for
>
> + the memory resource range
> specified by
>
> + BaseAddress and Length.
>
> +
>
> +**/
>
> +typedef
>
> +EFI_STATUS
>
> +(EFIAPI *EFI_SET_MEMORY_ATTRIBUTES)(
>
> + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
>
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>
> + IN UINT64 Length,
>
> + IN UINT64 Attributes
>
> + );
>
> +
>
> +/**
>
> + This function clears given attributes of the memory region specified by
>
> + BaseAddress and Length.
>
> +
>
> + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and
> EFI_MEMORY_RO.
>
> +
>
> + @param This The
> EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
>
> + @param BaseAddress The physical address that is the start
> address of
>
> + a memory region.
>
> + @param Length The size in bytes of the memory region.
>
> + @param Attributes The bit mask of attributes to clear for the
> memory
>
> + region.
>
> +
>
> + @retval EFI_SUCCESS The attributes were cleared for the
> memory region.
>
> + @retval EFI_INVALID_PARAMETER Length is zero.
>
> + Attributes specified an illegal
> combination of
>
> + attributes that cannot be cleared
> together.
>
> + @retval EFI_UNSUPPORTED The processor does not support one
> or more
>
> + bytes of the memory resource range
> specified
>
> + by BaseAddress and Length.
>
> + The bit mask of attributes is not
> supported for
>
> + the memory resource range
> specified by
>
> + BaseAddress and Length.
>
> +
>
> +**/
>
> +typedef
>
> +EFI_STATUS
>
> +(EFIAPI *EFI_CLEAR_MEMORY_ATTRIBUTES)(
>
> + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
>
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>
> + IN UINT64 Length,
>
> + IN UINT64 Attributes
>
> + );
>
> +
>
> +/**
>
> + This function retrieves the attributes of the memory region specified
by
>
> + BaseAddress and Length. If different attributes are got from different
part
>
> + of the memory region, EFI_NO_MAPPING will be returned.
>
> +
>
> + @param This The
> EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
>
> + @param BaseAddress The physical address that is the start
> address of
>
> + a memory region.
>
> + @param Length The size in bytes of the memory region.
>
> + @param Attributes Pointer to attributes returned.
>
> +
>
> + @retval EFI_SUCCESS The attributes got for the memory
> region.
>
> + @retval EFI_INVALID_PARAMETER Length is zero.
>
> + Attributes is NULL.
>
> + @retval EFI_NO_MAPPING Attributes are not consistent cross
> the memory
>
> + region.
>
> + @retval EFI_UNSUPPORTED The processor does not support one
> or more
>
> + bytes of the memory resource range
> specified
>
> + by BaseAddress and Length.
>
> +
>
> +**/
>
> +typedef
>
> +EFI_STATUS
>
> +(EFIAPI *EFI_GET_MEMORY_ATTRIBUTES)(
>
> + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
>
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>
> + IN UINT64 Length,
>
> + OUT UINT64 *Attributes
>
> + );
>
> +
>
> +///
>
> +/// EFI Memory Attribute Protocol provides services to retrieve or update
>
> +/// attribute of memory in the EFI environment.
>
> +///
>
> +struct _EFI_MEMORY_ATTRIBUTE_PROTOCOL {
>
> + EFI_GET_MEMORY_ATTRIBUTES GetMemoryAttributes;
>
> + EFI_SET_MEMORY_ATTRIBUTES SetMemoryAttributes;
>
> + EFI_CLEAR_MEMORY_ATTRIBUTES ClearMemoryAttributes;
>
> +};
>
> +
>
> +extern EFI_GUID gEfiMemoryAttributeProtocolGuid;
>
> +
>
> +#endif
>
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 3d08f20d15b0..a8658403c8fd 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -1915,6 +1915,9 @@ [Protocols]
> ## Include/Protocol/RedfishDiscover.h
>
> gEfiRedfishDiscoverProtocolGuid = { 0x5db12509, 0x4550, 0x4347,
> { 0x96, 0xb3, 0x73, 0xc0, 0xff, 0x6e, 0x86, 0x9f }}
>
>
>
> + ## Include/Protocol/MemoryAttribute.h
>
> + gEfiMemoryAttributeProtocolGuid = { 0xf4560cf6, 0x40ec, 0x4b4a, { 0xa1,
> 0x92, 0xbf, 0x1d, 0x57, 0xd0, 0xb1, 0x89 }}
>
> +
>
> #
>
> # Protocols defined in Shell2.0
>
> #
>
> --
> 2.39.0
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] 回复: [PATCH 1/4] MdePkg: Add Memory Attribute Protocol definition
2023-02-02 3:19 ` 回复: " gaoliming
@ 2023-02-02 9:25 ` Ard Biesheuvel
0 siblings, 0 replies; 24+ messages in thread
From: Ard Biesheuvel @ 2023-02-02 9:25 UTC (permalink / raw)
To: devel, gaoliming
Cc: Michael Kinney, Jiewen Yao, Michael Kubacki, Sean Brogan,
Rebecca Cran, Leif Lindholm, Sami Mujawar
On Thu, 2 Feb 2023 at 04:19, gaoliming via groups.io
<gaoliming=byosoft.com.cn@groups.io> wrote:
>
> Ard:
> I check this protocol definition in UEFI2.10 spec. GetMemoryAttributes and
> SetMemoryAttributes API return status include EFI_OUT_OF_RESOURCES and
> EFI_ACCESS_DENIED. But, they are missing in this patch. Can you help
> confirm?
>
I just took the Project Mu version without realizing this - I can fix
that up in v2.
> > -----邮件原件-----
> > 发件人: Ard Biesheuvel <ardb@kernel.org>
> > 发送时间: 2023年2月1日 6:36
> > 收件人: devel@edk2.groups.io
> > 抄送: Ard Biesheuvel <ardb@kernel.org>; Michael Kinney
> > <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> > Jiewen Yao <jiewen.yao@intel.com>; Michael Kubacki
> > <michael.kubacki@microsoft.com>; Sean Brogan
> > <sean.brogan@microsoft.com>; Rebecca Cran <quic_rcran@quicinc.com>;
> > Leif Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar
> > <sami.mujawar@arm.com>
> > 主题: [PATCH 1/4] MdePkg: Add Memory Attribute Protocol definition
> >
> > From: Sean Brogan <sean.brogan@microsoft.com>
> >
> > Add the Memory Attribute Protocol definition, which was adopted and
> > included in version 2.10 of the UEFI specification.
> >
> > Taken from Project Mu's mu_basecore repository.
> >
> > Link: https://bugzilla.tianocore.org/show_bug.cgi?id=3519
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > MdePkg/Include/Protocol/MemoryAttribute.h | 131
> > ++++++++++++++++++++
> > MdePkg/MdePkg.dec | 3 +
> > 2 files changed, 134 insertions(+)
> >
> > diff --git a/MdePkg/Include/Protocol/MemoryAttribute.h
> > b/MdePkg/Include/Protocol/MemoryAttribute.h
> > new file mode 100644
> > index 000000000000..b33138732ad1
> > --- /dev/null
> > +++ b/MdePkg/Include/Protocol/MemoryAttribute.h
> > @@ -0,0 +1,131 @@
> > +/** @file
> >
> > +
> >
> > + EFI Memory Attribute Protocol provides retrieval and update service
> >
> > + for memory attributes in EFI environment.
> >
> > +
> >
> > + Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
> >
> > + SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > +
> >
> > +**/
> >
> > +
> >
> > +#ifndef __EFI_MEMORY_ATTRIBUTE_H__
> >
> > +#define __EFI_MEMORY_ATTRIBUTE_H__
> >
> > +
> >
> > +#define EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID \
> >
> > + { \
> >
> > + 0xf4560cf6, 0x40ec, 0x4b4a, { 0xa1, 0x92, 0xbf, 0x1d, 0x57, 0xd0,
> 0xb1,
> > 0x89 } \
> >
> > + }
> >
> > +
> >
> > +typedef struct _EFI_MEMORY_ATTRIBUTE_PROTOCOL
> > EFI_MEMORY_ATTRIBUTE_PROTOCOL;
> >
> > +
> >
> > +/**
> >
> > + This function set given attributes of the memory region specified by
> >
> > + BaseAddress and Length.
> >
> > +
> >
> > + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and
> > EFI_MEMORY_RO.
> >
> > +
> >
> > + @param This The
> > EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> >
> > + @param BaseAddress The physical address that is the start
> > address of
> >
> > + a memory region.
> >
> > + @param Length The size in bytes of the memory region.
> >
> > + @param Attributes The bit mask of attributes to set for the
> > memory
> >
> > + region.
> >
> > +
> >
> > + @retval EFI_SUCCESS The attributes were set for the
> > memory region.
> >
> > + @retval EFI_INVALID_PARAMETER Length is zero.
> >
> > + Attributes specified an illegal
> > combination of
> >
> > + attributes that cannot be set
> > together.
> >
> > + @retval EFI_UNSUPPORTED The processor does not support one
> > or more
> >
> > + bytes of the memory resource range
> > specified
> >
> > + by BaseAddress and Length.
> >
> > + The bit mask of attributes is not
> > supported for
> >
> > + the memory resource range
> > specified by
> >
> > + BaseAddress and Length.
> >
> > +
> >
> > +**/
> >
> > +typedef
> >
> > +EFI_STATUS
> >
> > +(EFIAPI *EFI_SET_MEMORY_ATTRIBUTES)(
> >
> > + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
> >
> > + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> >
> > + IN UINT64 Length,
> >
> > + IN UINT64 Attributes
> >
> > + );
> >
> > +
> >
> > +/**
> >
> > + This function clears given attributes of the memory region specified by
> >
> > + BaseAddress and Length.
> >
> > +
> >
> > + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and
> > EFI_MEMORY_RO.
> >
> > +
> >
> > + @param This The
> > EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> >
> > + @param BaseAddress The physical address that is the start
> > address of
> >
> > + a memory region.
> >
> > + @param Length The size in bytes of the memory region.
> >
> > + @param Attributes The bit mask of attributes to clear for the
> > memory
> >
> > + region.
> >
> > +
> >
> > + @retval EFI_SUCCESS The attributes were cleared for the
> > memory region.
> >
> > + @retval EFI_INVALID_PARAMETER Length is zero.
> >
> > + Attributes specified an illegal
> > combination of
> >
> > + attributes that cannot be cleared
> > together.
> >
> > + @retval EFI_UNSUPPORTED The processor does not support one
> > or more
> >
> > + bytes of the memory resource range
> > specified
> >
> > + by BaseAddress and Length.
> >
> > + The bit mask of attributes is not
> > supported for
> >
> > + the memory resource range
> > specified by
> >
> > + BaseAddress and Length.
> >
> > +
> >
> > +**/
> >
> > +typedef
> >
> > +EFI_STATUS
> >
> > +(EFIAPI *EFI_CLEAR_MEMORY_ATTRIBUTES)(
> >
> > + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
> >
> > + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> >
> > + IN UINT64 Length,
> >
> > + IN UINT64 Attributes
> >
> > + );
> >
> > +
> >
> > +/**
> >
> > + This function retrieves the attributes of the memory region specified
> by
> >
> > + BaseAddress and Length. If different attributes are got from different
> part
> >
> > + of the memory region, EFI_NO_MAPPING will be returned.
> >
> > +
> >
> > + @param This The
> > EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> >
> > + @param BaseAddress The physical address that is the start
> > address of
> >
> > + a memory region.
> >
> > + @param Length The size in bytes of the memory region.
> >
> > + @param Attributes Pointer to attributes returned.
> >
> > +
> >
> > + @retval EFI_SUCCESS The attributes got for the memory
> > region.
> >
> > + @retval EFI_INVALID_PARAMETER Length is zero.
> >
> > + Attributes is NULL.
> >
> > + @retval EFI_NO_MAPPING Attributes are not consistent cross
> > the memory
> >
> > + region.
> >
> > + @retval EFI_UNSUPPORTED The processor does not support one
> > or more
> >
> > + bytes of the memory resource range
> > specified
> >
> > + by BaseAddress and Length.
> >
> > +
> >
> > +**/
> >
> > +typedef
> >
> > +EFI_STATUS
> >
> > +(EFIAPI *EFI_GET_MEMORY_ATTRIBUTES)(
> >
> > + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
> >
> > + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> >
> > + IN UINT64 Length,
> >
> > + OUT UINT64 *Attributes
> >
> > + );
> >
> > +
> >
> > +///
> >
> > +/// EFI Memory Attribute Protocol provides services to retrieve or update
> >
> > +/// attribute of memory in the EFI environment.
> >
> > +///
> >
> > +struct _EFI_MEMORY_ATTRIBUTE_PROTOCOL {
> >
> > + EFI_GET_MEMORY_ATTRIBUTES GetMemoryAttributes;
> >
> > + EFI_SET_MEMORY_ATTRIBUTES SetMemoryAttributes;
> >
> > + EFI_CLEAR_MEMORY_ATTRIBUTES ClearMemoryAttributes;
> >
> > +};
> >
> > +
> >
> > +extern EFI_GUID gEfiMemoryAttributeProtocolGuid;
> >
> > +
> >
> > +#endif
> >
> > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > index 3d08f20d15b0..a8658403c8fd 100644
> > --- a/MdePkg/MdePkg.dec
> > +++ b/MdePkg/MdePkg.dec
> > @@ -1915,6 +1915,9 @@ [Protocols]
> > ## Include/Protocol/RedfishDiscover.h
> >
> > gEfiRedfishDiscoverProtocolGuid = { 0x5db12509, 0x4550, 0x4347,
> > { 0x96, 0xb3, 0x73, 0xc0, 0xff, 0x6e, 0x86, 0x9f }}
> >
> >
> >
> > + ## Include/Protocol/MemoryAttribute.h
> >
> > + gEfiMemoryAttributeProtocolGuid = { 0xf4560cf6, 0x40ec, 0x4b4a, { 0xa1,
> > 0x92, 0xbf, 0x1d, 0x57, 0xd0, 0xb1, 0x89 }}
> >
> > +
> >
> > #
> >
> > # Protocols defined in Shell2.0
> >
> > #
> >
> > --
> > 2.39.0
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/4] MdePkg: Bump implemented UEFI version to v2.10
2023-01-31 22:35 [PATCH 0/4] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
2023-01-31 22:35 ` [PATCH 1/4] MdePkg: Add Memory Attribute Protocol definition Ard Biesheuvel
@ 2023-01-31 22:35 ` Ard Biesheuvel
2023-02-01 0:10 ` Michael D Kinney
2023-01-31 22:35 ` [PATCH 3/4] ArmPkg/CpuDxe: Unify PageAttributeToGcdAttribute helper Ard Biesheuvel
2023-01-31 22:35 ` [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Ard Biesheuvel
3 siblings, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2023-01-31 22:35 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar
Now that we are adding implementations of protocols added in v2.10,
let's bump the version we expose to v2.10 as well.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdePkg/Include/Uefi/UefiSpec.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
index 3abebbb8d904..9fef5bde4b2d 100644
--- a/MdePkg/Include/Uefi/UefiSpec.h
+++ b/MdePkg/Include/Uefi/UefiSpec.h
@@ -1789,6 +1789,8 @@ EFI_STATUS
// EFI Runtime Services Table
//
#define EFI_SYSTEM_TABLE_SIGNATURE SIGNATURE_64 ('I','B','I',' ','S','Y','S','T')
+#define EFI_2_100_SYSTEM_TABLE_REVISION ((2 << 16) | (100))
+#define EFI_2_90_SYSTEM_TABLE_REVISION ((2 << 16) | (90))
#define EFI_2_80_SYSTEM_TABLE_REVISION ((2 << 16) | (80))
#define EFI_2_70_SYSTEM_TABLE_REVISION ((2 << 16) | (70))
#define EFI_2_60_SYSTEM_TABLE_REVISION ((2 << 16) | (60))
@@ -1801,7 +1803,7 @@ EFI_STATUS
#define EFI_2_00_SYSTEM_TABLE_REVISION ((2 << 16) | (00))
#define EFI_1_10_SYSTEM_TABLE_REVISION ((1 << 16) | (10))
#define EFI_1_02_SYSTEM_TABLE_REVISION ((1 << 16) | (02))
-#define EFI_SYSTEM_TABLE_REVISION EFI_2_70_SYSTEM_TABLE_REVISION
+#define EFI_SYSTEM_TABLE_REVISION EFI_2_100_SYSTEM_TABLE_REVISION
#define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
#define EFI_RUNTIME_SERVICES_SIGNATURE SIGNATURE_64 ('R','U','N','T','S','E','R','V')
--
2.39.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 2/4] MdePkg: Bump implemented UEFI version to v2.10
2023-01-31 22:35 ` [PATCH 2/4] MdePkg: Bump implemented UEFI version to v2.10 Ard Biesheuvel
@ 2023-02-01 0:10 ` Michael D Kinney
2023-02-01 7:54 ` Ard Biesheuvel
0 siblings, 1 reply; 24+ messages in thread
From: Michael D Kinney @ 2023-02-01 0:10 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io
Cc: Gao, Liming, Yao, Jiewen, Kubacki, Michael, Sean Brogan,
Rebecca Cran, Leif Lindholm, Sami Mujawar, Kinney, Michael D
Hi Ard,
Have you verified that all the content from UEFI 2.10 has been added and that
all the behaviors of the edk2 implementations match UEFI 2.10?
What additional work items remain?
Thanks,
Mike
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Tuesday, January 31, 2023 2:36 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Yao, Jiewen <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>;
> Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <quic_rcran@quicinc.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>
> Subject: [PATCH 2/4] MdePkg: Bump implemented UEFI version to v2.10
>
> Now that we are adding implementations of protocols added in v2.10,
> let's bump the version we expose to v2.10 as well.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdePkg/Include/Uefi/UefiSpec.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
> index 3abebbb8d904..9fef5bde4b2d 100644
> --- a/MdePkg/Include/Uefi/UefiSpec.h
> +++ b/MdePkg/Include/Uefi/UefiSpec.h
> @@ -1789,6 +1789,8 @@ EFI_STATUS
> // EFI Runtime Services Table
>
> //
>
> #define EFI_SYSTEM_TABLE_SIGNATURE SIGNATURE_64 ('I','B','I',' ','S','Y','S','T')
>
> +#define EFI_2_100_SYSTEM_TABLE_REVISION ((2 << 16) | (100))
>
> +#define EFI_2_90_SYSTEM_TABLE_REVISION ((2 << 16) | (90))
>
> #define EFI_2_80_SYSTEM_TABLE_REVISION ((2 << 16) | (80))
>
> #define EFI_2_70_SYSTEM_TABLE_REVISION ((2 << 16) | (70))
>
> #define EFI_2_60_SYSTEM_TABLE_REVISION ((2 << 16) | (60))
>
> @@ -1801,7 +1803,7 @@ EFI_STATUS
> #define EFI_2_00_SYSTEM_TABLE_REVISION ((2 << 16) | (00))
>
> #define EFI_1_10_SYSTEM_TABLE_REVISION ((1 << 16) | (10))
>
> #define EFI_1_02_SYSTEM_TABLE_REVISION ((1 << 16) | (02))
>
> -#define EFI_SYSTEM_TABLE_REVISION EFI_2_70_SYSTEM_TABLE_REVISION
>
> +#define EFI_SYSTEM_TABLE_REVISION EFI_2_100_SYSTEM_TABLE_REVISION
>
> #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
>
>
>
> #define EFI_RUNTIME_SERVICES_SIGNATURE SIGNATURE_64 ('R','U','N','T','S','E','R','V')
>
> --
> 2.39.0
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/4] MdePkg: Bump implemented UEFI version to v2.10
2023-02-01 0:10 ` Michael D Kinney
@ 2023-02-01 7:54 ` Ard Biesheuvel
0 siblings, 0 replies; 24+ messages in thread
From: Ard Biesheuvel @ 2023-02-01 7:54 UTC (permalink / raw)
To: Kinney, Michael D
Cc: devel@edk2.groups.io, Gao, Liming, Yao, Jiewen, Kubacki, Michael,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
On Wed, 1 Feb 2023 at 01:10, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard,
>
> Have you verified that all the content from UEFI 2.10 has been added and that
> all the behaviors of the edk2 implementations match UEFI 2.10?
>
> What additional work items remain?
>
Ah no, I didn't quite consider that, tbh. I naively assumed that the
availability of any v2.10 material would justify bumping the version,
but as you ask the question, I realize that not everything is
optional. In particular, things like event groups that need to get
signaled at the right time need to be implemented before claiming
version 2.10 compliance.
I glossed over the changelog, and while there don't seem to be many
items that fit that category, going from v2.7 to v2.10 still requires
a lot of those to be double checked and sadly, I don't have the
bandwidth right now for doing that.
So I'll withdraw this patch. But please ack patch #1 if you are happy
for it to go in.
Thanks,
Ard.
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Tuesday, January 31, 2023 2:36 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Yao, Jiewen <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>;
> > Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <quic_rcran@quicinc.com>; Leif Lindholm
> > <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>
> > Subject: [PATCH 2/4] MdePkg: Bump implemented UEFI version to v2.10
> >
> > Now that we are adding implementations of protocols added in v2.10,
> > let's bump the version we expose to v2.10 as well.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > MdePkg/Include/Uefi/UefiSpec.h | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
> > index 3abebbb8d904..9fef5bde4b2d 100644
> > --- a/MdePkg/Include/Uefi/UefiSpec.h
> > +++ b/MdePkg/Include/Uefi/UefiSpec.h
> > @@ -1789,6 +1789,8 @@ EFI_STATUS
> > // EFI Runtime Services Table
> >
> > //
> >
> > #define EFI_SYSTEM_TABLE_SIGNATURE SIGNATURE_64 ('I','B','I',' ','S','Y','S','T')
> >
> > +#define EFI_2_100_SYSTEM_TABLE_REVISION ((2 << 16) | (100))
> >
> > +#define EFI_2_90_SYSTEM_TABLE_REVISION ((2 << 16) | (90))
> >
> > #define EFI_2_80_SYSTEM_TABLE_REVISION ((2 << 16) | (80))
> >
> > #define EFI_2_70_SYSTEM_TABLE_REVISION ((2 << 16) | (70))
> >
> > #define EFI_2_60_SYSTEM_TABLE_REVISION ((2 << 16) | (60))
> >
> > @@ -1801,7 +1803,7 @@ EFI_STATUS
> > #define EFI_2_00_SYSTEM_TABLE_REVISION ((2 << 16) | (00))
> >
> > #define EFI_1_10_SYSTEM_TABLE_REVISION ((1 << 16) | (10))
> >
> > #define EFI_1_02_SYSTEM_TABLE_REVISION ((1 << 16) | (02))
> >
> > -#define EFI_SYSTEM_TABLE_REVISION EFI_2_70_SYSTEM_TABLE_REVISION
> >
> > +#define EFI_SYSTEM_TABLE_REVISION EFI_2_100_SYSTEM_TABLE_REVISION
> >
> > #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
> >
> >
> >
> > #define EFI_RUNTIME_SERVICES_SIGNATURE SIGNATURE_64 ('R','U','N','T','S','E','R','V')
> >
> > --
> > 2.39.0
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/4] ArmPkg/CpuDxe: Unify PageAttributeToGcdAttribute helper
2023-01-31 22:35 [PATCH 0/4] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
2023-01-31 22:35 ` [PATCH 1/4] MdePkg: Add Memory Attribute Protocol definition Ard Biesheuvel
2023-01-31 22:35 ` [PATCH 2/4] MdePkg: Bump implemented UEFI version to v2.10 Ard Biesheuvel
@ 2023-01-31 22:35 ` Ard Biesheuvel
2023-01-31 22:35 ` [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Ard Biesheuvel
3 siblings, 0 replies; 24+ messages in thread
From: Ard Biesheuvel @ 2023-01-31 22:35 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar
In preparation for introducing an implementation of the EFI memory
attributes protocol that is shared between ARM and AArch64, unify the
existing code that converts a page table descriptor into a
EFI_MEMORY_xxx bitfield, so it can be called from the generic code.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 5 +--
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 46 +++++++++++---------
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 5 +++
3 files changed, 32 insertions(+), 24 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 8bb33046e707..4ec9fc0a582c 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -30,13 +30,12 @@ GetRootTranslationTableInfo (
*RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
}
-STATIC
UINT64
PageAttributeToGcdAttribute (
- IN UINT64 PageAttributes
+ IN UINTN PageAttributes
)
{
- UINT64 GcdAttributes;
+ UINTN GcdAttributes;
switch (PageAttributes & TT_ATTR_INDX_MASK) {
case TT_ATTR_INDX_DEVICE_MEMORY:
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 2daf47ba6fe5..9545a1c1d2d3 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -77,39 +77,46 @@ SectionToGcdAttributes (
return EFI_SUCCESS;
}
-EFI_STATUS
-PageToGcdAttributes (
- IN UINT32 PageAttributes,
- OUT UINT64 *GcdAttributes
+UINT64
+PageAttributeToGcdAttribute (
+ IN UINTN PageAttributes
)
{
- *GcdAttributes = 0;
+ UINT64 GcdAttributes;
// determine cacheability attributes
switch (PageAttributes & TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK) {
case TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED:
- *GcdAttributes |= EFI_MEMORY_UC;
+ GcdAttributes = EFI_MEMORY_UC;
break;
case TT_DESCRIPTOR_PAGE_CACHE_POLICY_SHAREABLE_DEVICE:
- *GcdAttributes |= EFI_MEMORY_UC;
+ GcdAttributes = EFI_MEMORY_UC;
break;
case TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC:
- *GcdAttributes |= EFI_MEMORY_WT;
+ GcdAttributes = EFI_MEMORY_WT;
break;
case TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_NO_ALLOC:
- *GcdAttributes |= EFI_MEMORY_WB;
+ GcdAttributes = EFI_MEMORY_WB;
break;
case TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE:
- *GcdAttributes |= EFI_MEMORY_WC;
+ GcdAttributes = EFI_MEMORY_WC;
break;
case TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC:
- *GcdAttributes |= EFI_MEMORY_WB;
+ GcdAttributes = EFI_MEMORY_WB;
break;
case TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_SHAREABLE_DEVICE:
- *GcdAttributes |= EFI_MEMORY_UC;
+ GcdAttributes = EFI_MEMORY_UC;
break;
default:
- return EFI_UNSUPPORTED;
+ DEBUG ((
+ DEBUG_ERROR,
+ "PageAttributeToGcdAttribute: PageAttributes:0x%X not supported.\n",
+ PageAttributes
+ ));
+ ASSERT (0);
+ // The Global Coherency Domain (GCD) value is defined as a bit set.
+ // Returning 0 means no attribute has been set.
+ GcdAttributes = 0;
}
// determine protection attributes
@@ -126,7 +133,7 @@ PageToGcdAttributes (
// read only cases map to write-protect
case TT_DESCRIPTOR_PAGE_AP_RO_NO:
case TT_DESCRIPTOR_PAGE_AP_RO_RO:
- *GcdAttributes |= EFI_MEMORY_RO;
+ GcdAttributes |= EFI_MEMORY_RO;
break;
default:
@@ -135,10 +142,10 @@ PageToGcdAttributes (
// now process eXectue Never attribute
if ((PageAttributes & TT_DESCRIPTOR_PAGE_XN_MASK) != 0 ) {
- *GcdAttributes |= EFI_MEMORY_XP;
+ GcdAttributes |= EFI_MEMORY_XP;
}
- return EFI_SUCCESS;
+ return GcdAttributes;
}
EFI_STATUS
@@ -152,7 +159,6 @@ SyncCacheConfigPage (
IN OUT UINT32 *NextSectionAttributes
)
{
- EFI_STATUS Status;
UINT32 i;
volatile ARM_PAGE_TABLE_ENTRY *SecondLevelTable;
UINT32 NextPageAttributes;
@@ -183,8 +189,7 @@ SyncCacheConfigPage (
NextPageAttributes = PageAttributes;
} else if (PageAttributes != NextPageAttributes) {
// Convert Section Attributes into GCD Attributes
- Status = PageToGcdAttributes (NextPageAttributes, &GcdAttributes);
- ASSERT_EFI_ERROR (Status);
+ GcdAttributes = PageAttributeToGcdAttribute (NextPageAttributes);
// update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK)
SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, *NextRegionBase, *NextRegionLength, GcdAttributes);
@@ -196,8 +201,7 @@ SyncCacheConfigPage (
}
} else if (NextPageAttributes != 0) {
// Convert Page Attributes into GCD Attributes
- Status = PageToGcdAttributes (NextPageAttributes, &GcdAttributes);
- ASSERT_EFI_ERROR (Status);
+ GcdAttributes = PageAttributeToGcdAttribute (NextPageAttributes);
// update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK)
SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, *NextRegionBase, *NextRegionLength, GcdAttributes);
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index ff672390ce51..8933fa90c4ed 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -126,4 +126,9 @@ SetGcdMemorySpaceAttributes (
IN UINT64 Attributes
);
+UINT64
+PageAttributeToGcdAttribute (
+ IN UINTN PageAttributes
+ );
+
#endif // CPU_DXE_H_
--
2.39.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-01-31 22:35 [PATCH 0/4] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (2 preceding siblings ...)
2023-01-31 22:35 ` [PATCH 3/4] ArmPkg/CpuDxe: Unify PageAttributeToGcdAttribute helper Ard Biesheuvel
@ 2023-01-31 22:35 ` Ard Biesheuvel
2023-02-01 18:41 ` [edk2-devel] " Taylor Beebe
3 siblings, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2023-01-31 22:35 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar
Expose the protocol introduced in v2.10 that permits the caller to
manage mapping permissions in the page tables.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 +
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 3 +
ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +
ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 242 ++++++++++++++++++++
4 files changed, 249 insertions(+)
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index e6742f0a25fc..d04958e79e52 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -244,6 +244,8 @@ CpuDxeInitialize (
&mCpuHandle,
&gEfiCpuArchProtocolGuid,
&mCpu,
+ &gEfiMemoryAttributeProtocolGuid,
+ &mMemoryAttribute,
NULL
);
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index 8933fa90c4ed..f45d2bc101a3 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -30,9 +30,12 @@
#include <Protocol/Cpu.h>
#include <Protocol/DebugSupport.h>
#include <Protocol/LoadedImage.h>
+#include <Protocol/MemoryAttribute.h>
extern BOOLEAN mIsFlushingGCD;
+extern EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute;
+
/**
This function registers and enables the handler specified by InterruptHandler for a processor
interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
index 10792b393fc8..e732e21cb94a 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
@@ -23,6 +23,7 @@ [Sources.Common]
CpuDxe.h
CpuMmuCommon.c
Exception.c
+ MemoryAttribute.c
[Sources.ARM]
Arm/Mmu.c
@@ -53,6 +54,7 @@ [LibraryClasses]
[Protocols]
gEfiCpuArchProtocolGuid
+ gEfiMemoryAttributeProtocolGuid
[Guids]
gEfiDebugImageInfoTableGuid
diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
new file mode 100644
index 000000000000..9aeb83fff1b9
--- /dev/null
+++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
@@ -0,0 +1,242 @@
+/** @file
+
+ Copyright (c) 2023, Google LLC. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "CpuDxe.h"
+
+/**
+ This function retrieves the attributes of the memory region specified by
+ BaseAddress and Length. If different attributes are got from different part
+ of the memory region, EFI_NO_MAPPING will be returned.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes Pointer to attributes returned.
+
+ @retval EFI_SUCCESS The attributes got for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes is NULL.
+ @retval EFI_NO_MAPPING Attributes are not consistent cross the memory
+ region.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+
+**/
+STATIC
+EFI_STATUS
+GetMemoryAttributes (
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ OUT UINT64 *Attributes
+ )
+{
+ UINTN RegionAddress;
+ UINTN RegionLength;
+ UINTN RegionAttributes;
+ UINTN Union;
+ UINTN Intersection;
+ EFI_STATUS Status;
+
+ if ((Length == 0) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ DEBUG ((DEBUG_INFO,
+ "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",
+ __FUNCTION__,
+ (UINTN)BaseAddress,
+ (UINTN)Length
+ ));
+
+ Union = 0;
+ Intersection = MAX_UINTN;
+
+ for (RegionAddress = (UINTN)BaseAddress;
+ RegionAddress < (UINTN)(BaseAddress + Length);
+ RegionAddress += RegionLength) {
+
+ Status = GetMemoryRegion (&RegionAddress,
+ &RegionLength,
+ &RegionAttributes
+ );
+
+ DEBUG ((DEBUG_INFO,
+ "%a: RegionAddress == 0x%lx, RegionLength == 0x%lx, RegionAttributes == 0x%lx\n",
+ __FUNCTION__,
+ RegionAddress,
+ RegionLength,
+ RegionAttributes
+ ));
+
+ if (EFI_ERROR (Status)) {
+ return EFI_NO_MAPPING;
+ }
+
+ Union |= RegionAttributes;
+ Intersection &= RegionAttributes;
+ }
+
+ DEBUG ((DEBUG_INFO,
+ "%a: Union == %lx, Intersection == %lx\n",
+ __FUNCTION__,
+ Union,
+ Intersection
+ ));
+
+ if (Union != Intersection) {
+ return EFI_NO_MAPPING;
+ }
+
+ *Attributes = PageAttributeToGcdAttribute (Union) & (EFI_MEMORY_RO | EFI_MEMORY_XP);
+ return EFI_SUCCESS;
+}
+
+/**
+ This function set given attributes of the memory region specified by
+ BaseAddress and Length.
+
+ The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes The bit mask of attributes to set for the memory
+ region.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes specified an illegal combination of
+ attributes that cannot be set together.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+ The bit mask of attributes is not supported for
+ the memory resource range specified by
+ BaseAddress and Length.
+
+**/
+STATIC
+EFI_STATUS
+SetMemoryAttributes (
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ )
+{
+ EFI_STATUS Status;
+
+ DEBUG ((DEBUG_INFO,
+ "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
+ __FUNCTION__,
+ (UINTN)BaseAddress,
+ (UINTN)Length,
+ (UINTN)Attributes
+ ));
+
+ if ((Length == 0) ||
+ ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if ((Attributes & EFI_MEMORY_RP) != 0) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if ((Attributes & EFI_MEMORY_RO) != 0) {
+ Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ if ((Attributes & EFI_MEMORY_XP) != 0) {
+ Status = ArmSetMemoryRegionNoExec (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
+/**
+ This function clears given attributes of the memory region specified by
+ BaseAddress and Length.
+
+ The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes The bit mask of attributes to clear for the memory
+ region.
+
+ @retval EFI_SUCCESS The attributes were cleared for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes specified an illegal combination of
+ attributes that cannot be cleared together.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+ The bit mask of attributes is not supported for
+ the memory resource range specified by
+ BaseAddress and Length.
+
+**/
+STATIC
+EFI_STATUS
+ClearMemoryAttributes (
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ )
+{
+ EFI_STATUS Status;
+
+ DEBUG ((DEBUG_INFO,
+ "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
+ __FUNCTION__,
+ (UINTN)BaseAddress,
+ (UINTN)Length,
+ (UINTN)Attributes
+ ));
+
+ if ((Length == 0) ||
+ ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if ((Attributes & EFI_MEMORY_RO) != 0) {
+ Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ if ((Attributes & EFI_MEMORY_XP) != 0) {
+ Status = ArmClearMemoryRegionNoExec (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
+EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute = {
+ GetMemoryAttributes,
+ SetMemoryAttributes,
+ ClearMemoryAttributes
+};
--
2.39.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-01-31 22:35 ` [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Ard Biesheuvel
@ 2023-02-01 18:41 ` Taylor Beebe
2023-02-02 9:43 ` Ard Biesheuvel
0 siblings, 1 reply; 24+ messages in thread
From: Taylor Beebe @ 2023-02-01 18:41 UTC (permalink / raw)
To: devel, ardb
Cc: Michael Kinney, Liming Gao, Jiewen Yao, Michael Kubacki,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
Hey Ard,
Have you encountered complications which stem from the lack of
pre-allocated page table memory on ARM devices utilizing the memory
protection policy?
My observation is the call stack can end up something like:
1. MemoryAttributeProtocol->SetMemoryAttributes(EFI_MEMORY_RO)
2. ArmSetMemoryRegionReadOnly ()
3. SetMemoryRegionAttribute()
4. UpdateRegionMapping()
5. UpdateRegionMappingRecursive()
6. AllocatePages() -> Need memory for a translation table entry
7. CoreAllocatePages()
8. ApplyMemoryProtectionPolicy() -> Policy says new page should be XN
9. gCpu->SetMemoryAttributes()
10. Back to 3 and loop infinitely
Thanks!
-Taylor
On 1/31/2023 2:35 PM, Ard Biesheuvel wrote:
> Expose the protocol introduced in v2.10 that permits the caller to
> manage mapping permissions in the page tables.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 +
> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 3 +
> ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +
> ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 242 ++++++++++++++++++++
> 4 files changed, 249 insertions(+)
>
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> index e6742f0a25fc..d04958e79e52 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> @@ -244,6 +244,8 @@ CpuDxeInitialize (
> &mCpuHandle,
>
> &gEfiCpuArchProtocolGuid,
>
> &mCpu,
>
> + &gEfiMemoryAttributeProtocolGuid,
>
> + &mMemoryAttribute,
>
> NULL
>
> );
>
>
>
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> index 8933fa90c4ed..f45d2bc101a3 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> @@ -30,9 +30,12 @@
> #include <Protocol/Cpu.h>
>
> #include <Protocol/DebugSupport.h>
>
> #include <Protocol/LoadedImage.h>
>
> +#include <Protocol/MemoryAttribute.h>
>
>
>
> extern BOOLEAN mIsFlushingGCD;
>
>
>
> +extern EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute;
>
> +
>
> /**
>
> This function registers and enables the handler specified by InterruptHandler for a processor
>
> interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
>
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> index 10792b393fc8..e732e21cb94a 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> @@ -23,6 +23,7 @@ [Sources.Common]
> CpuDxe.h
>
> CpuMmuCommon.c
>
> Exception.c
>
> + MemoryAttribute.c
>
>
>
> [Sources.ARM]
>
> Arm/Mmu.c
>
> @@ -53,6 +54,7 @@ [LibraryClasses]
>
>
> [Protocols]
>
> gEfiCpuArchProtocolGuid
>
> + gEfiMemoryAttributeProtocolGuid
>
>
>
> [Guids]
>
> gEfiDebugImageInfoTableGuid
>
> diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> new file mode 100644
> index 000000000000..9aeb83fff1b9
> --- /dev/null
> +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> @@ -0,0 +1,242 @@
> +/** @file
>
> +
>
> + Copyright (c) 2023, Google LLC. All rights reserved.
>
> +
>
> + SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +
>
> +**/
>
> +
>
> +#include "CpuDxe.h"
>
> +
>
> +/**
>
> + This function retrieves the attributes of the memory region specified by
>
> + BaseAddress and Length. If different attributes are got from different part
>
> + of the memory region, EFI_NO_MAPPING will be returned.
>
> +
>
> + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
>
> + @param BaseAddress The physical address that is the start address of
>
> + a memory region.
>
> + @param Length The size in bytes of the memory region.
>
> + @param Attributes Pointer to attributes returned.
>
> +
>
> + @retval EFI_SUCCESS The attributes got for the memory region.
>
> + @retval EFI_INVALID_PARAMETER Length is zero.
>
> + Attributes is NULL.
>
> + @retval EFI_NO_MAPPING Attributes are not consistent cross the memory
>
> + region.
>
> + @retval EFI_UNSUPPORTED The processor does not support one or more
>
> + bytes of the memory resource range specified
>
> + by BaseAddress and Length.
>
> +
>
> +**/
>
> +STATIC
>
> +EFI_STATUS
>
> +GetMemoryAttributes (
>
> + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
>
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>
> + IN UINT64 Length,
>
> + OUT UINT64 *Attributes
>
> + )
>
> +{
>
> + UINTN RegionAddress;
>
> + UINTN RegionLength;
>
> + UINTN RegionAttributes;
>
> + UINTN Union;
>
> + UINTN Intersection;
>
> + EFI_STATUS Status;
>
> +
>
> + if ((Length == 0) || (Attributes == NULL)) {
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + DEBUG ((DEBUG_INFO,
>
> + "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",
>
> + __FUNCTION__,
>
> + (UINTN)BaseAddress,
>
> + (UINTN)Length
>
> + ));
>
> +
>
> + Union = 0;
>
> + Intersection = MAX_UINTN;
>
> +
>
> + for (RegionAddress = (UINTN)BaseAddress;
>
> + RegionAddress < (UINTN)(BaseAddress + Length);
>
> + RegionAddress += RegionLength) {
>
> +
>
> + Status = GetMemoryRegion (&RegionAddress,
>
> + &RegionLength,
>
> + &RegionAttributes
>
> + );
>
> +
>
> + DEBUG ((DEBUG_INFO,
>
> + "%a: RegionAddress == 0x%lx, RegionLength == 0x%lx, RegionAttributes == 0x%lx\n",
>
> + __FUNCTION__,
>
> + RegionAddress,
>
> + RegionLength,
>
> + RegionAttributes
>
> + ));
>
> +
>
> + if (EFI_ERROR (Status)) {
>
> + return EFI_NO_MAPPING;
>
> + }
>
> +
>
> + Union |= RegionAttributes;
>
> + Intersection &= RegionAttributes;
>
> + }
>
> +
>
> + DEBUG ((DEBUG_INFO,
>
> + "%a: Union == %lx, Intersection == %lx\n",
>
> + __FUNCTION__,
>
> + Union,
>
> + Intersection
>
> + ));
>
> +
>
> + if (Union != Intersection) {
>
> + return EFI_NO_MAPPING;
>
> + }
>
> +
>
> + *Attributes = PageAttributeToGcdAttribute (Union) & (EFI_MEMORY_RO | EFI_MEMORY_XP);
>
> + return EFI_SUCCESS;
>
> +}
>
> +
>
> +/**
>
> + This function set given attributes of the memory region specified by
>
> + BaseAddress and Length.
>
> +
>
> + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
>
> +
>
> + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
>
> + @param BaseAddress The physical address that is the start address of
>
> + a memory region.
>
> + @param Length The size in bytes of the memory region.
>
> + @param Attributes The bit mask of attributes to set for the memory
>
> + region.
>
> +
>
> + @retval EFI_SUCCESS The attributes were set for the memory region.
>
> + @retval EFI_INVALID_PARAMETER Length is zero.
>
> + Attributes specified an illegal combination of
>
> + attributes that cannot be set together.
>
> + @retval EFI_UNSUPPORTED The processor does not support one or more
>
> + bytes of the memory resource range specified
>
> + by BaseAddress and Length.
>
> + The bit mask of attributes is not supported for
>
> + the memory resource range specified by
>
> + BaseAddress and Length.
>
> +
>
> +**/
>
> +STATIC
>
> +EFI_STATUS
>
> +SetMemoryAttributes (
>
> + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
>
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>
> + IN UINT64 Length,
>
> + IN UINT64 Attributes
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> +
>
> + DEBUG ((DEBUG_INFO,
>
> + "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
>
> + __FUNCTION__,
>
> + (UINTN)BaseAddress,
>
> + (UINTN)Length,
>
> + (UINTN)Attributes
>
> + ));
>
> +
>
> + if ((Length == 0) ||
>
> + ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + if ((Attributes & EFI_MEMORY_RP) != 0) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> +
>
> + if ((Attributes & EFI_MEMORY_RO) != 0) {
>
> + Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length);
>
> + if (EFI_ERROR (Status)) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> + }
>
> +
>
> + if ((Attributes & EFI_MEMORY_XP) != 0) {
>
> + Status = ArmSetMemoryRegionNoExec (BaseAddress, Length);
>
> + if (EFI_ERROR (Status)) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> + }
>
> +
>
> + return EFI_SUCCESS;
>
> +}
>
> +
>
> +/**
>
> + This function clears given attributes of the memory region specified by
>
> + BaseAddress and Length.
>
> +
>
> + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
>
> +
>
> + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
>
> + @param BaseAddress The physical address that is the start address of
>
> + a memory region.
>
> + @param Length The size in bytes of the memory region.
>
> + @param Attributes The bit mask of attributes to clear for the memory
>
> + region.
>
> +
>
> + @retval EFI_SUCCESS The attributes were cleared for the memory region.
>
> + @retval EFI_INVALID_PARAMETER Length is zero.
>
> + Attributes specified an illegal combination of
>
> + attributes that cannot be cleared together.
>
> + @retval EFI_UNSUPPORTED The processor does not support one or more
>
> + bytes of the memory resource range specified
>
> + by BaseAddress and Length.
>
> + The bit mask of attributes is not supported for
>
> + the memory resource range specified by
>
> + BaseAddress and Length.
>
> +
>
> +**/
>
> +STATIC
>
> +EFI_STATUS
>
> +ClearMemoryAttributes (
>
> + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
>
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>
> + IN UINT64 Length,
>
> + IN UINT64 Attributes
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> +
>
> + DEBUG ((DEBUG_INFO,
>
> + "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
>
> + __FUNCTION__,
>
> + (UINTN)BaseAddress,
>
> + (UINTN)Length,
>
> + (UINTN)Attributes
>
> + ));
>
> +
>
> + if ((Length == 0) ||
>
> + ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + if ((Attributes & EFI_MEMORY_RO) != 0) {
>
> + Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length);
>
> + if (EFI_ERROR (Status)) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> + }
>
> +
>
> + if ((Attributes & EFI_MEMORY_XP) != 0) {
>
> + Status = ArmClearMemoryRegionNoExec (BaseAddress, Length);
>
> + if (EFI_ERROR (Status)) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> + }
>
> +
>
> + return EFI_SUCCESS;
>
> +}
>
> +
>
> +EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute = {
>
> + GetMemoryAttributes,
>
> + SetMemoryAttributes,
>
> + ClearMemoryAttributes
>
> +};
>
--
Taylor Beebe
Software Engineer @ Microsoft
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-01 18:41 ` [edk2-devel] " Taylor Beebe
@ 2023-02-02 9:43 ` Ard Biesheuvel
2023-02-03 19:08 ` Taylor Beebe
0 siblings, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2023-02-02 9:43 UTC (permalink / raw)
To: devel, t
Cc: Michael Kinney, Liming Gao, Jiewen Yao, Michael Kubacki,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
On Wed, 1 Feb 2023 at 19:41, Taylor Beebe <t@taylorbeebe.com> wrote:
>
> Hey Ard,
>
> Have you encountered complications which stem from the lack of
> pre-allocated page table memory on ARM devices utilizing the memory
> protection policy?
>
Interesting. No I haven't, but I agree it is a potential concern.
> My observation is the call stack can end up something like:
>
> 1. MemoryAttributeProtocol->SetMemoryAttributes(EFI_MEMORY_RO)
> 2. ArmSetMemoryRegionReadOnly ()
> 3. SetMemoryRegionAttribute()
> 4. UpdateRegionMapping()
> 5. UpdateRegionMappingRecursive()
> 6. AllocatePages() -> Need memory for a translation table entry
> 7. CoreAllocatePages()
> 8. ApplyMemoryProtectionPolicy() -> Policy says new page should be XN
> 9. gCpu->SetMemoryAttributes()
> 10. Back to 3 and loop infinitely
>
So one thing that makes this scenario unlikely (I think) is that by
default, all unallocated space has the XP attribute set already, and
so allocating a page table with XP attributes should not result in the
need for a block entry split, and therefore no allocation for a next
level table. Perhaps we need some asserts to diagnose this at runtime?
> On 1/31/2023 2:35 PM, Ard Biesheuvel wrote:
> > Expose the protocol introduced in v2.10 that permits the caller to
> > manage mapping permissions in the page tables.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 +
> > ArmPkg/Drivers/CpuDxe/CpuDxe.h | 3 +
> > ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +
> > ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 242 ++++++++++++++++++++
> > 4 files changed, 249 insertions(+)
> >
> > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > index e6742f0a25fc..d04958e79e52 100644
> > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > @@ -244,6 +244,8 @@ CpuDxeInitialize (
> > &mCpuHandle,
> >
> > &gEfiCpuArchProtocolGuid,
> >
> > &mCpu,
> >
> > + &gEfiMemoryAttributeProtocolGuid,
> >
> > + &mMemoryAttribute,
> >
> > NULL
> >
> > );
> >
> >
> >
> > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > index 8933fa90c4ed..f45d2bc101a3 100644
> > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > @@ -30,9 +30,12 @@
> > #include <Protocol/Cpu.h>
> >
> > #include <Protocol/DebugSupport.h>
> >
> > #include <Protocol/LoadedImage.h>
> >
> > +#include <Protocol/MemoryAttribute.h>
> >
> >
> >
> > extern BOOLEAN mIsFlushingGCD;
> >
> >
> >
> > +extern EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute;
> >
> > +
> >
> > /**
> >
> > This function registers and enables the handler specified by InterruptHandler for a processor
> >
> > interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
> >
> > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > index 10792b393fc8..e732e21cb94a 100644
> > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > @@ -23,6 +23,7 @@ [Sources.Common]
> > CpuDxe.h
> >
> > CpuMmuCommon.c
> >
> > Exception.c
> >
> > + MemoryAttribute.c
> >
> >
> >
> > [Sources.ARM]
> >
> > Arm/Mmu.c
> >
> > @@ -53,6 +54,7 @@ [LibraryClasses]
> >
> >
> > [Protocols]
> >
> > gEfiCpuArchProtocolGuid
> >
> > + gEfiMemoryAttributeProtocolGuid
> >
> >
> >
> > [Guids]
> >
> > gEfiDebugImageInfoTableGuid
> >
> > diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> > new file mode 100644
> > index 000000000000..9aeb83fff1b9
> > --- /dev/null
> > +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> > @@ -0,0 +1,242 @@
> > +/** @file
> >
> > +
> >
> > + Copyright (c) 2023, Google LLC. All rights reserved.
> >
> > +
> >
> > + SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > +
> >
> > +**/
> >
> > +
> >
> > +#include "CpuDxe.h"
> >
> > +
> >
> > +/**
> >
> > + This function retrieves the attributes of the memory region specified by
> >
> > + BaseAddress and Length. If different attributes are got from different part
> >
> > + of the memory region, EFI_NO_MAPPING will be returned.
> >
> > +
> >
> > + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> >
> > + @param BaseAddress The physical address that is the start address of
> >
> > + a memory region.
> >
> > + @param Length The size in bytes of the memory region.
> >
> > + @param Attributes Pointer to attributes returned.
> >
> > +
> >
> > + @retval EFI_SUCCESS The attributes got for the memory region.
> >
> > + @retval EFI_INVALID_PARAMETER Length is zero.
> >
> > + Attributes is NULL.
> >
> > + @retval EFI_NO_MAPPING Attributes are not consistent cross the memory
> >
> > + region.
> >
> > + @retval EFI_UNSUPPORTED The processor does not support one or more
> >
> > + bytes of the memory resource range specified
> >
> > + by BaseAddress and Length.
> >
> > +
> >
> > +**/
> >
> > +STATIC
> >
> > +EFI_STATUS
> >
> > +GetMemoryAttributes (
> >
> > + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
> >
> > + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> >
> > + IN UINT64 Length,
> >
> > + OUT UINT64 *Attributes
> >
> > + )
> >
> > +{
> >
> > + UINTN RegionAddress;
> >
> > + UINTN RegionLength;
> >
> > + UINTN RegionAttributes;
> >
> > + UINTN Union;
> >
> > + UINTN Intersection;
> >
> > + EFI_STATUS Status;
> >
> > +
> >
> > + if ((Length == 0) || (Attributes == NULL)) {
> >
> > + return EFI_INVALID_PARAMETER;
> >
> > + }
> >
> > +
> >
> > + DEBUG ((DEBUG_INFO,
> >
> > + "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",
> >
> > + __FUNCTION__,
> >
> > + (UINTN)BaseAddress,
> >
> > + (UINTN)Length
> >
> > + ));
> >
> > +
> >
> > + Union = 0;
> >
> > + Intersection = MAX_UINTN;
> >
> > +
> >
> > + for (RegionAddress = (UINTN)BaseAddress;
> >
> > + RegionAddress < (UINTN)(BaseAddress + Length);
> >
> > + RegionAddress += RegionLength) {
> >
> > +
> >
> > + Status = GetMemoryRegion (&RegionAddress,
> >
> > + &RegionLength,
> >
> > + &RegionAttributes
> >
> > + );
> >
> > +
> >
> > + DEBUG ((DEBUG_INFO,
> >
> > + "%a: RegionAddress == 0x%lx, RegionLength == 0x%lx, RegionAttributes == 0x%lx\n",
> >
> > + __FUNCTION__,
> >
> > + RegionAddress,
> >
> > + RegionLength,
> >
> > + RegionAttributes
> >
> > + ));
> >
> > +
> >
> > + if (EFI_ERROR (Status)) {
> >
> > + return EFI_NO_MAPPING;
> >
> > + }
> >
> > +
> >
> > + Union |= RegionAttributes;
> >
> > + Intersection &= RegionAttributes;
> >
> > + }
> >
> > +
> >
> > + DEBUG ((DEBUG_INFO,
> >
> > + "%a: Union == %lx, Intersection == %lx\n",
> >
> > + __FUNCTION__,
> >
> > + Union,
> >
> > + Intersection
> >
> > + ));
> >
> > +
> >
> > + if (Union != Intersection) {
> >
> > + return EFI_NO_MAPPING;
> >
> > + }
> >
> > +
> >
> > + *Attributes = PageAttributeToGcdAttribute (Union) & (EFI_MEMORY_RO | EFI_MEMORY_XP);
> >
> > + return EFI_SUCCESS;
> >
> > +}
> >
> > +
> >
> > +/**
> >
> > + This function set given attributes of the memory region specified by
> >
> > + BaseAddress and Length.
> >
> > +
> >
> > + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
> >
> > +
> >
> > + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> >
> > + @param BaseAddress The physical address that is the start address of
> >
> > + a memory region.
> >
> > + @param Length The size in bytes of the memory region.
> >
> > + @param Attributes The bit mask of attributes to set for the memory
> >
> > + region.
> >
> > +
> >
> > + @retval EFI_SUCCESS The attributes were set for the memory region.
> >
> > + @retval EFI_INVALID_PARAMETER Length is zero.
> >
> > + Attributes specified an illegal combination of
> >
> > + attributes that cannot be set together.
> >
> > + @retval EFI_UNSUPPORTED The processor does not support one or more
> >
> > + bytes of the memory resource range specified
> >
> > + by BaseAddress and Length.
> >
> > + The bit mask of attributes is not supported for
> >
> > + the memory resource range specified by
> >
> > + BaseAddress and Length.
> >
> > +
> >
> > +**/
> >
> > +STATIC
> >
> > +EFI_STATUS
> >
> > +SetMemoryAttributes (
> >
> > + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
> >
> > + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> >
> > + IN UINT64 Length,
> >
> > + IN UINT64 Attributes
> >
> > + )
> >
> > +{
> >
> > + EFI_STATUS Status;
> >
> > +
> >
> > + DEBUG ((DEBUG_INFO,
> >
> > + "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
> >
> > + __FUNCTION__,
> >
> > + (UINTN)BaseAddress,
> >
> > + (UINTN)Length,
> >
> > + (UINTN)Attributes
> >
> > + ));
> >
> > +
> >
> > + if ((Length == 0) ||
> >
> > + ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {
> >
> > + return EFI_INVALID_PARAMETER;
> >
> > + }
> >
> > +
> >
> > + if ((Attributes & EFI_MEMORY_RP) != 0) {
> >
> > + return EFI_UNSUPPORTED;
> >
> > + }
> >
> > +
> >
> > + if ((Attributes & EFI_MEMORY_RO) != 0) {
> >
> > + Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length);
> >
> > + if (EFI_ERROR (Status)) {
> >
> > + return EFI_UNSUPPORTED;
> >
> > + }
> >
> > + }
> >
> > +
> >
> > + if ((Attributes & EFI_MEMORY_XP) != 0) {
> >
> > + Status = ArmSetMemoryRegionNoExec (BaseAddress, Length);
> >
> > + if (EFI_ERROR (Status)) {
> >
> > + return EFI_UNSUPPORTED;
> >
> > + }
> >
> > + }
> >
> > +
> >
> > + return EFI_SUCCESS;
> >
> > +}
> >
> > +
> >
> > +/**
> >
> > + This function clears given attributes of the memory region specified by
> >
> > + BaseAddress and Length.
> >
> > +
> >
> > + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
> >
> > +
> >
> > + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> >
> > + @param BaseAddress The physical address that is the start address of
> >
> > + a memory region.
> >
> > + @param Length The size in bytes of the memory region.
> >
> > + @param Attributes The bit mask of attributes to clear for the memory
> >
> > + region.
> >
> > +
> >
> > + @retval EFI_SUCCESS The attributes were cleared for the memory region.
> >
> > + @retval EFI_INVALID_PARAMETER Length is zero.
> >
> > + Attributes specified an illegal combination of
> >
> > + attributes that cannot be cleared together.
> >
> > + @retval EFI_UNSUPPORTED The processor does not support one or more
> >
> > + bytes of the memory resource range specified
> >
> > + by BaseAddress and Length.
> >
> > + The bit mask of attributes is not supported for
> >
> > + the memory resource range specified by
> >
> > + BaseAddress and Length.
> >
> > +
> >
> > +**/
> >
> > +STATIC
> >
> > +EFI_STATUS
> >
> > +ClearMemoryAttributes (
> >
> > + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
> >
> > + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> >
> > + IN UINT64 Length,
> >
> > + IN UINT64 Attributes
> >
> > + )
> >
> > +{
> >
> > + EFI_STATUS Status;
> >
> > +
> >
> > + DEBUG ((DEBUG_INFO,
> >
> > + "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
> >
> > + __FUNCTION__,
> >
> > + (UINTN)BaseAddress,
> >
> > + (UINTN)Length,
> >
> > + (UINTN)Attributes
> >
> > + ));
> >
> > +
> >
> > + if ((Length == 0) ||
> >
> > + ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {
> >
> > + return EFI_INVALID_PARAMETER;
> >
> > + }
> >
> > +
> >
> > + if ((Attributes & EFI_MEMORY_RO) != 0) {
> >
> > + Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length);
> >
> > + if (EFI_ERROR (Status)) {
> >
> > + return EFI_UNSUPPORTED;
> >
> > + }
> >
> > + }
> >
> > +
> >
> > + if ((Attributes & EFI_MEMORY_XP) != 0) {
> >
> > + Status = ArmClearMemoryRegionNoExec (BaseAddress, Length);
> >
> > + if (EFI_ERROR (Status)) {
> >
> > + return EFI_UNSUPPORTED;
> >
> > + }
> >
> > + }
> >
> > +
> >
> > + return EFI_SUCCESS;
> >
> > +}
> >
> > +
> >
> > +EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute = {
> >
> > + GetMemoryAttributes,
> >
> > + SetMemoryAttributes,
> >
> > + ClearMemoryAttributes
> >
> > +};
> >
>
> --
> Taylor Beebe
> Software Engineer @ Microsoft
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-02 9:43 ` Ard Biesheuvel
@ 2023-02-03 19:08 ` Taylor Beebe
2023-02-03 19:58 ` Marvin Häuser
0 siblings, 1 reply; 24+ messages in thread
From: Taylor Beebe @ 2023-02-03 19:08 UTC (permalink / raw)
To: Ard Biesheuvel, devel
Cc: Michael Kinney, Liming Gao, Jiewen Yao, Michael Kubacki,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
On 2/2/2023 1:43 AM, Ard Biesheuvel wrote:
> On Wed, 1 Feb 2023 at 19:41, Taylor Beebe <t@taylorbeebe.com> wrote:
>>
>> Hey Ard,
>>
>> Have you encountered complications which stem from the lack of
>> pre-allocated page table memory on ARM devices utilizing the memory
>> protection policy?
>>
>
> Interesting. No I haven't, but I agree it is a potential concern.
>
>> My observation is the call stack can end up something like:
>>
>> 1. MemoryAttributeProtocol->SetMemoryAttributes(EFI_MEMORY_RO)
>> 2. ArmSetMemoryRegionReadOnly ()
>> 3. SetMemoryRegionAttribute()
>> 4. UpdateRegionMapping()
>> 5. UpdateRegionMappingRecursive()
>> 6. AllocatePages() -> Need memory for a translation table entry
>> 7. CoreAllocatePages()
>> 8. ApplyMemoryProtectionPolicy() -> Policy says new page should be XN
>> 9. gCpu->SetMemoryAttributes()
>> 10. Back to 3 and loop infinitely
>>
>
> So one thing that makes this scenario unlikely (I think) is that by
> default, all unallocated space has the XP attribute set already, and
> so allocating a page table with XP attributes should not result in the
> need for a block entry split, and therefore no allocation for a next
> level table. Perhaps we need some asserts to diagnose this at runtime?
Ah yes, you're right. On Project Mu, we make an explicit check to the
attributes of the region in ApplyMemoryProtectionPolicy() to determine
if the attributes need to be updated. We do this to account for the
introduction of the memory attribute protocol and to mitigate a rare
issue where an allocation might not have consistent attributes when
allocating a type other than BootServicesData. This change to
ApplyMemoryProtectionPolicy() means an infinite loop is possible during
the memory protection initialization phase.
>
>> On 1/31/2023 2:35 PM, Ard Biesheuvel wrote:
>>> Expose the protocol introduced in v2.10 that permits the caller to
>>> manage mapping permissions in the page tables.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 +
>>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 3 +
>>> ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +
>>> ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 242 ++++++++++++++++++++
>>> 4 files changed, 249 insertions(+)
>>>
>>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>>> index e6742f0a25fc..d04958e79e52 100644
>>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>>> @@ -244,6 +244,8 @@ CpuDxeInitialize (
>>> &mCpuHandle,
>>>
>>> &gEfiCpuArchProtocolGuid,
>>>
>>> &mCpu,
>>>
>>> + &gEfiMemoryAttributeProtocolGuid,
>>>
>>> + &mMemoryAttribute,
>>>
>>> NULL
>>>
>>> );
>>>
>>>
>>>
>>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>>> index 8933fa90c4ed..f45d2bc101a3 100644
>>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>>> @@ -30,9 +30,12 @@
>>> #include <Protocol/Cpu.h>
>>>
>>> #include <Protocol/DebugSupport.h>
>>>
>>> #include <Protocol/LoadedImage.h>
>>>
>>> +#include <Protocol/MemoryAttribute.h>
>>>
>>>
>>>
>>> extern BOOLEAN mIsFlushingGCD;
>>>
>>>
>>>
>>> +extern EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute;
>>>
>>> +
>>>
>>> /**
>>>
>>> This function registers and enables the handler specified by InterruptHandler for a processor
>>>
>>> interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
>>>
>>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
>>> index 10792b393fc8..e732e21cb94a 100644
>>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
>>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
>>> @@ -23,6 +23,7 @@ [Sources.Common]
>>> CpuDxe.h
>>>
>>> CpuMmuCommon.c
>>>
>>> Exception.c
>>>
>>> + MemoryAttribute.c
>>>
>>>
>>>
>>> [Sources.ARM]
>>>
>>> Arm/Mmu.c
>>>
>>> @@ -53,6 +54,7 @@ [LibraryClasses]
>>>
>>>
>>> [Protocols]
>>>
>>> gEfiCpuArchProtocolGuid
>>>
>>> + gEfiMemoryAttributeProtocolGuid
>>>
>>>
>>>
>>> [Guids]
>>>
>>> gEfiDebugImageInfoTableGuid
>>>
>>> diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
>>> new file mode 100644
>>> index 000000000000..9aeb83fff1b9
>>> --- /dev/null
>>> +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
>>> @@ -0,0 +1,242 @@
>>> +/** @file
>>>
>>> +
>>>
>>> + Copyright (c) 2023, Google LLC. All rights reserved.
>>>
>>> +
>>>
>>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>>>
>>> +
>>>
>>> +**/
>>>
>>> +
>>>
>>> +#include "CpuDxe.h"
>>>
>>> +
>>>
>>> +/**
>>>
>>> + This function retrieves the attributes of the memory region specified by
>>>
>>> + BaseAddress and Length. If different attributes are got from different part
>>>
>>> + of the memory region, EFI_NO_MAPPING will be returned.
>>>
>>> +
>>>
>>> + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
>>>
>>> + @param BaseAddress The physical address that is the start address of
>>>
>>> + a memory region.
>>>
>>> + @param Length The size in bytes of the memory region.
>>>
>>> + @param Attributes Pointer to attributes returned.
>>>
>>> +
>>>
>>> + @retval EFI_SUCCESS The attributes got for the memory region.
>>>
>>> + @retval EFI_INVALID_PARAMETER Length is zero.
>>>
>>> + Attributes is NULL.
>>>
>>> + @retval EFI_NO_MAPPING Attributes are not consistent cross the memory
>>>
>>> + region.
>>>
>>> + @retval EFI_UNSUPPORTED The processor does not support one or more
>>>
>>> + bytes of the memory resource range specified
>>>
>>> + by BaseAddress and Length.
>>>
>>> +
>>>
>>> +**/
>>>
>>> +STATIC
>>>
>>> +EFI_STATUS
>>>
>>> +GetMemoryAttributes (
>>>
>>> + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
>>>
>>> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>>>
>>> + IN UINT64 Length,
>>>
>>> + OUT UINT64 *Attributes
>>>
>>> + )
>>>
>>> +{
>>>
>>> + UINTN RegionAddress;
>>>
>>> + UINTN RegionLength;
>>>
>>> + UINTN RegionAttributes;
>>>
>>> + UINTN Union;
>>>
>>> + UINTN Intersection;
>>>
>>> + EFI_STATUS Status;
>>>
>>> +
>>>
>>> + if ((Length == 0) || (Attributes == NULL)) {
>>>
>>> + return EFI_INVALID_PARAMETER;
>>>
>>> + }
>>>
>>> +
>>>
>>> + DEBUG ((DEBUG_INFO,
>>>
>>> + "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",
>>>
>>> + __FUNCTION__,
>>>
>>> + (UINTN)BaseAddress,
>>>
>>> + (UINTN)Length
>>>
>>> + ));
>>>
>>> +
>>>
>>> + Union = 0;
>>>
>>> + Intersection = MAX_UINTN;
>>>
>>> +
>>>
>>> + for (RegionAddress = (UINTN)BaseAddress;
>>>
>>> + RegionAddress < (UINTN)(BaseAddress + Length);
>>>
>>> + RegionAddress += RegionLength) {
>>>
>>> +
>>>
>>> + Status = GetMemoryRegion (&RegionAddress,
>>>
>>> + &RegionLength,
>>>
>>> + &RegionAttributes
>>>
>>> + );
>>>
>>> +
>>>
>>> + DEBUG ((DEBUG_INFO,
>>>
>>> + "%a: RegionAddress == 0x%lx, RegionLength == 0x%lx, RegionAttributes == 0x%lx\n",
>>>
>>> + __FUNCTION__,
>>>
>>> + RegionAddress,
>>>
>>> + RegionLength,
>>>
>>> + RegionAttributes
>>>
>>> + ));
>>>
>>> +
>>>
>>> + if (EFI_ERROR (Status)) {
>>>
>>> + return EFI_NO_MAPPING;
>>>
>>> + }
>>>
>>> +
>>>
>>> + Union |= RegionAttributes;
>>>
>>> + Intersection &= RegionAttributes;
>>>
>>> + }
>>>
>>> +
>>>
>>> + DEBUG ((DEBUG_INFO,
>>>
>>> + "%a: Union == %lx, Intersection == %lx\n",
>>>
>>> + __FUNCTION__,
>>>
>>> + Union,
>>>
>>> + Intersection
>>>
>>> + ));
>>>
>>> +
>>>
>>> + if (Union != Intersection) {
>>>
>>> + return EFI_NO_MAPPING;
>>>
>>> + }
>>>
>>> +
>>>
>>> + *Attributes = PageAttributeToGcdAttribute (Union) & (EFI_MEMORY_RO | EFI_MEMORY_XP);
>>>
>>> + return EFI_SUCCESS;
>>>
>>> +}
>>>
>>> +
>>>
>>> +/**
>>>
>>> + This function set given attributes of the memory region specified by
>>>
>>> + BaseAddress and Length.
>>>
>>> +
>>>
>>> + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
>>>
>>> +
>>>
>>> + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
>>>
>>> + @param BaseAddress The physical address that is the start address of
>>>
>>> + a memory region.
>>>
>>> + @param Length The size in bytes of the memory region.
>>>
>>> + @param Attributes The bit mask of attributes to set for the memory
>>>
>>> + region.
>>>
>>> +
>>>
>>> + @retval EFI_SUCCESS The attributes were set for the memory region.
>>>
>>> + @retval EFI_INVALID_PARAMETER Length is zero.
>>>
>>> + Attributes specified an illegal combination of
>>>
>>> + attributes that cannot be set together.
>>>
>>> + @retval EFI_UNSUPPORTED The processor does not support one or more
>>>
>>> + bytes of the memory resource range specified
>>>
>>> + by BaseAddress and Length.
>>>
>>> + The bit mask of attributes is not supported for
>>>
>>> + the memory resource range specified by
>>>
>>> + BaseAddress and Length.
>>>
>>> +
>>>
>>> +**/
>>>
>>> +STATIC
>>>
>>> +EFI_STATUS
>>>
>>> +SetMemoryAttributes (
>>>
>>> + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
>>>
>>> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>>>
>>> + IN UINT64 Length,
>>>
>>> + IN UINT64 Attributes
>>>
>>> + )
>>>
>>> +{
>>>
>>> + EFI_STATUS Status;
>>>
>>> +
>>>
>>> + DEBUG ((DEBUG_INFO,
>>>
>>> + "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
>>>
>>> + __FUNCTION__,
>>>
>>> + (UINTN)BaseAddress,
>>>
>>> + (UINTN)Length,
>>>
>>> + (UINTN)Attributes
>>>
>>> + ));
>>>
>>> +
>>>
>>> + if ((Length == 0) ||
>>>
>>> + ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {
>>>
>>> + return EFI_INVALID_PARAMETER;
>>>
>>> + }
>>>
>>> +
>>>
>>> + if ((Attributes & EFI_MEMORY_RP) != 0) {
>>>
>>> + return EFI_UNSUPPORTED;
>>>
>>> + }
>>>
>>> +
>>>
>>> + if ((Attributes & EFI_MEMORY_RO) != 0) {
>>>
>>> + Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length);
>>>
>>> + if (EFI_ERROR (Status)) {
>>>
>>> + return EFI_UNSUPPORTED;
>>>
>>> + }
>>>
>>> + }
>>>
>>> +
>>>
>>> + if ((Attributes & EFI_MEMORY_XP) != 0) {
>>>
>>> + Status = ArmSetMemoryRegionNoExec (BaseAddress, Length);
>>>
>>> + if (EFI_ERROR (Status)) {
>>>
>>> + return EFI_UNSUPPORTED;
>>>
>>> + }
>>>
>>> + }
>>>
>>> +
>>>
>>> + return EFI_SUCCESS;
>>>
>>> +}
>>>
>>> +
>>>
>>> +/**
>>>
>>> + This function clears given attributes of the memory region specified by
>>>
>>> + BaseAddress and Length.
>>>
>>> +
>>>
>>> + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
>>>
>>> +
>>>
>>> + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
>>>
>>> + @param BaseAddress The physical address that is the start address of
>>>
>>> + a memory region.
>>>
>>> + @param Length The size in bytes of the memory region.
>>>
>>> + @param Attributes The bit mask of attributes to clear for the memory
>>>
>>> + region.
>>>
>>> +
>>>
>>> + @retval EFI_SUCCESS The attributes were cleared for the memory region.
>>>
>>> + @retval EFI_INVALID_PARAMETER Length is zero.
>>>
>>> + Attributes specified an illegal combination of
>>>
>>> + attributes that cannot be cleared together.
>>>
>>> + @retval EFI_UNSUPPORTED The processor does not support one or more
>>>
>>> + bytes of the memory resource range specified
>>>
>>> + by BaseAddress and Length.
>>>
>>> + The bit mask of attributes is not supported for
>>>
>>> + the memory resource range specified by
>>>
>>> + BaseAddress and Length.
>>>
>>> +
>>>
>>> +**/
>>>
>>> +STATIC
>>>
>>> +EFI_STATUS
>>>
>>> +ClearMemoryAttributes (
>>>
>>> + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
>>>
>>> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>>>
>>> + IN UINT64 Length,
>>>
>>> + IN UINT64 Attributes
>>>
>>> + )
>>>
>>> +{
>>>
>>> + EFI_STATUS Status;
>>>
>>> +
>>>
>>> + DEBUG ((DEBUG_INFO,
>>>
>>> + "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
>>>
>>> + __FUNCTION__,
>>>
>>> + (UINTN)BaseAddress,
>>>
>>> + (UINTN)Length,
>>>
>>> + (UINTN)Attributes
>>>
>>> + ));
>>>
>>> +
>>>
>>> + if ((Length == 0) ||
>>>
>>> + ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {
>>>
>>> + return EFI_INVALID_PARAMETER;
>>>
>>> + }
>>>
>>> +
>>>
>>> + if ((Attributes & EFI_MEMORY_RO) != 0) {
>>>
>>> + Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length);
>>>
>>> + if (EFI_ERROR (Status)) {
>>>
>>> + return EFI_UNSUPPORTED;
>>>
>>> + }
>>>
>>> + }
>>>
>>> +
>>>
>>> + if ((Attributes & EFI_MEMORY_XP) != 0) {
>>>
>>> + Status = ArmClearMemoryRegionNoExec (BaseAddress, Length);
>>>
>>> + if (EFI_ERROR (Status)) {
>>>
>>> + return EFI_UNSUPPORTED;
>>>
>>> + }
>>>
>>> + }
>>>
>>> +
>>>
>>> + return EFI_SUCCESS;
>>>
>>> +}
>>>
>>> +
>>>
>>> +EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute = {
>>>
>>> + GetMemoryAttributes,
>>>
>>> + SetMemoryAttributes,
>>>
>>> + ClearMemoryAttributes
>>>
>>> +};
>>>
>>
>> --
>> Taylor Beebe
>> Software Engineer @ Microsoft
>>
>>
>>
>>
>>
--
Taylor Beebe
Software Engineer @ Microsoft
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-03 19:08 ` Taylor Beebe
@ 2023-02-03 19:58 ` Marvin Häuser
2023-02-07 1:18 ` Taylor Beebe
0 siblings, 1 reply; 24+ messages in thread
From: Marvin Häuser @ 2023-02-03 19:58 UTC (permalink / raw)
To: Taylor Beebe, devel
[-- Attachment #1: Type: text/plain, Size: 407 bytes --]
Hi Taylor,
Do you by any chance mean this bug? https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1544
I reported this a while ago at https://bugzilla.tianocore.org/show_bug.cgi?id=3316
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.
Best regards,
Marvin
[-- Attachment #2: Type: text/html, Size: 717 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-03 19:58 ` Marvin Häuser
@ 2023-02-07 1:18 ` Taylor Beebe
2023-02-07 8:29 ` Ard Biesheuvel
0 siblings, 1 reply; 24+ messages in thread
From: Taylor Beebe @ 2023-02-07 1:18 UTC (permalink / raw)
To: Marvin Häuser, devel
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 :)
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.
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.
-Taylor
On 2/3/2023 11:58 AM, Marvin Häuser wrote:
> Hi Taylor,
>
> Do you by any chance mean this bug?
> https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1544 <https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1544>
>
> I reported this a while ago at
> https://bugzilla.tianocore.org/show_bug.cgi?id=3316
> <https://bugzilla.tianocore.org/show_bug.cgi?id=3316>
>
> 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.
>
> Best regards,
> Marvin
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-07 1:18 ` Taylor Beebe
@ 2023-02-07 8:29 ` Ard Biesheuvel
2023-02-07 8:56 ` Marvin Häuser
0 siblings, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2023-02-07 8:29 UTC (permalink / raw)
To: devel, t; +Cc: Marvin Häuser
On Tue, 7 Feb 2023 at 02:18, Taylor Beebe <t@taylorbeebe.com> wrote:
>
> 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 :)
>
I cannot see that bugzilla entry either.
> 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.
>
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.
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 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.
>
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)
Thanks,
Ard.
> On 2/3/2023 11:58 AM, Marvin Häuser wrote:
> > Hi Taylor,
> >
> > Do you by any chance mean this bug?
> > https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1544 <https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1544>
> >
> > I reported this a while ago at
> > https://bugzilla.tianocore.org/show_bug.cgi?id=3316
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=3316>
> >
> > 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.
> >
> > Best regards,
> > Marvin
> >
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-07 8:29 ` Ard Biesheuvel
@ 2023-02-07 8:56 ` Marvin Häuser
2023-02-07 9:16 ` Ard Biesheuvel
0 siblings, 1 reply; 24+ messages in thread
From: Marvin Häuser @ 2023-02-07 8:56 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, t
Hi Taylor and Ard,
> On 7. Feb 2023, at 09:29, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 7 Feb 2023 at 02:18, Taylor Beebe <t@taylorbeebe.com> wrote:
>>
>> 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 :)
>>
>
> I cannot see that bugzilla entry either.
I CC’d you both.
>
>> 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.
>>
>
> 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.
>
> 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 understood Taylor correctly. The only scenario I first-hand experienced this bug with was unloading a PE image. I don’t have the time right now to check the guarding page code in detail, but from what I just saw, I can very well imagine it can trigger the BZ bug (and thus potentially the recursion issue?).
>
>
>> 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.
>>
>
> 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 downstream fork removed the requirement that BSData and ConvMem need to have the same permissions. In fact, ideally ConvMem is just unmapped. Can this be enabled for ARM without a solution like Taylor’s? You said you added the requirement as a mitigation.
Unrelated FYI, we also removed the XP checks for code downstream and all types but ConvMem (which is unmapped or read-only, I forgot) have default permissions of RW. The reason for that is that unlike an OS, we don’t 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 be executed. The execute flag is added after loading the code to ensure W^X.
Best regards,
Marvin
>
> Thanks,
> Ard.
>
>
>>> On 2/3/2023 11:58 AM, Marvin Häuser wrote:
>>> Hi Taylor,
>>>
>>> Do you by any chance mean this bug?
>>> https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1544 <https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1544>
>>>
>>> I reported this a while ago at
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3316
>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=3316>
>>>
>>> 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.
>>>
>>> Best regards,
>>> Marvin
>>>
>>
>>
>>
>>
>>
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-07 8:56 ` Marvin Häuser
@ 2023-02-07 9:16 ` Ard Biesheuvel
2023-02-07 10:00 ` Marvin Häuser
2023-02-07 10:01 ` Ard Biesheuvel
0 siblings, 2 replies; 24+ messages in thread
From: Ard Biesheuvel @ 2023-02-07 9:16 UTC (permalink / raw)
To: devel, mhaeuser; +Cc: t
On Tue, 7 Feb 2023 at 09:56, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hi Taylor and Ard,
>
> > On 7. Feb 2023, at 09:29, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Tue, 7 Feb 2023 at 02:18, Taylor Beebe <t@taylorbeebe.com> wrote:
> >>
> >> 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 :)
> >>
> >
> > I cannot see that bugzilla entry either.
>
> I CC’d you both.
>
Thanks.
I wrote that code but nobody ever involved me or mentioned that there
was anything wrong with it.
> >
> >> 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.
> >>
> >
> > 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.
> >
> > 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 understood Taylor correctly. The only scenario I first-hand experienced this bug with was unloading a PE image. I don’t have the time right now to check the guarding page code in detail, but from what I just saw, I can very well imagine it can trigger the BZ bug (and thus potentially the recursion issue?).
if we disregard explicit invocations of
EFI_DXE_SERVICES.SetMemorySpaceAttributes (), the typical region
transitions from EfiConventionalMemory to some other type and back,
and never directly from one type to another. So I would expect that
unloading a PE image would result in a FreePages () call with
EfiConventionalMemory as the new type.
However, it appears that UnprotectUefiImage does not restore the
region's permissions to whatever is configured as the default for the
associated memory type, and so we end up with EfiConventialMemory
regions that lack the XP attribute.
I'll send out a separate fix for that. We can resume the discussion on
your patch on the bugzilla at the same time.
>
> >
> >
> >> 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.
> >>
> >
> > 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 downstream fork removed the requirement that BSData and ConvMem need to have the same permissions. In fact, ideally ConvMem is just unmapped. Can this be enabled for ARM without a solution like Taylor’s? You said you added the requirement as a mitigation.
>
Mapping a single page in a large unmapped region may require the
allocation of page tables. If populating that page table means we have
to map it first, we have a circular dependency and the recursion, so
we cannot deal with that currently.
Note that the page table walker does not need this page to be mapped,
it is only the code running on the CPU that needs a mapping while it
creates the entries. So this is fixable in principle, by mapping the
page somewhere else in the address space temporarily, just so we can
populate its contents without the need for recursing into the page
table code to map the page table.
This would take a bit of work, though, and I'd like to avoid this if possible.
> Unrelated FYI, we also removed the XP checks for code downstream and all types but ConvMem (which is unmapped or read-only, I forgot) have default permissions of RW. The reason for that is that unlike an OS, we don’t 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 be executed. The execute flag is added after loading the code to ensure W^X.
>
I would like to do the same, but this is currently not feasible. I
recently removed the exec permissions from EfiLoaderData regions in
ArmVirtQemu, and even though GRUB was fixed 5+ years ago not to rely
on this, a couple of distro forks got broken, and so they had to
revert this change in their builds.
With the EFI memory attributes protocol, the OS can tolerate this, and
I am adding handling of this to Linux so it no longer relies on any
allocation being executable by default.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-07 9:16 ` Ard Biesheuvel
@ 2023-02-07 10:00 ` Marvin Häuser
2023-02-07 10:01 ` Ard Biesheuvel
1 sibling, 0 replies; 24+ messages in thread
From: Marvin Häuser @ 2023-02-07 10:00 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, t
[-- Attachment #1: Type: text/plain, Size: 7401 bytes --]
> On 7. Feb 2023, at 10:16, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 7 Feb 2023 at 09:56, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>
>> Hi Taylor and Ard,
>>
>>>> On 7. Feb 2023, at 09:29, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>
>>> On Tue, 7 Feb 2023 at 02:18, Taylor Beebe <t@taylorbeebe.com> wrote:
>>>>
>>>> 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 :)
>>>>
>>>
>>> I cannot see that bugzilla entry either.
>>
>> I CC’d you both.
>>
>
> Thanks.
>
> I wrote that code but nobody ever involved me or mentioned that there
> was anything wrong with it.
Sorry, I filed this privately because I wasn’t sure whether or how you could exploit this - after all, you can affect memory permissions by loading an image that after all is not even executed. I didn’t expect there to be any such issues with ARM at the time (and did expect that if there were, the security team would bring whoever can help in themselves), so I didn’t expect needing to consult the author (you as the patch author and you as an ARM maintainer are two separate roles to me). I thought it was just an oversight. Hence (at first), minimal ACL. Sorry, my mistake.
Past a point, I realised security is not a priority of TianoCore at a scale (no offence, just a mere observation form various factors, also should not reflect back on individual contributors) and CC’d people for whom this could be relevant to know, including downstream folks. I should have CC’d you then at the very latest. Though, I will certainly never in my life again file a private BZ with TianoCore - which will allow me to CC more people from the start! :)
The BZ should just be unembargoed straight away.
>
>>>
>>>> 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.
>>>>
>>>
>>> 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.
>>>
>>> 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 understood Taylor correctly. The only scenario I first-hand experienced this bug with was unloading a PE image. I don’t have the time right now to check the guarding page code in detail, but from what I just saw, I can very well imagine it can trigger the BZ bug (and thus potentially the recursion issue?).
>
> if we disregard explicit invocations of
> EFI_DXE_SERVICES.SetMemorySpaceAttributes (), the typical region
> transitions from EfiConventionalMemory to some other type and back,
> and never directly from one type to another. So I would expect that
> unloading a PE image would result in a FreePages () call with
> EfiConventionalMemory as the new type.
Yes.
>
> However, it appears that UnprotectUefiImage does not restore the
> region's permissions to whatever is configured as the default for the
> associated memory type, and so we end up with EfiConventialMemory
> regions that lack the XP attribute.
Yes, that’s the issue. I’m not sure though whether this should be a design requirement? I certainly don’t remember this being documented anywhere and ignoring this particular bug, at a high level, that feels like a pointless chore when the permissions will be replaced by the ConvMem ones again anyway. The stateful design also is prone to errors, as we can see. I’d prefer a stateless design, where the permissions are always forced to the same constant value, no matter the previous configuration.
Then again, I lack the entire context of how the ARM VM code works and this might be so much easier at a low level that the stateful design is indeed worth it. I’ll shut up and leave it up to you. :)
>
> I'll send out a separate fix for that. We can resume the discussion on
> your patch on the bugzilla at the same time.
Tyvm! Not sure I have anything else to contribute though.
>
>>
>>>
>>>
>>>> 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.
>>>>
>>>
>>> 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 downstream fork removed the requirement that BSData and ConvMem need to have the same permissions. In fact, ideally ConvMem is just unmapped. Can this be enabled for ARM without a solution like Taylor’s? You said you added the requirement as a mitigation.
>>
>
> Mapping a single page in a large unmapped region may require the
> allocation of page tables. If populating that page table means we have
> to map it first, we have a circular dependency and the recursion, so
> we cannot deal with that currently.
>
> Note that the page table walker does not need this page to be mapped,
> it is only the code running on the CPU that needs a mapping while it
> creates the entries. So this is fixable in principle, by mapping the
> page somewhere else in the address space temporarily, just so we can
> populate its contents without the need for recursing into the page
> table code to map the page table.
>
> This would take a bit of work, though, and I'd like to avoid this if possible.
Tyvm!
>
>> Unrelated FYI, we also removed the XP checks for code downstream and all types but ConvMem (which is unmapped or read-only, I forgot) have default permissions of RW. The reason for that is that unlike an OS, we don’t 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 be executed. The execute flag is added after loading the code to ensure W^X.
>>
>
> I would like to do the same, but this is currently not feasible. I
> recently removed the exec permissions from EfiLoaderData regions in
> ArmVirtQemu, and even though GRUB was fixed 5+ years ago not to rely
> on this, a couple of distro forks got broken, and so they had to
> revert this change in their builds.
:(
>
> With the EFI memory attributes protocol, the OS can tolerate this, and
> I am adding handling of this to Linux so it no longer relies on any
> allocation being executable by default.
Related: https://lwn.net/ml/linux-kernel/20220303142120.1975-1-baskov@ispras.ru/
Best regards,
Marvin
[-- Attachment #2: Type: text/html, Size: 13512 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-07 9:16 ` Ard Biesheuvel
2023-02-07 10:00 ` Marvin Häuser
@ 2023-02-07 10:01 ` Ard Biesheuvel
2023-02-07 10:13 ` Marvin Häuser
1 sibling, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2023-02-07 10:01 UTC (permalink / raw)
To: devel, mhaeuser; +Cc: t
On Tue, 7 Feb 2023 at 10:16, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 7 Feb 2023 at 09:56, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >
> > Hi Taylor and Ard,
> >
> > > On 7. Feb 2023, at 09:29, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Tue, 7 Feb 2023 at 02:18, Taylor Beebe <t@taylorbeebe.com> wrote:
> > >>
> > >> 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 :)
> > >>
> > >
> > > I cannot see that bugzilla entry either.
> >
> > I CC’d you both.
> >
>
> Thanks.
>
> I wrote that code but nobody ever involved me or mentioned that there
> was anything wrong with it.
>
> > >
> > >> 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.
> > >>
> > >
> > > 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.
> > >
> > > 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 understood Taylor correctly. The only scenario I first-hand experienced this bug with was unloading a PE image. I don’t have the time right now to check the guarding page code in detail, but from what I just saw, I can very well imagine it can trigger the BZ bug (and thus potentially the recursion issue?).
>
> if we disregard explicit invocations of
> EFI_DXE_SERVICES.SetMemorySpaceAttributes (), the typical region
> transitions from EfiConventionalMemory to some other type and back,
> and never directly from one type to another. So I would expect that
> unloading a PE image would result in a FreePages () call with
> EfiConventionalMemory as the new type.
>
> However, it appears that UnprotectUefiImage does not restore the
> region's permissions to whatever is configured as the default for the
> associated memory type, and so we end up with EfiConventialMemory
> regions that lack the XP attribute.
>
> I'll send out a separate fix for that. We can resume the discussion on
> your patch on the bugzilla at the same time.
>
Actually, it seems UnprotectUefiImage () is corrent under the
assumption that all code regions have EFI_MEMORY_XP cleared by
default.
However, if you redefine the policy to set EFI_MEMORY_XP on code
regions by default, and only permit execution after remapping the code
read-only explicitly, and only then clearing EFI_MEMORY_XP, that
routine should revert the region to EFI_MEMORY_XP. But given the
existing ASSERT()s on having EFI_MEMORY_XP cleared for all code
regions, the code as it is currently is not incorrect.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-07 10:01 ` Ard Biesheuvel
@ 2023-02-07 10:13 ` Marvin Häuser
2023-02-07 17:56 ` Ard Biesheuvel
0 siblings, 1 reply; 24+ messages in thread
From: Marvin Häuser @ 2023-02-07 10:13 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, t
[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]
> On 7. Feb 2023, at 11:01, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Actually, it seems UnprotectUefiImage () is corrent under the
> assumption that all code regions have EFI_MEMORY_XP cleared by
> default.
>
> However, if you redefine the policy to set EFI_MEMORY_XP on code
> regions by default, and only permit execution after remapping the code
> read-only explicitly, and only then clearing EFI_MEMORY_XP, that
> routine should revert the region to EFI_MEMORY_XP. But given the
> existing ASSERT()s on having EFI_MEMORY_XP cleared for all code
> regions, the code as it is currently is not incorrect.
Right. My main issue is, it’s nowhere documented that manually changed permissions must be restored to their default before freeing. Within DxeCore, this is easily done using the PCDs, but outside (say you allocate a trampoline buffer and then free it), you would need to manually query the permissions, store them, and restore later.
I did *not* look into the implementation code in detail, but does the new memory permission protocol impose the same constraint implementation-wise and if so, is this documented anywhere?
PS: Fetched the wrong link in my last mail: https://lkml.org/lkml/2022/12/15/352
Best regards,
Marvin
[-- Attachment #2: Type: text/html, Size: 1870 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-07 10:13 ` Marvin Häuser
@ 2023-02-07 17:56 ` Ard Biesheuvel
2023-02-07 18:19 ` Taylor Beebe
2023-02-07 18:19 ` Marvin Häuser
0 siblings, 2 replies; 24+ messages in thread
From: Ard Biesheuvel @ 2023-02-07 17:56 UTC (permalink / raw)
To: Marvin Häuser; +Cc: devel, t
On Tue, 7 Feb 2023 at 11:13, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> On 7. Feb 2023, at 11:01, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Actually, it seems UnprotectUefiImage () is corrent under the
> assumption that all code regions have EFI_MEMORY_XP cleared by
> default.
>
> However, if you redefine the policy to set EFI_MEMORY_XP on code
> regions by default, and only permit execution after remapping the code
> read-only explicitly, and only then clearing EFI_MEMORY_XP, that
> routine should revert the region to EFI_MEMORY_XP. But given the
> existing ASSERT()s on having EFI_MEMORY_XP cleared for all code
> regions, the code as it is currently is not incorrect.
>
>
> Right. My main issue is, it’s nowhere documented that manually changed permissions must be restored to their default before freeing. Within DxeCore, this is easily done using the PCDs, but outside (say you allocate a trampoline buffer and then free it), you would need to manually query the permissions, store them, and restore later.
>
Agreed. However, I'd still prefer to only call
SetMemorySpaceAttributes() if needed, and setting the same attributes
on the entire image allocation at least permits us to double check
whether the new attributes are already set on a region, and avoid the
call if that is the case.
Perhaps we should just set EFI_MEMORY_XP on all images regardless of
the policy - the memory should no longer be executable anyway, and
what we currently do is remap the entire region RWX after it has
executed, which is kind of nasty.
> I did *not* look into the implementation code in detail, but does the new memory permission protocol impose the same constraint implementation-wise and if so, is this documented anywhere?
>
Not sure I follow you: which constraint is that?
> PS: Fetched the wrong link in my last mail: https://lkml.org/lkml/2022/12/15/352
>
Yeah saw that. I'm hoping to get that in for v6.4 as we missed v6.3 by
now. (I did take his patch that adds the definition of the EFI memory
attribute protocol only, as I need it for EFI zboot)
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-07 17:56 ` Ard Biesheuvel
@ 2023-02-07 18:19 ` Taylor Beebe
2023-02-07 18:50 ` Marvin Häuser
2023-02-07 18:19 ` Marvin Häuser
1 sibling, 1 reply; 24+ messages in thread
From: Taylor Beebe @ 2023-02-07 18:19 UTC (permalink / raw)
To: devel, ardb, Marvin Häuser
On 2/7/2023 12:56 AM, Marvin Häuser wrote:
> Hi Taylor and Ard,
>
>> On 7. Feb 2023, at 09:29, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Tue, 7 Feb 2023 at 02:18, Taylor Beebe <t@taylorbeebe.com> wrote:
>>>
>>> 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 :)
>>>
>>
>> I cannot see that bugzilla entry either.
>
> I CC’d you both.
>
>>
>>> 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.
>>>
>>
>> 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.
>>
>> 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
understood Taylor correctly. The only scenario I first-hand experienced
this bug with was unloading a PE image. I don’t have the time right now
to check the guarding page code in detail, but from what I just saw, I
can very well imagine it can trigger the BZ bug (and thus potentially
the recursion issue?).
>
>
The Project Mu change Marvin linked does solve the Bugzilla he created
because it makes an explicit check to the attributes of the memory
region being updated via ApplyMemoryProtectionAttributes() instead of
relying on its type to determine if a call to SetAttributes() is
required. However, because an explicit check is being made to the region
attributes, the infinite loop is no longer negated by the requirement
that Conventional and BootServicesData have the same XP setting making
the loop likely to occur during InitializeDxeNxMemoryProtectionPolicy().
This does not occur on x86 platforms because a reserve of page table
memory is allocated when CpuDxe loads and even if more page table memory
must be allocated a global boolean is set in the driver to stop a
potential loop.
So, the Bugzilla is not directly related to the looping issue, but
solving it in the way Project Mu has reveals the issue.
On 2/7/2023 9:56 AM, Ard Biesheuvel wrote:
> On Tue, 7 Feb 2023 at 11:13, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>
>>
>> On 7. Feb 2023, at 11:01, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> Actually, it seems UnprotectUefiImage () is corrent under the
>> assumption that all code regions have EFI_MEMORY_XP cleared by
>> default.
>>
>> However, if you redefine the policy to set EFI_MEMORY_XP on code
>> regions by default, and only permit execution after remapping the code
>> read-only explicitly, and only then clearing EFI_MEMORY_XP, that
>> routine should revert the region to EFI_MEMORY_XP. But given the
>> existing ASSERT()s on having EFI_MEMORY_XP cleared for all code
>> regions, the code as it is currently is not incorrect.
>>
>>
>> Right. My main issue is, it’s nowhere documented that manually changed permissions must be restored to their default before freeing. Within DxeCore, this is easily done using the PCDs, but outside (say you allocate a trampoline buffer and then free it), you would need to manually query the permissions, store them, and restore later.
>>
>
> Agreed. However, I'd still prefer to only call
> SetMemorySpaceAttributes() if needed, and setting the same attributes
> on the entire image allocation at least permits us to double check
> whether the new attributes are already set on a region, and avoid the
> call if that is the case.
>
> Perhaps we should just set EFI_MEMORY_XP on all images regardless of
> the policy - the memory should no longer be executable anyway, and
> what we currently do is remap the entire region RWX after it has
> executed, which is kind of nasty.
>
>> I did *not* look into the implementation code in detail, but does the new memory permission protocol impose the same constraint implementation-wise and if so, is this documented anywhere?
>>
>
> Not sure I follow you: which constraint is that?
>
>> PS: Fetched the wrong link in my last mail: https://lkml.org/lkml/2022/12/15/352
>>
>
> Yeah saw that. I'm hoping to get that in for v6.4 as we missed v6.3 by
> now. (I did take his patch that adds the definition of the EFI memory
> attribute protocol only, as I need it for EFI zboot)
>
>
>
>
If I understand Marvin correctly, he means that there either needs to be
a requirement that if you change the attributes of an allocated buffer
you must change them back before freeing, or the memory management logic
should handle the possibility that the memory attributes may have
changed since allocation. In my opinion introducing the Memory Attribute
Protocol requires more attribute accounting when allocating and freeing.
I believe the two changes linked below ensure that attributes are
properly set.
1.
https://github.com/microsoft/mu_basecore/blob/788c17f750323efc206cdb042a2c14a587dcf27a/MdeModulePkg/Core/Dxe/Mem/Page.c#L1570
2.
https://github.com/microsoft/mu_basecore/blob/788c17f750323efc206cdb042a2c14a587dcf27a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1562
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-07 17:56 ` Ard Biesheuvel
2023-02-07 18:19 ` Taylor Beebe
@ 2023-02-07 18:19 ` Marvin Häuser
1 sibling, 0 replies; 24+ messages in thread
From: Marvin Häuser @ 2023-02-07 18:19 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, t
> On 7. Feb 2023, at 18:56, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 7 Feb 2023 at 11:13, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>
>>
>> On 7. Feb 2023, at 11:01, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> Actually, it seems UnprotectUefiImage () is corrent under the
>> assumption that all code regions have EFI_MEMORY_XP cleared by
>> default.
>>
>> However, if you redefine the policy to set EFI_MEMORY_XP on code
>> regions by default, and only permit execution after remapping the code
>> read-only explicitly, and only then clearing EFI_MEMORY_XP, that
>> routine should revert the region to EFI_MEMORY_XP. But given the
>> existing ASSERT()s on having EFI_MEMORY_XP cleared for all code
>> regions, the code as it is currently is not incorrect.
>>
>>
>> Right. My main issue is, it’s nowhere documented that manually changed permissions must be restored to their default before freeing. Within DxeCore, this is easily done using the PCDs, but outside (say you allocate a trampoline buffer and then free it), you would need to manually query the permissions, store them, and restore later.
>>
>
> Agreed. However, I'd still prefer to only call
> SetMemorySpaceAttributes() if needed
Hmm, couldn’t there be some optimisation within the function itself? To my understanding, the memory / GCD maps should have the permission information without having to look them up with a page table walk, no? Again, just talking high-level here, ignoring any low-level details.
> , and setting the same attributes
> on the entire image allocation at least permits us to double check
> whether the new attributes are already set on a region, and avoid the
> call if that is the case.
>
> Perhaps we should just set EFI_MEMORY_XP on all images regardless of
> the policy - the memory should no longer be executable anyway, and
> what we currently do is remap the entire region RWX after it has
> executed, which is kind of nasty.
I’d rather have FreePages() take care of that honestly. Or do you mean as a workaround for tightened policies like Mu or AUDK?
>
>> I did *not* look into the implementation code in detail, but does the new memory permission protocol impose the same constraint implementation-wise and if so, is this documented anywhere?
>>
>
> Not sure I follow you: which constraint is that?
Having to reset the permissions to the type’s defaults prior to freeing.
>
>> PS: Fetched the wrong link in my last mail: https://lkml.org/lkml/2022/12/15/352
>>
>
> Yeah saw that. I'm hoping to get that in for v6.4 as we missed v6.3 by
> now. (I did take his patch that adds the definition of the EFI memory
> attribute protocol only, as I need it for EFI zboot)
:)
Best regards,
Marvin
^ permalink raw reply [flat|nested] 24+ messages in thread