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 <mailto:kra...@redhat.com>> >> Sent: Thursday, March 2, 2023 10:51 PM >> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io> >> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org >> <mailto:ardb+tianoc...@kernel.org>>; Gerd Hoffmann <kra...@redhat.com >> <mailto:kra...@redhat.com>>; Wang, Jian J <jian.j.w...@intel.com >> <mailto:jian.j.w...@intel.com>>; Yao, >> Jiewen <jiewen....@intel.com <mailto:jiewen....@intel.com>>; Marvin Häuser >> <mhaeu...@posteo.de <mailto:mhaeu...@posteo.de>>; James Bottomley >> <j...@linux.ibm.com <mailto:j...@linux.ibm.com>>; Michael Roth >> <michael.r...@amd.com <mailto:michael.r...@amd.com>>; Wu, Hao A >> <hao.a...@intel.com <mailto:hao.a...@intel.com>>; Kinney, Michael D >> <michael.d.kin...@intel.com <mailto:michael.d.kin...@intel.com>>; Oliver >> Steffen >> <ostef...@redhat.com <mailto:ostef...@redhat.com>>; Xu, Min M >> <min.m...@intel.com <mailto:min.m...@intel.com>>; Gao, Liming >> <gaolim...@byosoft.com.cn <mailto:gaolim...@byosoft.com.cn>>; Ni, Ray >> <ray...@intel.com <mailto:ray...@intel.com>>; Tom >> Lendacky <thomas.lenda...@amd.com <mailto:thomas.lenda...@amd.com>>; Aktas, >> Erdem <erdemak...@google.com <mailto:erdemak...@google.com>>; Liu, Zhiguang >> <zhiguang....@intel.com <mailto:zhiguang....@intel.com>>; Pawel Polawski >> <ppola...@redhat.com <mailto:ppola...@redhat.com>>; Justen, Jordan L >> <jordan.l.jus...@intel.com <mailto:jordan.l.jus...@intel.com>>; Vitaly >> Cheptsov <vit9...@protonmail.com <mailto: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. View/Reply Online (#101511): https://edk2.groups.io/g/devel/message/101511 Mute This Topic: https://groups.io/mt/97357268/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
