From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=I69HWWHS; spf=pass (domain: apple.com, ip: 17.151.62.68, mailfrom: afish@apple.com) Received: from nwk-aaemail-lapp03.apple.com (nwk-aaemail-lapp03.apple.com [17.151.62.68]) by groups.io with SMTP; Wed, 17 Apr 2019 04:45:09 -0700 Received: from pps.filterd (nwk-aaemail-lapp03.apple.com [127.0.0.1]) by nwk-aaemail-lapp03.apple.com (8.16.0.27/8.16.0.27) with SMTP id x3HBahpY004606; Wed, 17 Apr 2019 04:45:09 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=mime-version : content-type : sender : from : message-id : subject : date : in-reply-to : cc : to : references; s=20180706; bh=V4b/zSC0Fo5+EoF93IILkytXub7GoD9tVD4WMzAz+jI=; b=I69HWWHSvqv3GLRuvcada1xvmdBGZa43YWECaTIsXg+kOs5sEwVBbDeJ6lPWLNdv/zyR F0cH2I4VIA6mO84bCeRlXxGy5PKnae0NmpvKYWn01qp3O81wb/jHtXkCXDuKjbAHzq3Z gHiB5YtVJc1hq7iGVEHxiJ6drG1uM6tuXQuTpvmCsI+2vYPr7nDEdegVGuK5MAGzOLg+ d2T2UGzW2CMAZNCXPhObPQ0UlJr9Ravljkd8WLiceIL+YFs3dnRoPRydcjD/CoTya7ty yb/lz2e5ht3CUUO3zGxICK4YF+7nvx7UuCuH4FXFSTv4PlTJxOdvNV4VgbvIy2tVsiI/ xg== Received: from ma1-mtap-s03.corp.apple.com (ma1-mtap-s03.corp.apple.com [17.40.76.7]) by nwk-aaemail-lapp03.apple.com with ESMTP id 2ruca9xnqa-2 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 17 Apr 2019 04:45:09 -0700 MIME-version: 1.0 Received: from nwk-mmpp-sz09.apple.com (nwk-mmpp-sz09.apple.com [17.128.115.80]) by ma1-mtap-s03.corp.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) with ESMTPS id <0PQ3006T5SN6YF40@ma1-mtap-s03.corp.apple.com>; Wed, 17 Apr 2019 04:45:07 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz09.apple.com by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) id <0PQ300A00S1LGB00@nwk-mmpp-sz09.apple.com>; Wed, 17 Apr 2019 04:45:06 -0700 (PDT) X-Va-A: X-Va-T-CD: 13715775cfe6ed78bc954dbcb503dbb2 X-Va-E-CD: f08c4a32db9ade74c76bebf226a1f3ee X-Va-R-CD: e8c358aab5b67ff3bc786143aabeee62 X-Va-CD: 0 X-Va-ID: 2f5a6f91-56ed-459d-a373-70a37990637e X-V-A: X-V-T-CD: 13715775cfe6ed78bc954dbcb503dbb2 X-V-E-CD: f08c4a32db9ade74c76bebf226a1f3ee X-V-R-CD: e8c358aab5b67ff3bc786143aabeee62 X-V-CD: 0 X-V-ID: 8f761cbf-5d7d-44af-9ef4-3a5d1d13873e X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-04-17_05:,, signatures=0 Received: from [17.234.54.192] (unknown [17.234.54.192]) by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) with ESMTPSA id <0PQ3002CTSN3M540@nwk-mmpp-sz09.apple.com>; Wed, 17 Apr 2019 04:45:04 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE Date: Wed, 17 Apr 2019 04:44:58 -0700 In-reply-to: <71e4f508-75f2-79a7-967e-d7a6a0e34341@redhat.com> Cc: devel@edk2.groups.io, Jordan Justen , Mike Kinney , Liming Gao To: Laszlo Ersek 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> <940201E3-0EDB-40B8-8680-CDE68DA0FD06@apple.com> <71e4f508-75f2-79a7-967e-d7a6a0e34341@redhat.com> X-Mailer: Apple Mail (2.3445.6.18) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-04-17_05:,, signatures=0 Content-type: multipart/alternative; boundary="Boundary_(ID_tG1IeuV7tOGEdBrwJ4AoWA)" --Boundary_(ID_tG1IeuV7tOGEdBrwJ4AoWA) Content-type: text/plain; CHARSET=US-ASCII Content-transfer-encoding: 7BIT > On Apr 17, 2019, at 3:29 AM, Laszlo Ersek wrote: > > Hi Andrew, > > On 04/17/19 01:25, Andrew Fish wrote: >> >> >>> On Apr 16, 2019, at 11:48 AM, 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. 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. >>> >> >> The thing that is really saving us is the 4 byte alignment requirement for FV sections in the PI Spec, but that is not expressed by EFI_COMMON_SECTION_HEADER to the compiler. The union is better as it enforces alignment of the structure. This enforced alignment is likely what makes static analysis tools happy. >> >> I don't think the casting in the existing SECTION_SIZE is UB (Undefined Behavior) as the extraction is not the issue as you are telling the compiler to extract 4 unaligned bytes. That is the same as accessing a packed structure. But this is only because you started with a byte pointer so you are not really violating any C alignment rules. Thus in a case like our cast, or pragma pack(1), the compiler has extra alignment info provided so all is good, the issue is if you assign a pointer into the type the only alignment info the compiler has is the rules for that type, and non of packing rules. A lot of the above C rules are making it clear what the compiler does in these cases. >> >> Ironically the new union macro is undefined behavior at runtime during the cast if the section header is not 4 byte algined since you did not start with something (unsigned char pointer) that allowed byte access. See my example of the ubsan fault. >> >> The nice thing about the union is it tells the compiler our intended constraint from the PI spec. >> >> // This 5 byte structure will align EFI_COMMON_SECTION_HEADER on an odd byte boundary. >> typedef struct { >> UINT8 Count; >> EFI_COMMON_SECTION_HEADER Sec; >> } UNALIGNED_SECTION_HEADER; >> >> // This will properly align EFI_COMMON_SECTION_HEADER_UNION on a 4 byte boundary. >> typedef struct { >> UINT8 Count; >> EFI_COMMON_SECTION_HEADER_UNION Sec; >> } ALIGNED_SECTION_HEADER2; >> >> I tried to validate this with the clang ubsan... The only runtime UB was the new macro against UNALIGNED_SECTION_HEADER. ALIGNED_SECTION_HEADER2 with the new union aligned everything correctly and works fine. >> >> ./a.out >> Fv.c:55:11: runtime error: member access within misaligned address 0x000104b23021 for type 'EFI_COMMON_SECTION_HEADER_UNION', which requires 4 byte alignment >> 0x000104b23021: note: pointer points here >> 00 00 00 ff 01 02 03 10 00 00 00 aa 00 00 00 01 02 03 10 2a 2f b2 04 01 00 00 00 34 00 00 00 0d >> ^ >> Fv.c:55:11: runtime error: load of misaligned address 0x000104b23021 for type 'UINT32' (aka 'unsigned int'), which requires 4 byte alignment >> 0x000104b23021: note: pointer points here >> 00 00 00 ff 01 02 03 10 00 00 00 aa 00 00 00 01 02 03 10 2a 2f b2 04 01 00 00 00 34 00 00 00 0d >> ^ >> >> $ clang -fsanitize=undefined Fv.c >> $ cat Fv.c >> typedef unsigned char UINT8; >> typedef UINT8 EFI_SECTION_TYPE; >> typedef unsigned int UINT32; >> typedef unsigned long long UINTN; >> >> typedef UINT8 EFI_SECTION_TYPE; >> >> /// >> /// Common section header. >> /// >> 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; >> /// >> /// Declares the section type. >> /// >> } EFI_COMMON_SECTION_HEADER; >> >> typedef union { >> EFI_COMMON_SECTION_HEADER Hdr; >> UINT32 Uint32; >> } EFI_COMMON_SECTION_HEADER_UNION; >> >> >> #define SECTION_SIZE(SectionHeaderPtr) \ >> ((UINT32) (*((UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size) & 0x00ffffff)) >> >> #define SECTION_SIZE2(SectionHeaderPtr) \ >> (((EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) (SectionHeaderPtr))->Uint32 & 0x00ffffff) >> >> >> typedef struct { >> UINT8 Count; >> EFI_COMMON_SECTION_HEADER Sec; >> } UNALIGNED_SECTION_HEADER; >> >> typedef struct { >> UINT8 Count; >> EFI_COMMON_SECTION_HEADER_UNION Sec; >> } ALIGNED_SECTION_HEADER2; >> >> UNALIGNED_SECTION_HEADER gSection = { 0xff, { { 0x01, 0x02, 0x3 }, 0x10 } }; >> ALIGNED_SECTION_HEADER2 gSection2 = { 0xaa, { } }; >> int >> main (int argc, char **argv) >> { >> UINT32 Size, Size2; >> gSection2.Sec.Uint32 = 0x10030201; >> >> Size = SECTION_SIZE(&gSection.Sec); >> Size2 = SECTION_SIZE2(&gSection.Sec); // runtime error: >> >> Size = SECTION_SIZE(&gSection2.Sec); >> Size2 = SECTION_SIZE2(&gSection2.Sec); >> return Size == Size2; >> } > > Thank you for this investigation! I really appreciate that you wrote it > up, and the example code makes your point clear. > > If my understanding is correct, you are saying that, from an alignment > perspective, both the pre-patch code and the post-patch code are > correct. In other words, we might want to apply the patch, but there > isn't a strong necessity for it. Is my understanding correct? > > If so, I'd like to point out that the original warning (see the commit > message) wasn't about alignment, but about "overrun": > > ------- > 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" > ------- > > This warning was reported by "RH covscan" as "CPPCHECK_WARNING"; that > is, it comes from the cppcheck tool, not from clang-analyzer. So while I > agree that the pre-patch code may not imply an invalid alignment for the > compiler, I claim that the *overrun* (i.e., a different issue) does exist: > > - The pre-patch code takes the Size member, which is an array of 3 UINT8 > elements, > - in that context, the Size member is implicitly converted to a pointer > to the first UINT8 element of the array, i.e. &Size[0], > - the resultant pointer is explicitly cast to a (UINT32*), and then > dereferenced. > > And the last step violates the effective type rules that I quoted > up-thread, because UINT32 is not a type that is compatible with UINT8. > >> PS If folks disagree I know a person that owns the clang ubsan architecture and sits on the C/C++ standards committees. I had a long talk with this person recently about C alignment UB and I think I understand it a little better from the compiler point of view now. > > I certainly accept (and thank you for) your explanation of the > alignment. The patch intends to fix an issue that is not > alignment-related though. > Laszlo, Sorry I digressed into the C specification discussion, and did not deal with the patch in general. My point is the original code is legal C code. If you lookup CWE-119 it is written as a restriction on what the C language allows. As I mentioned casting to specific alignment is legal, as is defining a structure that is pragma pack(1) that can make a UINT32 not be 4 byte aligned. Thus the cast created a legal UINT32 value. A cast to *(UINT32 *) is different that a cast to (UINT32 *). The rules you quote a triggered by the = and not the cast. Thus this is undefined behavior in C: UINT32 *Ub = (UINT32 *)gSection.Sec.Size; Size = *Ub & 0x00ffffff; And this is not Undefined behavior: UINT32 NotUb = *(UINT32 *)gSection.Sec.Size & 0x00ffffff; I also had a hard time interpreting what C spec was saying, but talking to the people who write the compiler and ubsan cleared it up for me. It also makes sense when you think about it. If you tell the compiler *(UINT32 *) it can know to generate byte reads if the hardware requires aligned access. If you do a (UINT32 *) that new pointer no longer carries the information about the alignment requirement. Thus the *(UINT32 *) cast is like making a packed structure. I agree the union is a good solution to CWE-119 and it better matches the alignment requirement in the PI spec. Thanks, Andrew Fish > 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 >>>>>>> >>>>>> >>>> >>> >>> --Boundary_(ID_tG1IeuV7tOGEdBrwJ4AoWA) Content-type: text/html; CHARSET=US-ASCII Content-transfer-encoding: quoted-printable
On Apr 17= , 2019, at 3:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:

Hi Andrew,

On 04/17/19 = 01:25, Andrew Fish wrote:


On Apr 16, 2019, at 11:48 AM, Jordan Justen <jordan.l.justen@intel.com> wrot= e:

On 2019-04-16 03:59:48, Laszlo Ersek wrote:=
On 04/16/19 11:04, Jord= an 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 rep= orts that accessing
"EFI_COMMON_SECTION_HEADER.Size", which i= s of type UINT8[3], through a
(UINT32*), is undefined behavio= r:

Error:= OVERRUN (CWE-119):
edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:1= 78: overrun-local: Overrunning
array of 3 bytes at byte offse= t 3 by dereferencing pointer
"(UINT32 *)((EFI_COMMON_SECTION_= HEADER *)(UINTN)Section)->Size".
#  176|   =     Section =3D (EFI_COMMON_SECTION_HEADER*)(UINTN) Cur= rentAddress;
#  177|
#  178|-> &nb= sp;   Size =3D SECTION_SIZE (Section);
#  = ;179|       if (Size < sizeof (*Section)) = {
#  180|        &nbs= p;return EFI_VOLUME_CORRUPTED;

Fi= x this by introducing EFI_COMMON_SECTION_HEADER_UNION, and expressing
SECTION_SIZE() in terms of "EFI_COMMON_SECTION_HEADER_UNION.Uint32= ".

Cc: Liming Gao <liming.gao@intel.com>
Cc: M= ichael D Kinney <michael.d.kinney@intel.com>
Bugzilla: https://bugz= illa.tianocore.org/show_bug.cgi?id=3D1710
Issue: scan-100= 7.txt
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdePkg/Include/Pi/PiFirmwareFile.h | 10 +++++++++-
1 = file changed, 9 insertions(+), 1 deletion(-)

d= iff --git a/MdePkg/Include/Pi/PiFirmwareFile.h b/MdePkg/Include/Pi/PiFirmwa= reFile.h
index a9f3bcc4eb8e..4fce8298d1c0 100644
--- a/MdePkg/Include/Pi/PiFirmwareFile.h
+++ b/MdePkg/Inclu= de/Pi/PiFirmwareFile.h
@@ -229,16 +229,24 @@ typedef struct {=
 ///
 UINT8     =         Size[3];
&nbs= p;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 {
+  EF= I_COMMON_SECTION_HEADER Hdr;
+  UINT32    = ;            &n= bsp;   Uint32;
+} EFI_COMMON_SECTION_HEADER_UN= ION;
+
typedef struct {
 ///=
 /// A 24-bit unsigned integer that contains the total = size of the section in bytes,
 /// including the EFI_COM= MON_SECTION_HEADER.
 ///
 UINT8  = ;           Size[3];=

 EFI_SECTION_TYPE  Type;
@@ -476,17 +484,17 @@ typedef struct {
 /// A UIN= T16 that represents a particular build. Subsequent builds have monotonicall= y
 /// increasing build numbers relative to earlier buil= ds.
 ///
 UINT16    &n= bsp;            = ;       BuildNumber;
 = ;CHAR16            &= nbsp;           Vers= ionString[1];
} EFI_VERSION_SECTION2;

#define SECTION_SIZE(SectionHeaderPtr) \
-   = ; ((UINT32) (*((UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) Secti= onHeaderPtr)->Size) & 0x00ffffff))
+    = ;(((EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) (SectionHeaderPtr))->Uint= 32 & 0x00ffffff)

Mike, all,
Can we add a typedef for EFI_COMMON_SECTION_HEA= DER_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 typ= e if their types are the same.
Additional rules for determini= ng 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 typ= e of the object, if any. [...]

7 An object sha= ll have its stored value accessed only by an lvalue
expressio= n 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 lar= ger, 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 restr= icts 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 a= re dereferencing a UINT32 pointer.


The thing that is really saving us is the 4 byte alignment= requirement for FV sections in the PI Spec, but that is not expressed by E= FI_COMMON_SECTION_HEADER to the compiler. The union is better as it enforce= s alignment of the structure. This enforced alignment is likely what makes = static analysis tools happy. 

I don't think the casting in the existing= SECTION_SIZE is UB (Undefined Behavior) as the extraction is not the issue= as you are telling the compiler to extract 4 unaligned bytes. That is the = same as accessing a packed structure. But this is only because you started = with a byte pointer so you are not really violating any C alignment rules. = Thus in a case like our cast, or pragma pack(1),  the compiler has ext= ra alignment info provided so all is good, the issue is if you assign a poi= nter into the type the only alignment info the compiler has is the rules fo= r that type, and non of packing rules. A lot of the above C rules are makin= g it clear what the compiler does in these cases. 

Ironically the new u= nion macro is undefined behavior at runtime during the cast if the section = header is not 4 byte algined since you did not start with something (unsign= ed char pointer) that allowed byte access. See my example of the ubsan faul= t. 

The nice thing about the union is it tells the compiler our intende= d constraint from the PI spec. <= /span>

// This 5 byte structure will align EFI= _COMMON_SECTION_HEADER on an odd byte boundary. 
typedef struct {
 = UINT8            &nb= sp;        Count;
&nb= sp;EFI_COMMON_SECTION_HEADER Sec;
} UNALIGNED_SECTION_HEADER;=

// This will properly align EFI_COMMON_SECTIO= N_HEADER_UNION on a 4 byte boundary.&= nbsp;
typedef struct {
 UINT8  = ;            &n= bsp;            = ;Count;
 EFI_COMMON_SECTION_HEADER_UNION Sec;
} ALIGNED_SECTION_HEADER2;

I tried to v= alidate this with the clang ubsan... The only runtime UB was the new macro = against UNALIGNED_SECTION_HEADER. ALIGNED_SECTION_HEADER2 with the new unio= n aligned everything correctly and works fine. 

./a.out
F= v.c:55:11: runtime error: member access within misaligned address 0x000104b= 23021 for type 'EFI_COMMON_SECTION_HEADER_UNION', which requires 4 byte ali= gnment
0x000104b23021: note: pointer points here
00 00 00  ff 01 02 03 10 00 00 00  aa 00 00 00 01 02 03 10 &nbs= p;2a 2f b2 04 01 00 00 00  34 00 00 00 0d
  &n= bsp;          ^ 
Fv.c:55:11: runtime= error: load of misaligned address 0x000104b23021 for type 'UINT32' (aka 'u= nsigned int'), which requires 4 byte alignment
0x000104b23021= : note: pointer points here
00 00 00  ff 01 02 03 10 00 = 00 00  aa 00 00 00 01 02 03 10  2a 2f b2 04 01 00 00 00  34 = 00 00 00 0d
        &= nbsp;    ^ <= /span>

$ clang -fsanitize=3Dundefined Fv.c
$ cat Fv.c
typedef unsigned char    = ;   UINT8;
typedef UINT8    &nb= sp;          EFI_SECTION_= TYPE;
typedef unsigned int      &nbs= p; UINT32;
typedef unsigned long long  UINTN;

typedef UINT8 EFI_SECTION_TYPE;

///
/// Common section header.
/// 
typedef stru= ct {
 ///
 /// A 24-bit unsigned inte= ger that contains the total size of the section in bytes, 
 /// including the EFI= _COMMON_SECTION_HEADER.
 ///
 UINT8 &= nbsp;           Size= [3];
 EFI_SECTION_TYPE  Type;
 /= //
 /// Declares the section type.
 /= //
} EFI_COMMON_SECTION_HEADER;

= typedef union {
 EFI_COMMON_SECTION_HEADER Hdr;
 UINT32          &= nbsp;         Uint32;
} EFI_COMMON_SECTION_HEADER_UNION;


#define SECTION_SIZE(SectionHeaderPtr) \
 &nb= sp; ((UINT32) (*((UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) Sec= tionHeaderPtr)->Size) & 0x00ffffff))

#d= efine SECTION_SIZE2(SectionHeaderPtr) \
   (((= EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) (SectionHeaderPtr))->Uint32 &= amp; 0x00ffffff)


typedef struct= {
 UINT8        &nbs= p;            C= ount;
 EFI_COMMON_SECTION_HEADER Sec;
} UN= ALIGNED_SECTION_HEADER;

typedef struct {
 UINT8          =             &nb= sp;    Count;
 EFI_COMMON_SECTION_HE= ADER_UNION Sec;
} ALIGNED_SECTION_HEADER2;

UNALIGNED_SECTION_HEADER  gSection  =3D { 0xff, { { 0x= 01, 0x02, 0x3 }, 0x10 } };
ALIGNED_SECTION_HEADER2  &nbs= p;gSection2 =3D { 0xaa, {   } };
int
= main (int argc, char **argv)
{
 UINT32 Siz= e, Size2;
 gSection2.Sec.Uint32 =3D 0x10030201;

 Size  =3D SECTION_SIZE(&gSection.Sec);=
 Size2 =3D SECTION_SIZE2(&gSection.Sec);  // r= untime error:

 Size  =3D SECTION_SIZ= E(&gSection2.Sec);
 Size2 =3D SECTION_SIZE2(&gSe= ction2.Sec);
 return Size =3D=3D Size2;
}<= br class=3D"">

Th= ank you for this investigation! I really appreciate that you wrote it
up, and the example code mak= es your point clear.

If my understanding is correct, you are saying that, from an = alignment
perspective, = both the pre-patch code and the post-patch code are
correct. In other words, we might want to appl= y the patch, but there
= isn't a strong necessity for it. Is my understanding correct?

If so, I'd like to point out that the original warning (see the commit
message) wasn't about al= ignment, but about "overrun":
=
-------
Error: OVERRUN (CWE-119):
edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:1= 78: overrun-local: Overrunning
array of 3 bytes at byte offset 3 by dereferencing pointer"(UINT32 *)((EFI_COMMON_SECTION= _HEADER *)(UINTN)Section)->Size"
-------

This warning was reported by "RH covs= can" as "CPPCHECK_WARNING"; that
is, it comes from the cppcheck tool, not from clang-analyzer. So = while I
agree that the = pre-patch code may not imply an invalid alignment for the
compiler, I claim that the *overrun* = (i.e., a different issue) does exist:

- The pre-patch code = takes the Size member, which is an array of 3 UINT8
elements,
- in that context, the Size member is implicitly converted = to a pointer
to the fir= st UINT8 element of the array, i.e. &Size[0],
- the resultant pointer is explicitly cast to a = (UINT32*), and then
der= eferenced.

And the last step violates the effective type ru= les that I quoted
up-= thread, because UINT32 is not a type that is compatible with UINT8.<= br style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 1= 2px; font-style: normal; font-variant-caps: normal; font-weight: normal; le= tter-spacing: normal; text-align: start; text-indent: 0px; text-transform: = none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0p= x; text-decoration: none;" class=3D"">
PS If folks disagree I know a person that owns the cl= ang ubsan architecture and sits on the C/C++ standards committees. I had a = long talk with this person recently about C alignment UB and I think I unde= rstand it a little better from the compiler point of view now. 

I certainly accept (and thank you f= or) your explanation of the
alignment. The patch intends to fix an issue that is not
alignment-related though.

Laszlo,

Sorry I digressed into the C specification discussion, = and did not deal with the patch in general. My point is the original code i= s legal C code. If you lookup CWE-119 it is written as a restriction on wha= t the C language allows. 

As I men= tioned casting to specific alignment is legal, as is defining a structure t= hat is pragma pack(1) that can make a UINT32 not be 4 byte aligned. Thus th= e cast created a legal UINT32 value. A cast to *(UINT32 *) is different tha= t a cast to (UINT32 *). The rules you quote a triggered by the =3D and not = the cast. 

Thus this is undefined = behavior in C:
  UINT32 *Ub =3D (UINT32 *)gSection.Sec.= Size;
  Size =3D *Ub & 0x00ffffff;

And this is not Undefined behavior:
  UINT= 32 NotUb =3D *(UINT32 *)gSection.Sec.Size & 0x00ffffff;

I also had a hard time interpre= ting what C spec was saying, but talking to the people who write the compil= er and ubsan cleared it up for me. It also makes sense when you think about= it. If you tell the compiler *(UINT32 *) it can know to generate byte read= s if the hardware requires aligned access. If you do a (UINT32 *) that new = pointer no longer carries the information about the alignment requirement. = Thus the  *(UINT32 *) cast is like making a packed structure. 

I agree the union i= s a good solution to CWE-119 and it better matches the alignment requiremen= t in the PI spec. 

Thanks,

An= drew Fish


Thanks!Laszlo


-Jordan


\u2014 a type compatible with the effe= ctive type of the object,
\u2014 a qualified version of a typ= e compatible with the effective type
  of the objec= t,
\u2014 a type that is the signed or unsigned type correspo= nding 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 o= f the aforementioned
  types among its members (inc= luding, recursively, a member of a
  subaggregate o= r contained union), or
\u2014 a character type.

- Regarding 6.5p6, the original object we inte= nd to access has
(declared) type EFI_COMMON_SECTION_HEADER. T= herefore 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_COMMO= N_SECTION_HEADER can be accessed as
EFI_COMMON_SECTION_HEADER= _UNION, because EFI_COMMON_SECTION_HEADER_UNION
includes "a t= ype compatible with the effective type of the object" (#1)
am= ong 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
somet= imes 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 S= ECTION_SIZE, the union is well-defined too.

Th= anks,
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:)<= br class=3D"">
Indeed the doubt you voice about ..._UNION cro= ssed my mind, but then I
too searched the PI spec for SECTION= _SIZE, with no hits.

Beyond that, I searched b= oth the PI and UEFI specs, for "_UNION" --
again no hits, des= pite our definitions of:

- EFI_IMAGE_OPTIONAL_= HEADER_UNION
- EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION

in

- "MdePkg/Include/Ind= ustryStandard/PeImage.h"
- "MdePkg/Include/Protocol/GraphicsO= utput.h"

respectively.

Thanks,
Laszlo


-Jordan




<= /div>
--Boundary_(ID_tG1IeuV7tOGEdBrwJ4AoWA)--