On 04/17/19 20:36, Kinney, Michael D wrote: > Andrew, > > My suggestion is to read the 3 bytes and shift and or to build 24-bit value. > That is what is in the patch at the bottom. It uses an extra layer of macros > that I am not in favor of. There is an additional email with a proposed > approach that makes the use of the array members more obvious.
I will rework both patches #2 and #4 to use the byte-wise accesses and the shifting, without additional helper macros. It's valid C and it seems to enjoy the widest acceptance. (I agree it's the easiest to reason about.) Thanks, Laszlo > > Mike > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andrew > Fish via Groups.Io > Sent: Wednesday, April 17, 2019 11:32 AM > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com> > Cc: ler...@redhat.com; Gao, Liming <liming....@intel.com> > Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined > behavior in FFS_FILE_SIZE > > 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.kin...@intel.com<mailto:michael.d.kin...@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] > 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....@intel.com<mailto:liming....@intel.com>>; Kinney, > Michael > D <michael.d.kin...@intel.com<mailto: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<mailto:liming....@intel.com>> > Cc: Michael D Kinney > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>> > Bugzilla: > https://bugzilla.tianocore.org/show_bug.cgi?id=1710 > Signed-off-by: Laszlo Ersek <ler...@redhat.com<mailto: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<mailto:devel+ow...@edk2.groups.io> > Unsubscribe: https://edk2.groups.io/g/devel/unsub > [michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>] > -=-=-=-=-=-= > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39290): https://edk2.groups.io/g/devel/message/39290 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] -=-=-=-=-=-=-=-=-=-=-=-
