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
> +**/
> +#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.
> +**/
> +#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 (#101506): https://edk2.groups.io/g/devel/message/101506
Mute This Topic: https://groups.io/mt/97357268/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
