From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: jordan.l.justen@intel.com) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by groups.io with SMTP; Sun, 14 Apr 2019 00:19:38 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Apr 2019 00:19:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,348,1549958400"; d="scan'208";a="142580141" Received: from jgoodma2-mobl.amr.corp.intel.com (HELO localhost) ([10.251.156.152]) by fmsmga007.fm.intel.com with ESMTP; 14 Apr 2019 00:19:37 -0700 MIME-Version: 1.0 In-Reply-To: <20190412233128.4756-3-lersek@redhat.com> References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-3-lersek@redhat.com> To: Laszlo Ersek , edk2-devel-groups-io , Michael D Kinney From: "Jordan Justen" Cc: Liming Gao Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE Message-ID: <155522637661.21857.4743822681286277764@jljusten-skl> User-Agent: alot/0.8 Date: Sun, 14 Apr 2019 00:19:36 -0700 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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: >=20 > > 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 =3D (EFI_COMMON_SECTION_HEADER*)(UINTN) CurrentAd= dress; > > # 177| > > # 178|-> Size =3D SECTION_SIZE (Section); > > # 179| if (Size < sizeof (*Section)) { > > # 180| return EFI_VOLUME_CORRUPTED; >=20 > Fix this by introducing EFI_COMMON_SECTION_HEADER_UNION, and expressing > SECTION_SIZE() in terms of "EFI_COMMON_SECTION_HEADER_UNION.Uint32". >=20 > 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(-) >=20 > diff --git a/MdePkg/Include/Pi/PiFirmwareFile.h b/MdePkg/Include/Pi/PiFir= mwareFile.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 UINT32 o= bject. > +/// > +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 sect= ion 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 hav= e 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) Secti= onHeaderPtr)->Size) & 0x00ffffff)) > + (((EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) (SectionHeaderPtr))->U= int32 & 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) Then again, I see SECTION_SIZE is not in the spec, so maybe it's ok to add the typedef. -Jordan