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. 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 (#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 (#39312): https://edk2.groups.io/g/devel/message/39312 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] -=-=-=-=-=-=-=-=-=-=-=-
