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=GzDt9/Em; spf=pass (domain: apple.com, ip: 17.171.2.68, mailfrom: afish@apple.com) Received: from ma1-aaemail-dr-lapp02.apple.com (ma1-aaemail-dr-lapp02.apple.com [17.171.2.68]) by groups.io with SMTP; Tue, 16 Apr 2019 16:25:50 -0700 Received: from pps.filterd (ma1-aaemail-dr-lapp02.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp02.apple.com (8.16.0.27/8.16.0.27) with SMTP id x3GNLurg004453; Tue, 16 Apr 2019 16:25:48 -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=c8FuRUcDbH6ma6M5QDGBZRgwriFXIfLIUXSUpq9a5vM=; b=GzDt9/EmJrn9B+r3M06USLnwHiMLpkD2HiBQ/NCHOEhlam74N62BeM1fs+MuxKekMiML Mgw9Ltk18CAFvwDfgqiN6ZefKpSTGJnVhciO3FYgYnYfNHn5jisz6o+rhDgKnnTLhOKr 51QD2v7xV+xpzMHmFMOyAxzlChEdRQhgUkR9Isap+vYyITHz1d57/3BbgFAwy9xe7L/h 7BtuduU9sVR9H6h1HfWRtGpJAHSW3uKsm1BBAtLRtbIaJ7KDGOd9aGbO0akAC6DaM8M0 JL9Ei0b88f6CGL7hgzy0iwLxj0gsuFPHUoUcS/DXvAUIwNs8rYjYvDO8HCvUOOp7Dev5 cw== Received: from mr2-mtap-s02.rno.apple.com (mr2-mtap-s02.rno.apple.com [17.179.226.134]) by ma1-aaemail-dr-lapp02.apple.com with ESMTP id 2ruceybj0u-2 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Tue, 16 Apr 2019 16:25:48 -0700 MIME-version: 1.0 Received: from nwk-mmpp-sz09.apple.com (nwk-mmpp-sz09.apple.com [17.128.115.80]) by mr2-mtap-s02.rno.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) with ESMTPS id <0PQ200IGUUEZWQC0@mr2-mtap-s02.rno.apple.com>; Tue, 16 Apr 2019 16:25:47 -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 <0PQ200J00UCLK600@nwk-mmpp-sz09.apple.com>; Tue, 16 Apr 2019 16:25:47 -0700 (PDT) X-Va-A: X-Va-T-CD: f54f1ff3cb229f8d9c22945dc7381250 X-Va-E-CD: f08c4a32db9ade74c76bebf226a1f3ee X-Va-R-CD: e8c358aab5b67ff3bc786143aabeee62 X-Va-CD: 0 X-Va-ID: 56dbdd9f-7bfc-4336-bbd0-3f04076acf42 X-V-A: X-V-T-CD: f54f1ff3cb229f8d9c22945dc7381250 X-V-E-CD: f08c4a32db9ade74c76bebf226a1f3ee X-V-R-CD: e8c358aab5b67ff3bc786143aabeee62 X-V-CD: 0 X-V-ID: 0ab84629-867b-4f9c-97b9-423dc51a5151 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-04-16_09:,, signatures=0 Received: from [17.226.41.197] (unknown [17.226.41.197]) by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) with ESMTPSA id <0PQ200KK2UEY7L40@nwk-mmpp-sz09.apple.com>; Tue, 16 Apr 2019 16:25:47 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: <940201E3-0EDB-40B8-8680-CDE68DA0FD06@apple.com> Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE Date: Tue, 16 Apr 2019 16:25:41 -0700 In-reply-to: <155544052538.15733.153410443320244157@jljusten-skl> Cc: Laszlo Ersek , Mike Kinney , Liming Gao To: devel@edk2.groups.io, Jordan Justen 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> X-Mailer: Apple Mail (2.3445.6.18) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-04-16_09:,, signatures=0 Content-type: multipart/alternative; boundary="Boundary_(ID_dibpXldVXoqRSlinNuL5HA)" --Boundary_(ID_dibpXldVXoqRSlinNuL5HA) Content-type: text/plain; CHARSET=US-ASCII Content-transfer-encoding: 7BIT > 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; } Thanks, Andrew Fish 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. > -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_dibpXldVXoqRSlinNuL5HA) Content-type: text/html; CHARSET=US-ASCII Content-transfer-encoding: quoted-printable


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

On 2019-04-16 03:59:48, Laszlo Ersek wro= te:
On 04/16/19 11:04, J= ordan Justen wrote:
On 2= 019-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", whic= h is of type UINT8[3], through a
(UINT32*), is undefined beha= vior:

Err= or: OVERRUN (CWE-119):
edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.= c:178: overrun-local: Overrunning
array of 3 bytes at byte of= fset 3 by dereferencing pointer
"(UINT32 *)((EFI_COMMON_SECTI= ON_HEADER *)(UINTN)Section)->Size".
#  176|  &nb= sp;    Section =3D (EFI_COMMON_SECTION_HEADER*)(UINTN) = CurrentAddress;
#  177|
#  178|-> =     Size =3D SECTION_SIZE (Section);
# &n= bsp;179|       if (Size < sizeof (*Section= )) {
#  180|        &= nbsp;return EFI_VOLUME_CORRUPTED;

Fix this by introducing EFI_COMMON_SECTION_HEADER_UNION, and expressingSECTION_SIZE() in terms of "EFI_COMMON_SECTION_HEADER_UNION.Uin= t32".

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

diff --git a/MdePkg/Include/Pi/PiFirmwareFile.h b/MdePkg/Include/P= i/PiFirmwareFile.h
index a9f3bcc4eb8e..4fce8298d1c0 100644--- a/MdePkg/Include/Pi/PiFirmwareFile.h
+++ b/Md= ePkg/Include/Pi/PiFirmwareFile.h
@@ -229,16 +229,24 @@ typede= f struct {
  ///
  UINT8 =             Siz= e[3];
  EFI_SECTION_TYPE  Type;
  ///
  /// Declares the section type.=
  ///
} EFI_COMMON_SECTION_HEADER;=

+///
+/// Union that permits ac= cessing EFI_COMMON_SECTION_HEADER as a UINT32 object.
+///+typedef union {
+  EFI_COMMON_SECTION_HEADE= R Hdr;
+  UINT32       &nb= sp;            = 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_COM= MON_SECTION_HEADER.
  ///
 &nb= sp;UINT8            =  Size[3];

  EFI_SECTION_TYPE &= nbsp;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         =             &nb= sp;  BuildNumber;
  CHAR16   &= nbsp;           &nbs= p;        VersionString[1];
} EFI_VERSION_SECTION2;

#define SECTI= ON_SIZE(SectionHeaderPtr) \
-    ((UINT32) (*(= (UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Siz= e) & 0x00ffffff))
+    (((EFI_COMMON_SECTI= ON_HEADER_UNION *) (UINTN) (SectionHeaderPtr))->Uint32 & 0x00ffffff)=

Mike, all,

Can we add a typedef for EFI_COMMON_SECTION_HEADER_UNION if it's no= t
in the PI spec?

If it's not al= lowed, 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.
<= br class=3D"">I think you are still accessing it through a UINT32*, since y= ou are
using a pointer to a union, and an field of type UINT3= 2 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 A= n 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 lva= lues, 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 d= ereferenced.

If the pointer is dereferenced to= a type that is larger, then I
understand that there are case= s where deferencing the pointer could
have unintended side ef= fects, but that is not the case here.

The dere= ference could also be undefined if big-endian was in the
pict= ure, but UEFI restricts that.

Of course none o= f this stops a tool from trying to delve further into
the poi= nter 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 thin= g that is really saving us is the 4 byte alignment requirement for FV secti= ons in the PI Spec, but that is not expressed by EFI_COMMON_SECTION_HE= ADER to the compiler. The union is better as it enforces alignment of the s= tructure. This enforced alignment is likely what makes static analysis tool= s happy. 

I don't think the castin= g 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 by= tes. That is the same as accessing a packed structure. But this is only bec= ause 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 ha= s 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 undefin= ed 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 comp= iler our intended constraint from the PI spec. 

// This 5 byte structure will align EFI_COMMON_SECTION_HEADER= on an odd byte boundary. 
typedef struct {
&= nbsp; UINT8                   =   Count;
  EFI_COMMON_SECTION_HEADER Sec;
} U= NALIGNED_SECTION_HEADER;

// This will p= roperly align EFI_COMMON_SECTION_HEADER_UNION on a 4 byte boundary.&nb= sp;
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 t= he 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 e= rror: member access within misaligned address 0x000104= b23021 for type 'EFI_COMMON_SECTION_HEADER_UNION', which requires 4 byte al= ignment
0x000104b23021: note: pointer = points here
 00 00 00  ff 01 02 03 10 00 00 00&nbs= p; aa 00 00 00 01 02 03 10  2a 2f b2 04 01 00 00 00  34 00 00 00 = 0d
    &nbs= p;         ^ 
= 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
              ^&= nbsp;

$= clang -fsanitize=3Dundefined Fv.c
$ cat Fv.c
typedef unsigned char     &nb= sp; UINT8;
typedef UINT8           =     EFI_SECTION_TYPE;
typedef unsigned int  &= nbsp;     UINT32;
typedef unsigned long long = UINTN;

typ= edef UINT8 EFI_SECTION_TYPE;

///
/// Common section header.
/// 
typedef struct {
  ///<= /span>
  /// A 24-bit unsigned integer that contains the total= size of the section in bytes, 
  /// including th= e EFI_COMMON_SECTION_HEADER.
  ///
<= span style=3D"font-variant-ligatures: no-common-ligatures" class=3D""> = ; UINT8             Size[3];
&= nbsp; EFI_SECTION_TYPE  Type;
  ///
  /// Declares the section type.
  ///
} EFI_COMMON_SECTION_HEADER;

typedef union {
  EFI_COMMON_S= ECTION_HEADER Hdr;
  UINT32        =             Uint32;
} EFI_COMM= ON_SECTION_HEADER_UNION;


#defin= e SECTION_SIZE(SectionHeaderPtr) \
    ((UINT32) (= *((UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->S= ize) & 0x00ffffff))

#define SECTION_SIZE2(SectionHeaderPtr) \
&n= bsp;   (((EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) (SectionHeaderPtr= ))->Uint32 & 0x00ffffff)

<= span style=3D"font-variant-ligatures: no-common-ligatures" class=3D"">typed= ef struct {
  UINT8           =           Count;
  EFI_COMMON_= SECTION_HEADER Sec;
} UNALIGNED_SECTION_HEADER;
=

typedef struct {=
  UINT8               &nbs= p;           Count;
  EFI_COM= MON_SECTION_HEADER_UNION Sec;
} ALIGNED_SECTION_HEADER2;

UNALIGNED_SECT= ION_HEADER  gSection  =3D { 0xff, { { 0x01, 0x02, 0x3 }, 0x10 } }= ;
ALIGNED_SECTION_HEADER2   gSection2 =3D { 0xaa, { &nb= sp; } };
int
main (int argc, char **argv)
{
  UINT32 Size, Size2;
= &nbs= p; gSection2.Sec.Uint32 =3D 0x10030201;
&n= bsp; 
  Siz= e  =3D SECTION_SIZE(&gSection.Sec);
  Size2 = =3D SECTION_SIZE2(&gSection.Sec);  // runtime error:
=

  Size  =3D S= ECTION_SIZE(&gSection2.Sec);
  Size2 =3D SECTION_SI= ZE2(&gSection2.Sec);
  return Size =3D=3D Size2;
}

Thanks,

Andrew Fish
=
PS If folks disagree I know a person that owns th= e clang ubsan architecture and sits on the C/C++ standards committees. I ha= d 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. 
-Jordan


 \u201= 4 a type compatible with the effective type of the object,
&= nbsp;\u2014 a qualified version of a type compatible with the effective typ= e
   of the object,
 \u20= 14 a type that is the signed or unsigned type corresponding to the
   effective type of the object,
&nbs= p;\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 includ= es one of the aforementioned
   types among i= ts members (including, recursively, a member of a
 &nbs= p; 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 E= FI_COMMON_SECTION_HEADER.

- Based on 6.2.7p1, = EFI_COMMON_SECTION_HEADER is compatible with
EFI_COMMON_SECTI= ON_HEADER. (Because they are the same.)

- Base= d on 6.5p7 item #5, EFI_COMMON_SECTION_HEADER can be accessed as
EFI_COMMON_SECTION_HEADER_UNION, because EFI_COMMON_SECTION_HEADER_U= NION
includes "a type compatible with the effective type of t= he object" (#1)
among its members -- namely an EFI_COMMON_SEC= TION_HEADER, which is
compatible with EFI_COMMON_SECTION_HEAD= ER, 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-d= efined 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 s= pec, so maybe it's ok to
add the typedef.

(More importantly:)

Ind= eed 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,
Las= zlo


-Jordan


=





--Boundary_(ID_dibpXldVXoqRSlinNuL5HA)--