Base.h: Introduce various alignment-related macros

Marvin Häuser Tue, 21 Mar 2023 15:44:00 -0700

On 21. Mar 2023, at 23:29, Kinney, Michael D <michael.d.kin...@intel.com> wrote:

I am suggesting adding “minimum” because aligning to a boundary could be to the next one or any multiples past that.

For example, offset is 1, and alignment request is 4. The obvious result is 3, but 7, 11, 15, 19, … also satisfy the current description.

They do not, the current description explicitly says “*next* alignment boundary”.

Best regards,

Marvin

Mike

From: Marvin Häuser <mhaeu...@posteo.de>
Sent: Tuesday, March 21, 2023 3:00 PM
To: Kinney, Michael D <michael.d.kin...@intel.com>
Cc: Gerd Hoffmann <kra...@redhat.com>; devel@edk2.groups.io; Ard Biesheuvel <ardb+tianoc...@kernel.org>; Wang, Jian J <jian.j.w...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; James Bottomley <j...@linux.ibm.com>; Michael Roth <michael.r...@amd.com>; Wu, Hao A <hao.a...@intel.com>; Oliver Steffen <ostef...@redhat.com>; Xu, Min M <min.m...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Ni, Ray <ray...@intel.com>; Tom Lendacky <thomas.lenda...@amd.com>; Aktas, Erdem <erdemak...@google.com>; Liu, Zhiguang <zhiguang....@intel.com>; Pawel Polawski <ppola...@redhat.com>; Justen, Jordan L <jordan.l.jus...@intel.com>; Vitaly Cheptsov <vit9...@protonmail.com>
Subject: Re: [PATCH 3/5] MdePkg/Base.h: Introduce various alignment-related macros

Hi Mike,

On 21. Mar 2023, at 22:37, Kinney, Michael D <michael.d.kin...@intel.com> wrote:

Hi Gerd,

A few comments included below.

Thanks,

Mike

-----Original Message-----
From: Gerd Hoffmann <kra...@redhat.com>
Sent: Thursday, March 2, 2023 10:51 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>; Gerd Hoffmann <kra...@redhat.com>; Wang, Jian J <jian.j.w...@intel.com>; Yao,
Jiewen <jiewen....@intel.com>; Marvin Häuser <mhaeu...@posteo.de>; James Bottomley <j...@linux.ibm.com>; Michael Roth
<michael.r...@amd.com>; Wu, Hao A <hao.a...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; Oliver Steffen
<ostef...@redhat.com>; Xu, Min M <min.m...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Ni, Ray <ray...@intel.com>; Tom
Lendacky <thomas.lenda...@amd.com>; Aktas, Erdem <erdemak...@google.com>; Liu, Zhiguang <zhiguang....@intel.com>; Pawel Polawski
<ppola...@redhat.com>; Justen, Jordan L <jordan.l.jus...@intel.com>; Vitaly Cheptsov <vit9...@protonmail.com>
Subject: [PATCH 3/5] MdePkg/Base.h: Introduce various alignment-related macros

From: Marvin Häuser <mhaeu...@posteo.de>

ALIGNOF: Determining the alignment requirement of data types is
crucial to ensure safe memory accesses when parsing untrusted data.

IS_POW2: Determining whether a value is a power of two is important
to verify whether untrusted values are valid alignment values.

IS_ALIGNED: In combination with ALIGNOF data offsets can be verified.
A more general version of the IS_ALIGNED macro previously defined by several modules.

ADDRESS_IS_ALIGNED: Variant of IS_ALIGNED for pointers and addresses.
Replaces module-specific definitions throughout the codebase.

ALIGN_VALUE_ADDEND: The addend to align up can be used to directly
determine the required offset for data alignment.

Cc: Michael D Kinney <michael.d.kin...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang....@intel.com>
Cc: Vitaly Cheptsov <vit9...@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeu...@posteo.de>
---
MdePkg/Include/Base.h | 95 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index d209e6de280a..2053314b50d1 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -758,6 +758,40 @@ typedef UINTN *BASE_LIST;
#define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
#endif

+/**
+ Returns the alignment requirement of a type.
+
+ @param   TYPE The name of the type to retrieve the alignment requirement of.
+
+ @return Alignment requirement, in Bytes, of TYPE.
+**/
+#if defined (__cplusplus)
+//
+// Standard C++ operator.
+//
+#define ALIGNOF(TYPE) alignof (TYPE)
+#elif defined (__GNUC__) || defined (__clang__) || (defined (_MSC_VER) && _MSC_VER >= 1900)
+//
+// All supported versions of GCC and Clang, as well as MSVC 2015 and later,
+// support the standard operator _Alignof.
+//
+#define ALIGNOF(TYPE) _Alignof (TYPE)
+#elif defined (_MSC_EXTENSIONS)
+//
+// Earlier versions of MSVC, at least MSVC 2008 and later, support the vendor
+// extension __alignof.
+//
+#define ALIGNOF(TYPE) __alignof (TYPE)
+#else
+//
+// For compilers that do not support inbuilt alignof operators, use OFFSET_OF.
+// CHAR8 is known to have both a size and an alignment requirement of 1 Byte.
+// As such, A must be located exactly at the offset equal to its alignment
+// requirement.
+//
+#define ALIGNOF(TYPE) OFFSET_OF (struct { CHAR8 C; TYPE A; }, A)
+#endif
+
/**
  Portable definition for compile time assertions.
  Equivalent to C11 static_assert macro from assert.h.
@@ -793,6 +827,21 @@ STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specif
STATIC_ASSERT (sizeof (L'A')    == 2, "sizeof (L'A') does not meet UEFI Specification Data Type requirements");
STATIC_ASSERT (sizeof (L"A")    == 4, "sizeof (L\"A\") does not meet UEFI Specification Data Type requirements");

+STATIC_ASSERT (ALIGNOF (BOOLEAN) == sizeof (BOOLEAN), "Alignment of BOOLEAN does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INT8)    == sizeof (INT8), "Alignment of INT8 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT8)   == sizeof (UINT8), "Alignment of INT16 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INT16)   == sizeof (INT16), "Alignment of INT16 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (UINT16) == sizeof (UINT16), "Alignment of UINT16 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INT32)   == sizeof (INT32), "Alignment of INT32 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (UINT32) == sizeof (UINT32), "Alignment of UINT32 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INT64)   == sizeof (INT64), "Alignment of INT64 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (UINT64) == sizeof (UINT64), "Alignment of UINT64 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (CHAR8)   == sizeof (CHAR8), "Alignment of CHAR8 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (CHAR16) == sizeof (CHAR16), "Alignment of CHAR16 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INTN)    == sizeof (INTN), "Alignment of INTN does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINTN)   == sizeof (UINTN), "Alignment of UINTN does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (VOID *) == sizeof (VOID *), "Alignment of VOID * does not meet UEFI Specification Data Type
requirements");
+
//
// The following three enum types are used to verify that the compiler
// configuration for enum types is compliant with Section 2.3.1 of the
@@ -816,6 +865,10 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not me
STATIC_ASSERT (sizeof (__VERIFY_UINT16_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements");
STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements");

+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT8_ENUM_SIZE) == sizeof (__VERIFY_UINT8_ENUM_SIZE), "Alignment of enum does not meet UEFI
Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT16_ENUM_SIZE) == sizeof (__VERIFY_UINT16_ENUM_SIZE), "Alignment of enum does not meet UEFI
Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT32_ENUM_SIZE) == sizeof (__VERIFY_UINT32_ENUM_SIZE), "Alignment of enum does not meet UEFI
Specification Data Type requirements");

This will need to be merged with latest edk2 because of change from UINT32 to INT32 for the 32-bit size checks

+
/**
  Macro that returns a pointer to the data structure that contains a specified field of
  that data structure. This is a lightweight method to hide information by placing a
@@ -837,6 +890,46 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not m
**/
#define BASE_CR(Record, TYPE, Field) ((TYPE *) ((CHAR8 *) (Record) - OFFSET_OF (TYPE, Field)))

+/**
+ Checks whether a value is a power of two.
+
+ @param   Value The value to check.
+
+ @return Whether Value is a power of two.

Change to @retval TRUE and @retval FALSE descriptions

+**/
+#define IS_POW2(Value) ((Value) != 0U && ((Value) & ((Value) - 1U)) == 0U)
+
+/**
+ Checks whether a value is aligned by a specified alignment.
+
+ @param   Value      The value to check.
+ @param   Alignment The alignment boundary used to check against.
+
+ @return Whether Value is aligned by Alignment.

Change to @retval TRUE and @retval FALSE descriptions

+**/
+#define IS_ALIGNED(Value, Alignment) (((Value) & ((Alignment) - 1U)) == 0U)
+
+/**
+ Checks whether a pointer or address is aligned by a specified alignment.
+
+ @param   Address    The pointer or address to check.
+ @param   Alignment The alignment boundary used to check against.
+
+ @return Whether Address is aligned by Alignment.

Change to @retval TRUE and @retval FALSE descriptions

I wouldn't object, but this adds verbosity with no additional information.

+**/
+#define ADDRESS_IS_ALIGNED(Address, Alignment) IS_ALIGNED ((UINTN) (Address), Alignment)
+
+/**
+ Determines the addend to add to a value to round it up to the next boundary of
+ a specified alignment.

Determines the minimum number of bytes to add to a value to round it up to the next boundary of a specified alignment.

+
+ @param   Value      The value to round up.
+ @param   Alignment The alignment boundary used to return the addend.
+
+ @return Addend to round Value up to alignment boundary Alignment.

Minimum number of bytes to add to Value to reach the next alignment boundary specified by Alignment.

Hmm. I would not object against explicitly mentioning "bytes", but there is no reason why this would be limited to this unit, so I don't quite see the point. I would object against "minimum", as the value is unambiguous (i.e., the result of the function spec is well-defined) - there is no "non-minimum".

Best regards,

Marvin

+**/
+#define ALIGN_VALUE_ADDEND(Value, Alignment) (((Alignment) - (Value)) & ((Alignment) - 1U))
+
/**
  Rounds a value up to the next boundary using a specified alignment.

@@ -849,7 +942,7 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not m
  @return A value up to the next boundary.

**/
-#define ALIGN_VALUE(Value, Alignment) ((Value) + (((Alignment) - (Value)) & ((Alignment) - 1)))
+#define ALIGN_VALUE(Value, Alignment) ((Value) + ALIGN_VALUE_ADDEND (Value, Alignment))

/**
  Adjust a pointer by adding the minimum offset required for it to be aligned on
--
2.39.2

_._,_._,_

Groups.io Links:

You receive all messages sent to this group.

_._,_._,_

Re: [edk2-devel] [PATCH 3/5] MdePkg/Base.h: Introduce various alignment-related macros

Reply via email to