[edk2-devel] [PATCH v2 1/2] MdePkg/BaseLib: Fix unaligned API prototypes
Marvin Häuser
mhaeuser at posteo.de
Tue Aug 10 08:53:22 UTC 2021
On 09/08/2021 23:32, Andrew Fish via groups.io wrote:
>
>
>> On Aug 9, 2021, at 9:15 AM, Michael D Kinney
>> <michael.d.kinney at intel.com <mailto:michael.d.kinney at 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.
They do contain UB, inherently from the combination of their prototype
and their usage.
Please refer to the new BZ added for V2:
https://bugzilla.tianocore.org/show_bug.cgi?id=3542
I could only speculate why UBsan does not check cast safety...
To be fair, I don't know a real-world platform that has issues with this
UB, but the fix is simple enough in my opinion.
Actually, enabling "-Wcast-align" some day would be great either way. :)
Best regards,
Marvin
>
> 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
> <https://developer.apple.com/documentation/xcode/misaligned-pointer>
>
> Thanks,
>
> Andrew Fish
>
>> Thanks,
>>
>> Mike
>>
>>
>>> -----Original Message-----
>>> From: Marvin Häuser <mhaeuser at posteo.de <mailto:mhaeuser at posteo.de>>
>>> Sent: Monday, August 9, 2021 2:51 AM
>>> To:devel at edk2.groups.io <mailto:devel at edk2.groups.io>
>>> Cc: Kinney, Michael D <michael.d.kinney at intel.com
>>> <mailto:michael.d.kinney at intel.com>>; Liming Gao
>>> <gaoliming at byosoft.com.cn <mailto:gaoliming at byosoft.com.cn>>; Liu,
>>> Zhiguang
>>> <zhiguang.liu at intel.com <mailto:zhiguang.liu at intel.com>>; Vitaly
>>> Cheptsov <vit9696 at protonmail.com <mailto:vit9696 at 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 at intel.com
>>> <mailto:michael.d.kinney at intel.com>>
>>> Cc: Liming Gao <gaoliming at byosoft.com.cn
>>> <mailto:gaoliming at byosoft.com.cn>>
>>> Cc: Zhiguang Liu <zhiguang.liu at intel.com
>>> <mailto:zhiguang.liu at intel.com>>
>>> Cc: Vitaly Cheptsov <vit9696 at protonmail.com
>>> <mailto:vit9696 at protonmail.com>>
>>> Signed-off-by: Marvin Häuser <mhaeuser at posteo.de
>>> <mailto:mhaeuser at 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
>>
>>
>>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79025): https://edk2.groups.io/g/devel/message/79025
Mute This Topic: https://groups.io/mt/84764900/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list