Laszlo: I think it is OK to add UNION type. UNION is not new data structure. It is convenience for developer to consume the data structure. This change is good to me. Reviewed-by: Liming Gao <liming....@intel.com>
Thanks Liming >-----Original Message----- >From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >Laszlo Ersek >Sent: Tuesday, April 16, 2019 12:16 AM >To: Justen, Jordan L <jordan.l.jus...@intel.com>; edk2-devel-groups-io ><devel@edk2.groups.io>; Kinney, Michael D <michael.d.kin...@intel.com> >Cc: Gao, Liming <liming....@intel.com> >Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix >undefined behavior in SECTION_SIZE > >On 04/14/19 09:19, Jordan Justen wrote: >> On 2019-04-12 16:31:20, Laszlo Ersek wrote: >>> RH covscan justifiedly reports that accessing >>> "EFI_COMMON_SECTION_HEADER.Size", which is of type UINT8[3], >through a >>> (UINT32*), is undefined behavior: >>> >>>> Error: OVERRUN (CWE-119): >>>> edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:178: overrun-local: >Overrunning >>>> array of 3 bytes at byte offset 3 by dereferencing pointer >>>> "(UINT32 *)((EFI_COMMON_SECTION_HEADER *)(UINTN)Section)- >>Size". >>>> # 176| Section = (EFI_COMMON_SECTION_HEADER*)(UINTN) >CurrentAddress; >>>> # 177| >>>> # 178|-> Size = SECTION_SIZE (Section); >>>> # 179| if (Size < sizeof (*Section)) { >>>> # 180| return EFI_VOLUME_CORRUPTED; >>> >>> Fix this by introducing EFI_COMMON_SECTION_HEADER_UNION, and >expressing >>> SECTION_SIZE() in terms of >"EFI_COMMON_SECTION_HEADER_UNION.Uint32". >>> >>> 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 >>> Issue: scan-1007.txt >>> 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 a9f3bcc4eb8e..4fce8298d1c0 100644 >>> --- a/MdePkg/Include/Pi/PiFirmwareFile.h >>> +++ b/MdePkg/Include/Pi/PiFirmwareFile.h >>> @@ -229,16 +229,24 @@ typedef struct { >>> /// >>> UINT8 Size[3]; >>> EFI_SECTION_TYPE Type; >>> /// >>> /// Declares the section type. >>> /// >>> } EFI_COMMON_SECTION_HEADER; >>> >>> +/// >>> +/// Union that permits accessing EFI_COMMON_SECTION_HEADER as a >UINT32 object. >>> +/// >>> +typedef union { >>> + EFI_COMMON_SECTION_HEADER Hdr; >>> + UINT32 Uint32; >>> +} EFI_COMMON_SECTION_HEADER_UNION; >>> + >>> typedef struct { >>> /// >>> /// A 24-bit unsigned integer that contains the total size of the >>> section in >bytes, >>> /// including the EFI_COMMON_SECTION_HEADER. >>> /// >>> UINT8 Size[3]; >>> >>> EFI_SECTION_TYPE Type; >>> @@ -476,17 +484,17 @@ typedef struct { >>> /// A UINT16 that represents a particular build. Subsequent builds have >monotonically >>> /// increasing build numbers relative to earlier builds. >>> /// >>> UINT16 BuildNumber; >>> CHAR16 VersionString[1]; >>> } EFI_VERSION_SECTION2; >>> >>> #define SECTION_SIZE(SectionHeaderPtr) \ >>> - ((UINT32) (*((UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) >SectionHeaderPtr)->Size) & 0x00ffffff)) >>> + (((EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) >(SectionHeaderPtr))->Uint32 & 0x00ffffff) >> >> Mike, all, >> >> Can we add a typedef for EFI_COMMON_SECTION_HEADER_UNION if it's >not >> in the PI spec? >> >> If it's not allowed, I think something like this might work too: >> >> #define SECTION_SIZE(SectionHeaderPtr) \ >> (*((UINT32*)(UINTN)(SectionHeaderPtr)) & 0x00ffffff) > >(Less importantly:) > >It might shut up the static analyzer, but regarding the C standard, it's >equally undefined behavior. > >Anyway I don't feel too strongly about this, given that we disable the >strict aliasing / effective type rules in "tools_def.template" >("-fno-strict-aliasing"). > >> Then again, I see SECTION_SIZE is not in the spec, so maybe it's ok to >> add the typedef. > >(More importantly:) > >Indeed the doubt you voice about ..._UNION crossed my mind, but then I >too searched the PI spec for SECTION_SIZE, with no hits. > >Beyond that, I searched both the PI and UEFI specs, for "_UNION" -- >again no hits, despite our definitions of: > >- EFI_IMAGE_OPTIONAL_HEADER_UNION >- EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION > >in > >- "MdePkg/Include/IndustryStandard/PeImage.h" >- "MdePkg/Include/Protocol/GraphicsOutput.h" > >respectively. > >Thanks, >Laszlo > >> >> -Jordan >> > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39157): https://edk2.groups.io/g/devel/message/39157 Mute This Topic: https://groups.io/mt/31070302/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
