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] > 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 (#38989): > https://edk2.groups.io/g/devel/message/38989 > Mute This Topic: https://groups.io/mt/31070304/1643496 > Group Owner: devel+ow...@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub > [michael.d.kin...@intel.com] > -=-=-=-=-=-= -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39260): https://edk2.groups.io/g/devel/message/39260 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] -=-=-=-=-=-=-=-=-=-=-=-