Re: [edk2-devel] [PATCH v2 1/2] MdePkg/BaseLib: Fix unaligned API prototypes

Tue, 10 Aug 2021 01:53:33 -0700 

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.kin...@intel.com <mailto:michael.d.kin...@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 <mhaeu...@posteo.de <mailto:mhaeu...@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.kin...@intel.com 
<mailto:michael.d.kin...@intel.com>>; Liming Gao 
<gaolim...@byosoft.com.cn <mailto:gaolim...@byosoft.com.cn>>; Liu, 
Zhiguang
<zhiguang....@intel.com <mailto:zhiguang....@intel.com>>; Vitaly 
Cheptsov <vit9...@protonmail.com <mailto:vit9...@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.kin...@intel.com 
<mailto:michael.d.kin...@intel.com>>
Cc: Liming Gao <gaolim...@byosoft.com.cn 
<mailto:gaolim...@byosoft.com.cn>>
Cc: Zhiguang Liu <zhiguang....@intel.com 
<mailto:zhiguang....@intel.com>>
Cc: Vitaly Cheptsov <vit9...@protonmail.com 
<mailto:vit9...@protonmail.com>>
Signed-off-by: Marvin Häuser <mhaeu...@posteo.de 
<mailto:mhaeu...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
























					Reply via email to