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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/18/19 7:59 PM, Kinney, Michael D wrote:
> Philippe,
> 
> Comments below.
> 
> Thanks,
> 
> Mike
> 
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>> Sent: Thursday, April 18, 2019 10:20 AM
>> To: devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>; lersek@redhat.com
>> Cc: Gao, Liming <liming.gao@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 04/10]
>> MdePkg/PiFirmwareFile: fix undefined behavior in
>> FFS_FILE_SIZE
>>
>> Hi Michael,
>>
>> On 4/17/19 7:52 PM, Michael D Kinney 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.
>>
>> This ARM implementation assumes Buffer is halfword-
>> aligned OR the
>> microarchitectures supports unaligned halfword access.
>>
>> The 3x 8-bit accesses macro looks simpler than adding a
>> 16-bit alignment
>> check on Buffer, such:
>>
>>    if (Buffer & 1) {
>>      return (UINT32)(
>>              ((UINT8*)Buffer)[0] |
>>              (ReadUnaligned16
>> ((UINT16*)&(((UINT8*)Buffer)[1])) << 8)
>>              );
>>    } else {
>>      return (UINT32)(
>>              ReadUnaligned16 ((UINT16*)Buffer) |
>>              (((UINT8*)Buffer)[2] << 16)
>>              );
>>    }
>>
> 
> The ARM/AARCH64 implementation of ReadUnaligned16() just
> does the byte access which will always work.  So not need
> to do the 2 modes you suggest above.

I should have check that first ;)

Thanks for correcting me!

> 
> UINT16
> EFIAPI
> ReadUnaligned16 (
>   IN CONST UINT16              *Buffer
>   )
> {
>   volatile UINT8 LowerByte;
>   volatile UINT8 HigherByte;
> 
>   ASSERT (Buffer != NULL);
> 
>   LowerByte = ((UINT8*)Buffer)[0];
>   HigherByte = ((UINT8*)Buffer)[1];
> 
>   return (UINT16)(LowerByte | (HigherByte << 8));
> }
> 
>>> Best regards,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: 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>
>>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney,
>> Michael
>>>> D <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