Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 7112 bytes --]

Mike,

I kind of ratholed us on alignment when Laszlo was more concerned about strict aliasing and the effective type rule. Sorry! I don't think your proposal fixes the effective type issue, and actually may make it worse if I'm correct. 

It seems like the safest thing to do is read byte by byte and shift which I thought we did a long time ago?

The last point I poorly tried to make to Laszlo was I thought you could cast around the effective type issue in C99. If you think about it the union and the cast are both telling the compiler the intent is to break the effective type rule and that is the bigger point I was trying to make. Given my insomnia I used alignment examples that I understand better. If we try to convert Size to a UINT32 * I think that does trigger the effect type issue Laszlo referenced. So I was only debating the boundary of enforcement of the effect type rule using alignment as a confusing example. 

I was actually writing a mail to some people that sit on the C/C++ standards committee that are UB experts to get some clarification when you sent this mail. I'm concerned I'm conflating behavior with what the standard states, and may have some recency bias with solving alignment issues that makes me think the cast should work. 

I'm basically asking if this code pedantic conforms to C99 and C11:

EFI_COMMON_SECTION_HEADER gSec = { { 0x01, 0x02, 0x3 }, 0x10 };

return *(UINT32 *)gSec.Size & 0x00ffffff;

I ran the clang static analyzer and runtime ubsan on the above code and it did not complain (I force strict aliasing via -fstrict-aliasing, and I'm using the Sys V ABI since this is just the command line compiler on my Mac). 

Thanks,

Andrew Fish

> On Apr 17, 2019, at 10:52 AM, Michael D Kinney <michael.d.kinney@intel.com> wrote:
> 
> Laszlo,
> 
> I have been following this thread.  I think the style
> used here to access the 3 array elements to build the
> 24-bit size value is the best approach.  I prefer this
> over adding the union.
> 
> I agree there is a read overrun issue when using UINT32 to
> read the Size[3] array contents.
> 
> I do not think this is a real issue in practice, because the
> Size[3] array accessed is part of the larger
> EFI_COMMON_SECTION_HEADER structure.  However, we always should
> clean up code to not do any read/write overruns without this
> type of analysis and the need to keep track of exceptions.
> 
> There is a related set of code in the BaseLib for Read/Write
> Unaligned24().
> 
> UINT32
> EFIAPI
> ReadUnaligned24 (
>  IN CONST UINT32              *Buffer
>  );
> 
> UINT32
> EFIAPI
> WriteUnaligned24 (
>  OUT UINT32                    *Buffer,
>  IN  UINT32                    Value
>  );
> 
> This API does not get flagged for read overrun issues because
> a UINT32 is passed in.  However, for CPU archs that required aligned
> access, the 24-bit value must be read in pieces.  This is why there
> are 2 different implementations:
> 
> IA32/X64
> ========
> UINT32
> EFIAPI
> ReadUnaligned24 (
>  IN CONST UINT32              *Buffer
>  )
> {
>  ASSERT (Buffer != NULL);
> 
>  return *Buffer & 0xffffff;
> }
> 
> 
> ARM/AARCH64
> ============
> UINT32
> EFIAPI
> ReadUnaligned24 (
>  IN CONST UINT32              *Buffer
>  )
> {
>  ASSERT (Buffer != NULL);
> 
>  return (UINT32)(
>            ReadUnaligned16 ((UINT16*)Buffer) |
>            (((UINT8*)Buffer)[2] << 16)
>            );
> }
> 
> The ARM/ARCH64 implementation is clean because it does
> not do a read overrun of the 24-bit field.  The IA32/X64
> implementation may have an issue because it reads a 32-bit
> value and strips the upper 8 bits.
> 
> If we apply the same technique to the Size field of
> EFI_COMMON_SECTION_HEADER, then the 24-bit value would be
> built from reading only the 3 bytes of the array.
> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>]
>> On Behalf Of Laszlo Ersek
>> Sent: Friday, April 12, 2019 4:31 PM
>> To: edk2-devel-groups-io <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
>> Cc: Gao, Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>; Kinney, Michael
>> D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>> Subject: [edk2-devel] [PATCH 04/10]
>> MdePkg/PiFirmwareFile: fix undefined behavior in
>> FFS_FILE_SIZE
>> 
>> Accessing "EFI_FFS_FILE_HEADER.Size", which is of type
>> UINT8[3], through a
>> (UINT32*), is undefined behavior. Fix it by accessing
>> the array elements
>> individually.
>> 
>> (We can't use a union here, unfortunately, as easily as
>> with
>> "EFI_COMMON_SECTION_HEADER", given the fields in
>> "EFI_FFS_FILE_HEADER".)
>> 
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Bugzilla:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=1710
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> MdePkg/Include/Pi/PiFirmwareFile.h | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/MdePkg/Include/Pi/PiFirmwareFile.h
>> b/MdePkg/Include/Pi/PiFirmwareFile.h
>> index 4fce8298d1c0..0668f3fa9af4 100644
>> --- a/MdePkg/Include/Pi/PiFirmwareFile.h
>> +++ b/MdePkg/Include/Pi/PiFirmwareFile.h
>> @@ -174,18 +174,26 @@ typedef struct {
>>   /// If FFS_ATTRIB_LARGE_FILE is not set then
>> EFI_FFS_FILE_HEADER is used.
>>   ///
>>   UINT64                    ExtendedSize;
>> } EFI_FFS_FILE_HEADER2;
>> 
>> #define IS_FFS_FILE2(FfsFileHeaderPtr) \
>>     (((((EFI_FFS_FILE_HEADER *) (UINTN)
>> FfsFileHeaderPtr)->Attributes) & FFS_ATTRIB_LARGE_FILE)
>> == FFS_ATTRIB_LARGE_FILE)
>> 
>> +#define FFS_FILE_SIZE_ARRAY(FfsFileHeaderPtr) \
>> +    (((EFI_FFS_FILE_HEADER *) (UINTN)
>> (FfsFileHeaderPtr))->Size)
>> +
>> +#define FFS_FILE_SIZE_ELEMENT(FfsFileHeaderPtr, Index)
>> \
>> +    ((UINT32) FFS_FILE_SIZE_ARRAY
>> (FfsFileHeaderPtr)[(Index)])
>> +
>> #define FFS_FILE_SIZE(FfsFileHeaderPtr) \
>> -    ((UINT32) (*((UINT32 *) ((EFI_FFS_FILE_HEADER *)
>> (UINTN) FfsFileHeaderPtr)->Size) & 0x00ffffff))
>> +    ((FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 0) <<
>> 0) | \
>> +     (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 1) <<
>> 8) | \
>> +     (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 2) <<
>> 16))
>> 
>> #define FFS_FILE2_SIZE(FfsFileHeaderPtr) \
>>     ((UINT32) (((EFI_FFS_FILE_HEADER2 *) (UINTN)
>> FfsFileHeaderPtr)->ExtendedSize))
>> 
>> typedef UINT8 EFI_SECTION_TYPE;
>> 
>> ///
>> /// Pseudo type. It is used as a wild card when
>> retrieving sections.
>> --
>> 2.19.1.3.g30247aa5d201
>> 
>> 
>> 
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this
>> group.
>> 
>> View/Reply Online (#38989):
>> https://edk2.groups.io/g/devel/message/38989
>> Mute This Topic: https://groups.io/mt/31070304/1643496 <https://groups.io/mt/31070304/1643496>
>> Group Owner: devel+owner@edk2.groups.io <mailto:devel+owner@edk2.groups.io>
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub <https://edk2.groups.io/g/devel/unsub>
>> [michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>]
>> -=-=-=-=-=-=
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 63104 bytes --]