On 4/18/19 7:59 PM, Kinney, Michael D wrote:
> Philippe,
>
> Comments below.
>
> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
>> Sent: Thursday, April 18, 2019 10:20 AM
>> To: devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kin...@intel.com>; ler...@redhat.com
>> Cc: Gao, Liming <liming....@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....@intel.com>; Kinney,
>> Michael
>>>> D <michael.d.kin...@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....@intel.com>
>>>> Cc: Michael D Kinney <michael.d.kin...@intel.com>
>>>> Bugzilla:
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=1710
>>>> Signed-off-by: Laszlo Ersek <ler...@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 (#39313): https://edk2.groups.io/g/devel/message/39313
Mute This Topic: https://groups.io/mt/31070304/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
