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; Wed, 17 Apr 2019 03:01:12 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 849FC86675; Wed, 17 Apr 2019 10:01:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-177.rdu2.redhat.com [10.10.120.177]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9A9635D707; Wed, 17 Apr 2019 10:01:09 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE To: Jordan Justen , Michael D Kinney , edk2-devel-groups-io Cc: Liming Gao 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> <155544052538.15733.153410443320244157@jljusten-skl> From: "Laszlo Ersek" Message-ID: <70b46ce4-8dce-1192-c691-50693f7a2b05@redhat.com> Date: Wed, 17 Apr 2019 12:01:08 +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: <155544052538.15733.153410443320244157@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 17 Apr 2019 10:01:11 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/16/19 20:48, Jordan Justen wrote: > 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: 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. >>> >>> 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: > > I think maybe this all applies to lvalues, not rvalues. Why "maybe"? The standard explicitly writes "lvalue expression" above. And (PointerToUnion->Member) is equivalent to ((*PointerToUnion).Member); and (*PointerToUnion) is an lvalue. > 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. Post-patch, we are *not* dereferencing a (UINT32*). We are dereferencing a (EFI_COMMON_SECTION_HEADER_UNION*), and then accessing the Uint32 member in the pointed-to EFI_COMMON_SECTION_HEADER_UNION. If we continued dereferencing a (UINT32*), then the effective type rules that I quoted would indeed not apply. But that's not what the patch does. Thanks Laszlo > > -Jordan > >>> >>> \u2014 a type compatible with the effective type of the object, >>> \u2014 a qualified version of a type compatible with the effective type >>> 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 aforementioned >>> types among its members (including, recursively, a member of a >>> subaggregate or contained union), or >>> \u2014 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 >>>>> >>>> >>