From: "Andrew Fish" <afish@apple.com>
To: edk2-devel-groups-io <devel@edk2.groups.io>,
Mike Kinney <michael.d.kinney@intel.com>
Cc: "Marvin Häuser" <mhaeuser@posteo.de>,
"Liming Gao" <gaoliming@byosoft.com.cn>,
"Liu, Zhiguang" <zhiguang.liu@intel.com>,
"Vitaly Cheptsov" <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [PATCH v2 1/2] MdePkg/BaseLib: Fix unaligned API prototypes
Date: Mon, 09 Aug 2021 14:32:51 -0700 [thread overview]
Message-ID: <C91EE76C-5168-45A9-AFC2-7667FE6E5812@apple.com> (raw)
In-Reply-To: <CO1PR11MB4929E8F3A9F2A806875D3F95D2F69@CO1PR11MB4929.namprd11.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 12177 bytes --]
> On Aug 9, 2021, at 9:15 AM, Michael D Kinney <michael.d.kinney@intel.com> wrote:
>
> Hi Marvin,
>
> Can you provide an example of which C compiler is flagging this as
> an error and what error message is generated.
>
> Please enter a BZ with this background information and add link to the
> BZ in the commit message.
>
> This is a change to the BaseLib class, so we need to make sure there
> are no impacts to any existing code. I looks like a safe change
> because changing from a pointer to a fixed size type to VOID *
> should be compatible. Please add that analysis to the background
> in the BZ as well.
>
MIke,
I want to say we had a discussion about this years ago? I don’t remember the outcome.
Dereferencing a misaligned pointer is UB (Undefined Behavior) in C [1], but historically x86 compilers have let it slide.
I think the situation we are in is the BaseLib functions don’t contain UB, but it is UB for the caller to use the returned pointer directly.
Here is a simple example with clang UndefinedBehaviorSanitizer (UBSan) .
~/work/Compiler>cat ub.c
#include <stdlib.h>
#define EFIAPI
#define IN
#define OUT
typedef unsigned char UINT8;
typedef unsigned short UINT16;
UINT16
EFIAPI
WriteUnaligned16 (
OUT UINT16 *Buffer,
IN UINT16 Value
)
{
// ASSERT (Buffer != NULL);
((volatile UINT8*)Buffer)[0] = (UINT8)Value;
((volatile UINT8*)Buffer)[1] = (UINT8)(Value >> 8);
return Value;
}
int main()
{
UINT8 *buffer = malloc(64);
UINT16 *pointer = (UINT16 *)(buffer + 1);
WriteUnaligned16 (pointer, 42);
// *pointer = 42; // Error: misaligned integer pointer assignment
return *pointer;
}
~/work/Compiler>clang -fsanitize=undefined ub.c
~/work/Compiler>./a.out
ub.c:34:9: runtime error: load of misaligned address 0x7feac6405aa1 for type 'UINT16' (aka 'unsigned short'), which requires 2 byte alignment
0x7feac6405aa1: note: pointer points here
00 00 00 64 2a 00 79 6d 28 52 54 4c 44 5f 44 45 46 41 55 4c 54 2c 20 73 77 69 66 74 5f 64 65 6d
^
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ub.c:34:9 in
FYI line 39 is `return *pointer`and 42 is 0x2A. So reading an writing to *pointer is UB.
As you can see in [1] the general advice is to take code that looks like:
int8_t *buffer = malloc(64);
int32_t *pointer = (int32_t *)(buffer + 1);
*pointer = 42; // Error: misaligned integer pointer assignment
And replace it with;
int8_t *buffer = malloc(64);
int32_t value = 42;
memcpy(buffer + 1, &value, sizeof(int32_t)); // Correct
But in these cases the result is in a byte aligned buffer….
[1] https://developer.apple.com/documentation/xcode/misaligned-pointer
Thanks,
Andrew Fish
> Thanks,
>
> Mike
>
>
>> -----Original Message-----
>> From: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>
>> Sent: Monday, August 9, 2021 2:51 AM
>> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Liming Gao <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>; Liu, Zhiguang
>> <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>; Vitaly Cheptsov <vit9696@protonmail.com <mailto:vit9696@protonmail.com>>
>> Subject: [PATCH v2 1/2] MdePkg/BaseLib: Fix unaligned API prototypes
>>
>> C prohibits not only dereferencing but also casting to unaligned
>> pointers. Thus, the current set of unaligned APIs cannot be called
>> safely. Update their prototypes to take VOID * pointers, which must
>> be able to represent any valid pointer.
>>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
>> ---
>> MdePkg/Library/BaseLib/Arm/Unaligned.c | 14 ++++-----
>> MdePkg/Library/BaseLib/Unaligned.c | 32 ++++++++++----------
>> MdePkg/Include/Library/BaseLib.h | 16 +++++-----
>> 3 files changed, 31 insertions(+), 31 deletions(-)
>>
>> diff --git a/MdePkg/Library/BaseLib/Arm/Unaligned.c b/MdePkg/Library/BaseLib/Arm/Unaligned.c
>> index e9934e7003cb..57f19fc44e0b 100644
>> --- a/MdePkg/Library/BaseLib/Arm/Unaligned.c
>> +++ b/MdePkg/Library/BaseLib/Arm/Unaligned.c
>> @@ -59,7 +59,7 @@ ReadUnaligned16 (
>> UINT16
>>
>> EFIAPI
>>
>> WriteUnaligned16 (
>>
>> - OUT UINT16 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT16 Value
>>
>> )
>>
>> {
>>
>> @@ -87,7 +87,7 @@ WriteUnaligned16 (
>> UINT32
>>
>> EFIAPI
>>
>> ReadUnaligned24 (
>>
>> - IN CONST UINT32 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> )
>>
>> {
>>
>> ASSERT (Buffer != NULL);
>>
>> @@ -116,7 +116,7 @@ ReadUnaligned24 (
>> UINT32
>>
>> EFIAPI
>>
>> WriteUnaligned24 (
>>
>> - OUT UINT32 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT32 Value
>>
>> )
>>
>> {
>>
>> @@ -143,7 +143,7 @@ WriteUnaligned24 (
>> UINT32
>>
>> EFIAPI
>>
>> ReadUnaligned32 (
>>
>> - IN CONST UINT32 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> )
>>
>> {
>>
>> UINT16 LowerBytes;
>>
>> @@ -175,7 +175,7 @@ ReadUnaligned32 (
>> UINT32
>>
>> EFIAPI
>>
>> WriteUnaligned32 (
>>
>> - OUT UINT32 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT32 Value
>>
>> )
>>
>> {
>>
>> @@ -202,7 +202,7 @@ WriteUnaligned32 (
>> UINT64
>>
>> EFIAPI
>>
>> ReadUnaligned64 (
>>
>> - IN CONST UINT64 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> )
>>
>> {
>>
>> UINT32 LowerBytes;
>>
>> @@ -234,7 +234,7 @@ ReadUnaligned64 (
>> UINT64
>>
>> EFIAPI
>>
>> WriteUnaligned64 (
>>
>> - OUT UINT64 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT64 Value
>>
>> )
>>
>> {
>>
>> diff --git a/MdePkg/Library/BaseLib/Unaligned.c b/MdePkg/Library/BaseLib/Unaligned.c
>> index a419cb85e53c..3041adcde606 100644
>> --- a/MdePkg/Library/BaseLib/Unaligned.c
>> +++ b/MdePkg/Library/BaseLib/Unaligned.c
>> @@ -26,12 +26,12 @@
>> UINT16
>>
>> EFIAPI
>>
>> ReadUnaligned16 (
>>
>> - IN CONST UINT16 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> )
>>
>> {
>>
>> ASSERT (Buffer != NULL);
>>
>>
>>
>> - return *Buffer;
>>
>> + return *(CONST UINT16 *) Buffer;
>>
>> }
>>
>>
>>
>> /**
>>
>> @@ -52,13 +52,13 @@ ReadUnaligned16 (
>> UINT16
>>
>> EFIAPI
>>
>> WriteUnaligned16 (
>>
>> - OUT UINT16 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT16 Value
>>
>> )
>>
>> {
>>
>> ASSERT (Buffer != NULL);
>>
>>
>>
>> - return *Buffer = Value;
>>
>> + return *(UINT16 *) Buffer = Value;
>>
>> }
>>
>>
>>
>> /**
>>
>> @@ -77,12 +77,12 @@ WriteUnaligned16 (
>> UINT32
>>
>> EFIAPI
>>
>> ReadUnaligned24 (
>>
>> - IN CONST UINT32 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> )
>>
>> {
>>
>> ASSERT (Buffer != NULL);
>>
>>
>>
>> - return *Buffer & 0xffffff;
>>
>> + return *(CONST UINT32 *) Buffer & 0xffffff;
>>
>> }
>>
>>
>>
>> /**
>>
>> @@ -103,13 +103,13 @@ ReadUnaligned24 (
>> UINT32
>>
>> EFIAPI
>>
>> WriteUnaligned24 (
>>
>> - OUT UINT32 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT32 Value
>>
>> )
>>
>> {
>>
>> ASSERT (Buffer != NULL);
>>
>>
>>
>> - *Buffer = BitFieldWrite32 (*Buffer, 0, 23, Value);
>>
>> + *(UINT32 *) Buffer = BitFieldWrite32 (*(CONST UINT32 *) Buffer, 0, 23, Value);
>>
>> return Value;
>>
>> }
>>
>>
>>
>> @@ -129,12 +129,12 @@ WriteUnaligned24 (
>> UINT32
>>
>> EFIAPI
>>
>> ReadUnaligned32 (
>>
>> - IN CONST UINT32 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> )
>>
>> {
>>
>> ASSERT (Buffer != NULL);
>>
>>
>>
>> - return *Buffer;
>>
>> + return *(CONST UINT32 *) Buffer;
>>
>> }
>>
>>
>>
>> /**
>>
>> @@ -155,13 +155,13 @@ ReadUnaligned32 (
>> UINT32
>>
>> EFIAPI
>>
>> WriteUnaligned32 (
>>
>> - OUT UINT32 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT32 Value
>>
>> )
>>
>> {
>>
>> ASSERT (Buffer != NULL);
>>
>>
>>
>> - return *Buffer = Value;
>>
>> + return *(UINT32 *) Buffer = Value;
>>
>> }
>>
>>
>>
>> /**
>>
>> @@ -180,12 +180,12 @@ WriteUnaligned32 (
>> UINT64
>>
>> EFIAPI
>>
>> ReadUnaligned64 (
>>
>> - IN CONST UINT64 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> )
>>
>> {
>>
>> ASSERT (Buffer != NULL);
>>
>>
>>
>> - return *Buffer;
>>
>> + return *(CONST UINT64 *) Buffer;
>>
>> }
>>
>>
>>
>> /**
>>
>> @@ -206,11 +206,11 @@ ReadUnaligned64 (
>> UINT64
>>
>> EFIAPI
>>
>> WriteUnaligned64 (
>>
>> - OUT UINT64 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT64 Value
>>
>> )
>>
>> {
>>
>> ASSERT (Buffer != NULL);
>>
>>
>>
>> - return *Buffer = Value;
>>
>> + return *(UINT64 *) Buffer = Value;
>>
>> }
>>
>> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
>> index 2452c1d92e51..4d30f0539c6b 100644
>> --- a/MdePkg/Include/Library/BaseLib.h
>> +++ b/MdePkg/Include/Library/BaseLib.h
>> @@ -3420,7 +3420,7 @@ DivS64x64Remainder (
>> UINT16
>>
>> EFIAPI
>>
>> ReadUnaligned16 (
>>
>> - IN CONST UINT16 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> );
>>
>>
>>
>>
>>
>> @@ -3442,7 +3442,7 @@ ReadUnaligned16 (
>> UINT16
>>
>> EFIAPI
>>
>> WriteUnaligned16 (
>>
>> - OUT UINT16 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT16 Value
>>
>> );
>>
>>
>>
>> @@ -3463,7 +3463,7 @@ WriteUnaligned16 (
>> UINT32
>>
>> EFIAPI
>>
>> ReadUnaligned24 (
>>
>> - IN CONST UINT32 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> );
>>
>>
>>
>>
>>
>> @@ -3485,7 +3485,7 @@ ReadUnaligned24 (
>> UINT32
>>
>> EFIAPI
>>
>> WriteUnaligned24 (
>>
>> - OUT UINT32 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT32 Value
>>
>> );
>>
>>
>>
>> @@ -3506,7 +3506,7 @@ WriteUnaligned24 (
>> UINT32
>>
>> EFIAPI
>>
>> ReadUnaligned32 (
>>
>> - IN CONST UINT32 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> );
>>
>>
>>
>>
>>
>> @@ -3528,7 +3528,7 @@ ReadUnaligned32 (
>> UINT32
>>
>> EFIAPI
>>
>> WriteUnaligned32 (
>>
>> - OUT UINT32 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT32 Value
>>
>> );
>>
>>
>>
>> @@ -3549,7 +3549,7 @@ WriteUnaligned32 (
>> UINT64
>>
>> EFIAPI
>>
>> ReadUnaligned64 (
>>
>> - IN CONST UINT64 *Buffer
>>
>> + IN CONST VOID *Buffer
>>
>> );
>>
>>
>>
>>
>>
>> @@ -3571,7 +3571,7 @@ ReadUnaligned64 (
>> UINT64
>>
>> EFIAPI
>>
>> WriteUnaligned64 (
>>
>> - OUT UINT64 *Buffer,
>>
>> + OUT VOID *Buffer,
>>
>> IN UINT64 Value
>>
>> );
>>
>>
>>
>> --
>> 2.31.1
>
>
>
>
[-- Attachment #2: Type: text/html, Size: 49911 bytes --]
next prev parent reply other threads:[~2021-08-09 21:33 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-09 9:51 [PATCH v2 0/7] Fix various issues regarding DebugImageInfoTable Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 1/2] BaseTools: Define the read-only data section name per toolchain Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 2/2] UefiCpuPkg/BaseUefiCpuLib: Use toolchain-specific rodata section name Marvin Häuser
2021-08-10 2:43 ` Ni, Ray
2021-08-10 4:40 ` [edk2-devel] " Andrew Fish
2021-08-10 8:43 ` Marvin Häuser
2021-08-10 4:19 ` [edk2-devel] [PATCH v2 1/2] BaseTools: Define the read-only data section name per toolchain Andrew Fish
2021-08-10 8:27 ` Marvin Häuser
2021-08-10 19:35 ` Andrew Fish
2021-08-10 21:30 ` Marvin Häuser
2021-08-10 21:58 ` Andrew Fish
2021-08-11 8:11 ` Marvin Häuser
2021-08-11 17:19 ` Andrew Fish
2021-08-12 7:26 ` Marvin Häuser
2021-08-12 20:25 ` Marvin Häuser
2021-08-12 22:53 ` Andrew Fish
[not found] ` <169AB0F8BD9C50BA.13770@groups.io>
2021-08-16 21:13 ` Andrew Fish
[not found] ` <169A090BBBBE12C1.15606@groups.io>
2021-08-10 19:49 ` Andrew Fish
2021-08-10 21:24 ` Marvin Häuser
2021-08-10 21:54 ` Andrew Fish
2021-08-09 9:51 ` [PATCH v2 1/7] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 1/2] MdePkg/BaseLib: Fix unaligned API prototypes Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 2/2] BaseTools/CommonLib: " Marvin Häuser
2021-08-09 16:15 ` [PATCH v2 1/2] MdePkg/BaseLib: " Michael D Kinney
2021-08-09 21:32 ` Andrew Fish [this message]
2021-08-10 8:53 ` [edk2-devel] " Marvin Häuser
2021-08-10 17:36 ` Andrew Fish
2021-08-10 21:14 ` Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 1/2] SecurityPkg/DxeImageVerificationLib: Fix certificate lookup algorithm Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 2/2] SecurityPkg/SecureBootConfigDxe: " Marvin Häuser
2021-08-12 1:12 ` [edk2-devel] " Min Xu
2021-08-12 1:11 ` [edk2-devel] [PATCH v2 1/2] SecurityPkg/DxeImageVerificationLib: " Min Xu
2021-08-09 9:51 ` [PATCH v2 2/7] MdeModulePkg/DxeCore: Fix DebugImageInfoTable size report Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 3/7] EmbeddedPkg/GdbStub: Check DebugImageInfoTable type safely Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 4/7] ArmPkg/DefaultExceptionHandlerLib: " Marvin Häuser
2021-08-09 11:55 ` Ard Biesheuvel
2021-08-09 12:40 ` [edk2-devel] " Marvin Häuser
2021-08-09 21:19 ` Marvin Häuser
2021-08-16 9:50 ` Ard Biesheuvel
2021-08-09 9:51 ` [PATCH v2 5/7] MdeModulePkg/CoreDxe: Mandatory LoadedImage for DebugImageInfoTable Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 6/7] EmbeddedPkg/GdbStub: " Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 7/7] ArmPkg/DefaultExceptionHandlerLib: " Marvin Häuser
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=C91EE76C-5168-45A9-AFC2-7667FE6E5812@apple.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox