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.65, mailfrom: jordan.l.justen@intel.com) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by groups.io with SMTP; Tue, 16 Apr 2019 11:48:47 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Apr 2019 11:48:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,358,1549958400"; d="scan'208";a="131947556" Received: from mjguyett-mobl1.amr.corp.intel.com (HELO localhost) ([10.251.132.204]) by orsmga007.jf.intel.com with ESMTP; 16 Apr 2019 11:48:46 -0700 MIME-Version: 1.0 In-Reply-To: <413ac018-bcf2-f510-00d0-33315974a3c2@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> <155540548458.13612.11281694046292591090@jljusten-skl> <413ac018-bcf2-f510-00d0-33315974a3c2@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 From: "Jordan Justen" Cc: Liming Gao Message-ID: <155544052538.15733.153410443320244157@jljusten-skl> User-Agent: alot/0.8 Date: Tue, 16 Apr 2019 11:48:45 -0700 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2019-04-16 03:59:48, Laszlo Ersek wrote: > 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: Overrun= ning > >>>>> 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) Curre= ntAddress; > >>>>> # 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 express= ing > >>>> 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/= 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; > >>>> =20 > >>>> +/// > >>>> +/// Union that permits accessing EFI_COMMON_SECTION_HEADER as a UIN= T32 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]; > >>>> =20 > >>>> EFI_SECTION_TYPE Type; > >>>> @@ -476,17 +484,17 @@ typedef struct { > >>>> /// A UINT16 that represents a particular build. Subsequent build= s 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) = 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. > >=20 > > 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. >=20 > Using a union makes the behavior well-defined. >=20 > > 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 [...] >=20 > > 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: I think maybe this all applies to lvalues, not rvalues. The boon and bane of C is that any pointer can be easily cast to a pointer of any other type and then dereferenced. If the pointer is dereferenced to a type that is larger, then I understand that there are cases where deferencing the pointer could have unintended side effects, but that is not the case here. The dereference could also be undefined if big-endian was in the picture, but UEFI restricts that. Of course none of this stops a tool from trying to delve further into the pointer usage to look for possible issues. But, I don't see how any of this changes the fact that with or without the union, we are dereferencing a UINT32 pointer. -Jordan > > > > \u2014 a type compatible with the effective type of the object, > > \u2014 a qualified version of a type compatible with the effective ty= pe > > of the object, > > \u2014 a type that is the signed or unsigned type corresponding to the > > effective type of the object, > > \u2014 a type that is the signed or unsigned type corresponding to a > > qualified version of the effective type of the object, > > \u2014 an aggregate or union type that includes one of the aforementi= oned > > types among its members (including, recursively, a member of a > > subaggregate or contained union), or > > \u2014 a character type. >=20 > - 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. >=20 > - Based on 6.2.7p1, EFI_COMMON_SECTION_HEADER is compatible with > EFI_COMMON_SECTION_HEADER. (Because they are the same.) >=20 > - 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. >=20 > > I guess it might more well defined to shift the bytes, like is > > sometimes done with the FFS file sizes. >=20 > I did that (i.e. byte-shifting) in the other patch: >=20 > [edk2-devel] [PATCH 04/10] > MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE >=20 > but for SECTION_SIZE, the union is well-defined too. >=20 > Thanks, > Laszlo >=20 > >=20 > > -Jordan > >=20 > >> 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 > >>> > >> >=20