On 04/16/19 11:04, Jordan Justen wrote: > On 2019-04-15 09:15:31, Laszlo Ersek wrote: >> 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. > > I think you are still accessing it through a UINT32*, since you are > using a pointer to a union, and an field of type UINT32 within the > union.
Using a union makes the behavior well-defined. > 6.2.7 Compatible type and composite type > > 1 Two types have compatible type if their types are the same. > Additional rules for determining whether two types are compatible > are described in [...] > 6.5 Expressions > > 6 The /effective type/ of an object for an access to its stored value > is the declared type of the object, if any. [...] > > 7 An object shall have its stored value accessed only by an lvalue > expression that has one of the following types: > > — a type compatible with the effective type of the object, > — a qualified version of a type compatible with the effective type > of the object, > — a type that is the signed or unsigned type corresponding to the > effective type of the object, > — a type that is the signed or unsigned type corresponding to a > qualified version of the effective type of the object, > — an aggregate or union type that includes one of the aforementioned > types among its members (including, recursively, a member of a > subaggregate or contained union), or > — a character type. - Regarding 6.5p6, the original object we intend to access has (declared) type EFI_COMMON_SECTION_HEADER. Therefore the effective type is EFI_COMMON_SECTION_HEADER. - Based on 6.2.7p1, EFI_COMMON_SECTION_HEADER is compatible with EFI_COMMON_SECTION_HEADER. (Because they are the same.) - Based on 6.5p7 item #5, EFI_COMMON_SECTION_HEADER can be accessed as EFI_COMMON_SECTION_HEADER_UNION, because EFI_COMMON_SECTION_HEADER_UNION includes "a type compatible with the effective type of the object" (#1) among its members -- namely an EFI_COMMON_SECTION_HEADER, which is compatible with EFI_COMMON_SECTION_HEADER, because they are the same. > I guess it might more well defined to shift the bytes, like is > sometimes done with the FFS file sizes. I did that (i.e. byte-shifting) in the other patch: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE but for SECTION_SIZE, the union is well-defined too. Thanks, Laszlo > > -Jordan > >> 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 (#39171): https://edk2.groups.io/g/devel/message/39171 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] -=-=-=-=-=-=-=-=-=-=-=-