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.115, mailfrom: michael.d.kinney@intel.com) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by groups.io with SMTP; Wed, 17 Apr 2019 11:36:44 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Apr 2019 11:36:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,362,1549958400"; d="scan'208,217";a="141524161" Received: from orsmsx103.amr.corp.intel.com ([10.22.225.130]) by fmsmga008.fm.intel.com with ESMTP; 17 Apr 2019 11:36:42 -0700 Received: from orsmsx114.amr.corp.intel.com (10.22.240.10) by ORSMSX103.amr.corp.intel.com (10.22.225.130) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 17 Apr 2019 11:36:39 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.24]) by ORSMSX114.amr.corp.intel.com ([169.254.8.50]) with mapi id 14.03.0415.000; Wed, 17 Apr 2019 11:36:39 -0700 From: "Michael D Kinney" To: "devel@edk2.groups.io" , "afish@apple.com" , "Kinney, Michael D" CC: "lersek@redhat.com" , "Gao, Liming" Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE Thread-Topic: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE Thread-Index: AQHU8YfjFhWGOuXuXkWmHX/V/l+BpaZAp/hQgACCAID//4tjEA== Date: Wed, 17 Apr 2019 18:36:38 +0000 Message-ID: References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-5-lersek@redhat.com> <6FA88780-622D-4788-BDF8-6C807D85D9D1@apple.com> In-Reply-To: <6FA88780-622D-4788-BDF8-6C807D85D9D1@apple.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.22.254.138] MIME-Version: 1.0 Return-Path: michael.d.kinney@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_E92EE9817A31E24EB0585FDF735412F5B9C9A4EAORSMSX113amrcor_" --_000_E92EE9817A31E24EB0585FDF735412F5B9C9A4EAORSMSX113amrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Andrew, My suggestion is to read the 3 bytes and shift and or to build 24-bit valu= e. That is what is in the patch at the bottom. It uses an extra layer of = macros that I am not in favor of. There is an additional email with a propo= sed approach that makes the use of the array members more obvious. Mike From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andr= ew Fish via Groups.Io Sent: Wednesday, April 17, 2019 11:32 AM To: devel@edk2.groups.io; Kinney, Michael D Cc: lersek@redhat.com; Gao, Liming Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefin= ed behavior in FFS_FILE_SIZE Mike, I kind of ratholed us on alignment when Laszlo was more concerned about st= rict aliasing and the effective type rule. Sorry! I don't think your propos= al fixes the effective type issue, and actually may make it worse if I'm co= rrect. It seems like the safest thing to do is read byte by byte and shift which = I thought we did a long time ago? The last point I poorly tried to make to Laszlo was I thought you could ca= st around the effective type issue in C99. If you think about it the union = and the cast are both telling the compiler the intent is to break the effec= tive type rule and that is the bigger point I was trying to make. Given my = insomnia I used alignment examples that I understand better. If we try to c= onvert Size to a UINT32 * I think that does trigger the effect type issue L= aszlo referenced. So I was only debating the boundary of enforcement of the= effect type rule using alignment as a confusing example. I was actually writing a mail to some people that sit on the C/C++ standar= ds committee that are UB experts to get some clarification when you sent th= is mail. I'm concerned I'm conflating behavior with what the standard state= s, and may have some recency bias with solving alignment issues that makes = me think the cast should work. I'm basically asking if this code pedantic conforms to C99 and C11: EFI_COMMON_SECTION_HEADER gSec =3D { { 0x01, 0x02, 0x3 }, 0x10 }; return *(UINT32 *)gSec.Size & 0x00ffffff; I ran the clang static analyzer and runtime ubsan on the above code and it= did not complain (I force strict aliasing via -fstrict-aliasing, and I'm u= sing the Sys V ABI since this is just the command line compiler on my Mac). Thanks, Andrew Fish On Apr 17, 2019, at 10:52 AM, Michael D Kinney > wrote: Laszlo, I have been following this thread. I think the style used here to access the 3 array elements to build the 24-bit size value is the best approach. I prefer this over adding the union. I agree there is a read overrun issue when using UINT32 to read the Size[3] array contents. I do not think this is a real issue in practice, because the Size[3] array accessed is part of the larger EFI_COMMON_SECTION_HEADER structure. However, we always should clean up code to not do any read/write overruns without this type of analysis and the need to keep track of exceptions. There is a related set of code in the BaseLib for Read/Write Unaligned24(). UINT32 EFIAPI ReadUnaligned24 ( IN CONST UINT32 *Buffer ); UINT32 EFIAPI WriteUnaligned24 ( OUT UINT32 *Buffer, IN UINT32 Value ); This API does not get flagged for read overrun issues because a UINT32 is passed in. However, for CPU archs that required aligned access, the 24-bit value must be read in pieces. This is why there are 2 different implementations: IA32/X64 =3D=3D=3D=3D=3D=3D=3D=3D UINT32 EFIAPI ReadUnaligned24 ( IN CONST UINT32 *Buffer ) { ASSERT (Buffer !=3D NULL); return *Buffer & 0xffffff; } ARM/AARCH64 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D UINT32 EFIAPI ReadUnaligned24 ( IN CONST UINT32 *Buffer ) { ASSERT (Buffer !=3D NULL); return (UINT32)( ReadUnaligned16 ((UINT16*)Buffer) | (((UINT8*)Buffer)[2] << 16) ); } The ARM/ARCH64 implementation is clean because it does not do a read overrun of the 24-bit field. The IA32/X64 implementation may have an issue because it reads a 32-bit value and strips the upper 8 bits. If we apply the same technique to the Size field of EFI_COMMON_SECTION_HEADER, then the 24-bit value would be built from reading only the 3 bytes of the array. Best regards, Mike -----Original Message----- From: devel@edk2.groups.io [mailto:devel@edk2= .groups.io] On Behalf Of Laszlo Ersek Sent: Friday, April 12, 2019 4:31 PM To: edk2-devel-groups-io > Cc: Gao, Liming >; Kinne= y, Michael D > Subject: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE Accessing "EFI_FFS_FILE_HEADER.Size", which is of type UINT8[3], through a (UINT32*), is undefined behavior. Fix it by accessing the array elements individually. (We can't use a union here, unfortunately, as easily as with "EFI_COMMON_SECTION_HEADER", given the fields in "EFI_FFS_FILE_HEADER".) Cc: Liming Gao > Cc: Michael D Kinney > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1710 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 4fce8298d1c0..0668f3fa9af4 100644 --- a/MdePkg/Include/Pi/PiFirmwareFile.h +++ b/MdePkg/Include/Pi/PiFirmwareFile.h @@ -174,18 +174,26 @@ typedef struct { /// If FFS_ATTRIB_LARGE_FILE is not set then EFI_FFS_FILE_HEADER is used. /// UINT64 ExtendedSize; } EFI_FFS_FILE_HEADER2; #define IS_FFS_FILE2(FfsFileHeaderPtr) \ (((((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Attributes) & FFS_ATTRIB_LARGE_FILE) =3D=3D FFS_ATTRIB_LARGE_FILE) +#define FFS_FILE_SIZE_ARRAY(FfsFileHeaderPtr) \ + (((EFI_FFS_FILE_HEADER *) (UINTN) (FfsFileHeaderPtr))->Size) + +#define FFS_FILE_SIZE_ELEMENT(FfsFileHeaderPtr, Index) \ + ((UINT32) FFS_FILE_SIZE_ARRAY (FfsFileHeaderPtr)[(Index)]) + #define FFS_FILE_SIZE(FfsFileHeaderPtr) \ - ((UINT32) (*((UINT32 *) ((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size) & 0x00ffffff)) + ((FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 0) << 0) | \ + (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 1) << 8) | \ + (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 2) << 16)) #define FFS_FILE2_SIZE(FfsFileHeaderPtr) \ ((UINT32) (((EFI_FFS_FILE_HEADER2 *) (UINTN) FfsFileHeaderPtr)->ExtendedSize)) typedef UINT8 EFI_SECTION_TYPE; /// /// Pseudo type. It is used as a wild card when retrieving sections. -- 2.19.1.3.g30247aa5d201 -=3D-=3D-=3D-=3D-=3D-=3D Groups.io Links: You receive all messages sent to this group. View/Reply Online (#38989): https://edk2.groups.io/g/devel/message/38989 Mute This Topic: https://groups.io/mt/31070304/1643496 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com] -=3D-=3D-=3D-=3D-=3D-=3D --_000_E92EE9817A31E24EB0585FDF735412F5B9C9A4EAORSMSX113amrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Andrew,

 

My suggestion is to read the 3 bytes and shift and or to build= 24-bit value.  That is what is in the patch at the bottom.  It uses an extra layer of macros that I am not in favor of. There i= s an additional email with a proposed approach that makes the use of the ar= ray members more obvious.

 

Mike

 

From: = devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andrew Fish via Groups.= Io
Sent: Wednesday, April 17, 2019 11:32 AM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@in= tel.com>
Cc: lersek@redhat.com; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix = undefined behavior in FFS_FILE_SIZE

 

Mike,

 

I kind of ratholed us on alignment when Laszlo was more co= ncerned about strict aliasing and the effective type rule. Sorry! I don't t= hink your proposal fixes the effective type issue, and actually may make it worse if I'm correct. 

 

It seems like the safest thing to do is read byte by byte = and shift which I thought we did a long time ago?

 

The last point I poorly tried to make to Laszlo was I thou= ght you could cast around the effective type issue in C99. If you think abo= ut it the union and the cast are both telling the compiler the intent is to break the effective type rule and that is the b= igger point I was trying to make. Given my insomnia I used alignment exampl= es that I understand better. If we try to convert Size to a UINT32 * I thin= k that does trigger the effect type issue Laszlo referenced. So I was only debating the boundary of enforceme= nt of the effect type rule using alignment as a confusing example. 

 

I was actually writing a mail to some people that sit on t= he C/C++ standards committee that are UB experts to get some clarif= ication when you sent this mail. I'm concerned I'm conflating behavior with what the standard states, and may have some recency bias wi= th solving alignment issues that makes me think the cast should work. =

 

I'm basically asking if this code pedantic conforms to C99= and C11:

 

EFI_COMMON_SECTION_HEADER gSec =3D { { 0x01, 0x02, 0x3 }, 0x10 }= ;

 

return *(UINT32 *)gSec.Size & 0x00ffffff;

 

I ran the clang static analyzer and runtime ubsan on the a= bove code and it did not complain (I force strict aliasing via -fstric= t-aliasing, and I'm using the Sys V ABI since this is just the command line compiler on my Mac). 

 

Thanks,

 

Andrew Fish



On Apr 17, 2019, at 10:52 AM, Michael D Kinney <michael.d.kinney@intel.com> = wrote:

 

Laszlo,

I have been following this thread.  I think the style
used here to access the 3 array elements to build the
24-bit size value is the best approach.  I prefer this
over adding the union.

I agree there is a read overrun issue when using UINT32 to
read the Size[3] array contents.

I do not think this is a real issue in practice, because the
Size[3] array accessed is part of the larger
EFI_COMMON_SECTION_HEADER structure.  However, we always should
clean up code to not do any read/write overruns without this
type of analysis and the need to keep track of exceptions.

There is a related set of code in the BaseLib for Read/Write
Unaligned24().

UINT32
EFIAPI
ReadUnaligned24 (
 IN CONST UINT32         &nbs= p;    *Buffer
 );

UINT32
EFIAPI
WriteUnaligned24 (
 OUT UINT32          &nb= sp;         *Buffer,
 IN  UINT32         &nbs= p;          Value
 );

This API does not get flagged for read overrun issues because
a UINT32 is passed in.  However, for CPU archs that required aligned<= br> access, the 24-bit value must be read in pieces.  This is why there are 2 different implementations:

IA32/X64
=3D=3D=3D=3D=3D=3D=3D=3D
UINT32
EFIAPI
ReadUnaligned24 (
 IN CONST UINT32         &nbs= p;    *Buffer
 )
{
 ASSERT (Buffer !=3D NULL);

 return *Buffer & 0xffffff;
}


ARM/AARCH64
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
UINT32
EFIAPI
ReadUnaligned24 (
 IN CONST UINT32         &nbs= p;    *Buffer
 )
{
 ASSERT (Buffer !=3D NULL);

 return (UINT32)(
           ReadUnal= igned16 ((UINT16*)Buffer) |
           (((UINT8= *)Buffer)[2] << 16)
           );
}

The ARM/ARCH64 implementation is clean because it does
not do a read overrun of the 24-bit field.  The IA32/X64
implementation may have an issue because it reads a 32-bit
value and strips the upper 8 bits.

If we apply the same technique to the Size field of
EFI_COMMON_SECTION_HEADER, then the 24-bit value would be
built from reading only the 3 bytes of the array.

Best regards,

Mike


-----Original Message-----
From: devel@edk2.groups.io [mailto:devel= @edk2.groups.io]
On Behalf Of Laszlo Ersek
Sent: Friday, April 12, 2019 4:31 PM
To: edk2-devel-groups-io <devel= @edk2.groups.io>
Cc: Gao, Liming <liming.gao@int= el.com>; Kinney, Michael
D <michael.d.kinney@intel= .com>
Subject: [edk2-devel] [PATCH 04/10]
MdePkg/PiFirmwareFile: fix undefined behavior in
FFS_FILE_SIZE

Accessing "EFI_FFS_FILE_HEADER.Size", which is of type
UINT8[3], through a
(UINT32*), is undefined behavior. Fix it by accessing
the array elements
individually.

(We can't use a union here, unfortunately, as easily as
with
"EFI_COMMON_SECTION_HEADER", given the fields in
"EFI_FFS_FILE_HEADER".)

Cc: Liming Gao <liming.gao@inte= l.com>
Cc: Michael D Kinney <mic= hael.d.kinney@intel.com>
Bugzilla:
https://= bugzilla.tianocore.org/show_bug.cgi?id=3D1710
Signed-off-by: Laszlo Ersek <lerse= k@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/Pi/PiFirmwareFile.h
index 4fce8298d1c0..0668f3fa9af4 100644
--- a/MdePkg/Include/Pi/PiFirmwareFile.h
+++ b/MdePkg/Include/Pi/PiFirmwareFile.h
@@ -174,18 +174,26 @@ typedef struct {
  /// If FFS_ATTRIB_LARGE_FILE is not set then
EFI_FFS_FILE_HEADER is used.
  ///
  UINT64          &= nbsp;         ExtendedSize; } EFI_FFS_FILE_HEADER2;

#define IS_FFS_FILE2(FfsFileHeaderPtr) \
    (((((EFI_FFS_FILE_HEADER *) (UINTN)
FfsFileHeaderPtr)->Attributes) & FFS_ATTRIB_LARGE_FILE)
=3D=3D FFS_ATTRIB_LARGE_FILE)

+#define FFS_FILE_SIZE_ARRAY(FfsFileHeaderPtr) \
+    (((EFI_FFS_FILE_HEADER *) (UINTN)
(FfsFileHeaderPtr))->Size)
+
+#define FFS_FILE_SIZE_ELEMENT(FfsFileHeaderPtr, Index)
\
+    ((UINT32) FFS_FILE_SIZE_ARRAY
(FfsFileHeaderPtr)[(Index)])
+
#define FFS_FILE_SIZE(FfsFileHeaderPtr) \
-    ((UINT32) (*((UINT32 *) ((EFI_FFS_FILE_HEADER *)
(UINTN) FfsFileHeaderPtr)->Size) & 0x00ffffff))
+    ((FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 0) &l= t;<
0) | \
+     (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), = 1) <<
8) | \
+     (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), = 2) <<
16))

#define FFS_FILE2_SIZE(FfsFileHeaderPtr) \
    ((UINT32) (((EFI_FFS_FILE_HEADER2 *) (UINTN)
FfsFileHeaderPtr)->ExtendedSize))

typedef UINT8 EFI_SECTION_TYPE;

///
/// Pseudo type. It is used as a wild card when
retrieving sections.
--
2.19.1.3.g30247aa5d201



-=3D-=3D-=3D-=3D-=3D-=3D
Groups.io Links: You receive all messages sent to this
group.

View/Reply Online (#38989):
https://edk2.grou= ps.io/g/devel/message/38989
Mute This Topic: https://groups.io/mt/31070304/1= 643496
Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub<= /a>
[michael.d.kinney@intel.com<= /a>]
-=3D-=3D-=3D-=3D-=3D-=3D




 

--_000_E92EE9817A31E24EB0585FDF735412F5B9C9A4EAORSMSX113amrcor_--