From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 15 Apr 2019 09:15:35 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DEEF23086231; Mon, 15 Apr 2019 16:15:33 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-136.rdu2.redhat.com [10.10.121.136]) by smtp.corp.redhat.com (Postfix) with ESMTP id AE4E060123; Mon, 15 Apr 2019 16:15:32 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE To: Jordan Justen , edk2-devel-groups-io , Michael D Kinney Cc: Liming Gao References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-3-lersek@redhat.com> <155522637661.21857.4743822681286277764@jljusten-skl> From: "Laszlo Ersek" Message-ID: <3bbbb85e-5557-d99b-1c3b-50a844455d20@redhat.com> Date: Mon, 15 Apr 2019 18:15:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <155522637661.21857.4743822681286277764@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Mon, 15 Apr 2019 16:15:34 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Michael D Kinney >> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710 >> Issue: scan-1007.txt >> Signed-off-by: Laszlo Ersek >> --- >> 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 >