From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: jordan.l.justen@intel.com) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by groups.io with SMTP; Tue, 16 Apr 2019 02:04:46 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Apr 2019 02:04:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,357,1549958400"; d="scan'208";a="134749788" Received: from mjguyett-mobl1.amr.corp.intel.com (HELO localhost) ([10.251.132.204]) by orsmga008.jf.intel.com with ESMTP; 16 Apr 2019 02:04:45 -0700 MIME-Version: 1.0 In-Reply-To: <3bbbb85e-5557-d99b-1c3b-50a844455d20@redhat.com> References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-3-lersek@redhat.com> <155522637661.21857.4743822681286277764@jljusten-skl> <3bbbb85e-5557-d99b-1c3b-50a844455d20@redhat.com> To: Laszlo Ersek , Michael D Kinney , edk2-devel-groups-io Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE Cc: Liming Gao From: "Jordan Justen" Message-ID: <155540548458.13612.11281694046292591090@jljusten-skl> User-Agent: alot/0.8 Date: Tue, 16 Apr 2019 02:04:44 -0700 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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: Overrunni= ng > >>> array of 3 bytes at byte offset 3 by dereferencing pointer > >>> "(UINT32 *)((EFI_COMMON_SECTION_HEADER *)(UINTN)Section)->Size". > >>> # 176| Section =3D (EFI_COMMON_SECTION_HEADER*)(UINTN) Current= Address; > >>> # 177| > >>> # 178|-> Size =3D 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=3D1710 > >> 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/Pi= FirmwareFile.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; > >> =20 > >> +/// > >> +/// Union that permits accessing EFI_COMMON_SECTION_HEADER as a UINT3= 2 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 s= ection in bytes, > >> /// including the EFI_COMMON_SECTION_HEADER. > >> /// > >> UINT8 Size[3]; > >> =20 > >> 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; > >> =20 > >> #define SECTION_SIZE(SectionHeaderPtr) \ > >> - ((UINT32) (*((UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) Se= ctionHeaderPtr)->Size) & 0x00ffffff)) > >> + (((EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) (SectionHeaderPtr))= ->Uint32 & 0x00ffffff) > >=20 > > Mike, all, > >=20 > > Can we add a typedef for EFI_COMMON_SECTION_HEADER_UNION if it's not > > in the PI spec? > >=20 > > If it's not allowed, I think something like this might work too: > >=20 > > #define SECTION_SIZE(SectionHeaderPtr) \ > > (*((UINT32*)(UINTN)(SectionHeaderPtr)) & 0x00ffffff) >=20 > (Less importantly:) >=20 > 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. I guess it might more well defined to shift the bytes, like is sometimes done with the FFS file sizes. -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"). >=20 > > Then again, I see SECTION_SIZE is not in the spec, so maybe it's ok to > > add the typedef. >=20 > (More importantly:) >=20 > Indeed the doubt you voice about ..._UNION crossed my mind, but then I > too searched the PI spec for SECTION_SIZE, with no hits. >=20 > Beyond that, I searched both the PI and UEFI specs, for "_UNION" -- > again no hits, despite our definitions of: >=20 > - EFI_IMAGE_OPTIONAL_HEADER_UNION > - EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION >=20 > in >=20 > - "MdePkg/Include/IndustryStandard/PeImage.h" > - "MdePkg/Include/Protocol/GraphicsOutput.h" >=20 > respectively. >=20 > Thanks, > Laszlo >=20 > >=20 > > -Jordan > >=20 >=20