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=mLVPS2ww; spf=pass (domain: apple.com, ip: 17.171.2.60, mailfrom: afish@apple.com) Received: from ma1-aaemail-dr-lapp01.apple.com (ma1-aaemail-dr-lapp01.apple.com [17.171.2.60]) by groups.io with SMTP; Wed, 17 Apr 2019 11:32:44 -0700 Received: from pps.filterd (ma1-aaemail-dr-lapp01.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp01.apple.com (8.16.0.27/8.16.0.27) with SMTP id x3HIQV6w015702; Wed, 17 Apr 2019 11:32:43 -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=VDjkMhALReEY0UKzcnESpEL+OqvuGXvACrUG1ncdOuY=; b=mLVPS2ww+9yKc+EbOprfoGXf/CZPQwldqgd08arOiu0hJaJg9f9b2Jp3XLKsuo18oVkJ TZj8Rnt1I6CbeyHvLH2bd6yzzs2313IGBrLN1HxXAidm4PrklxG7M21yMQtBjDw8k1mz pAGjxXcRS5igiBhN/v2b1ZZxztYPr87Z6ve9Z6wuJJtnTKzaWynJqTRwG1vDPkPVt//2 EIw+FODuYqRQqrx5F/s/kTcD9D6wal9hW4T2MPapB8xitwfNNlM29OSTCK+hXKSbxNP+ aCGSJYHF4KwOkgm27KfwTvhDASzs629CLod5UHNpLWwr8e1TInwa8ONgcFbCbW9tzyeD UA== Received: from mr2-mtap-s02.rno.apple.com (mr2-mtap-s02.rno.apple.com [17.179.226.134]) by ma1-aaemail-dr-lapp01.apple.com with ESMTP id 2ruee9ffs1-14 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 17 Apr 2019 11:32:43 -0700 MIME-version: 1.0 Received: from nwk-mmpp-sz13.apple.com (nwk-mmpp-sz13.apple.com [17.128.115.216]) by mr2-mtap-s02.rno.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) with ESMTPS id <0PQ400EXTBIGV920@mr2-mtap-s02.rno.apple.com>; Wed, 17 Apr 2019 11:32:40 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz13.apple.com by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) id <0PQ400B00BF5E100@nwk-mmpp-sz13.apple.com>; Wed, 17 Apr 2019 11:32:40 -0700 (PDT) X-Va-A: X-Va-T-CD: e48e8dc3f6c377b8dc939b4126ad19f3 X-Va-E-CD: 7d8f040fcc5f7a40fdf5cf3d4378ebc7 X-Va-R-CD: 33fbd2273acab4e6baf248d1316e523c X-Va-CD: 0 X-Va-ID: a16e3bb5-f738-4918-8b73-ad0ef68e0018 X-V-A: X-V-T-CD: ea8ecdd7c7fec670404234df68c44261 X-V-E-CD: 7d8f040fcc5f7a40fdf5cf3d4378ebc7 X-V-R-CD: 33fbd2273acab4e6baf248d1316e523c X-V-CD: 0 X-V-ID: 6781a48e-13e9-478a-828f-fec8490527c8 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-04-17_08:,, signatures=0 Received: from [17.226.41.197] (unknown [17.226.41.197]) by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) with ESMTPSA id <0PQ400EX7BHG8150@nwk-mmpp-sz13.apple.com>; Wed, 17 Apr 2019 11:32:05 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: <6FA88780-622D-4788-BDF8-6C807D85D9D1@apple.com> Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE Date: Wed, 17 Apr 2019 11:31:59 -0700 In-reply-to: Cc: "lersek@redhat.com" , "Gao, Liming" To: devel@edk2.groups.io, Mike Kinney References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-5-lersek@redhat.com> X-Mailer: Apple Mail (2.3445.6.18) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-04-17_08:,, signatures=0 Content-type: multipart/alternative; boundary="Boundary_(ID_5lJv2j52fmnaN6Q8X3SNqg)" --Boundary_(ID_5lJv2j52fmnaN6Q8X3SNqg) Content-type: text/plain; CHARSET=US-ASCII Content-transfer-encoding: 7BIT Mike, I kind of ratholed us on alignment when Laszlo was more concerned about strict aliasing and the effective type rule. Sorry! I don't think 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 thought you could cast 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 effective 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 convert Size to a UINT32 * I think that does trigger the effect type issue Laszlo 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++ standards committee that are UB experts to get some clarification when you sent this mail. I'm concerned I'm conflating behavior with what the standard states, 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 = { { 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 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 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 > ======== > UINT32 > EFIAPI > ReadUnaligned24 ( > IN CONST UINT32 *Buffer > ) > { > ASSERT (Buffer != NULL); > > return *Buffer & 0xffffff; > } > > > ARM/AARCH64 > ============ > UINT32 > EFIAPI > ReadUnaligned24 ( > IN CONST UINT32 *Buffer > ) > { > ASSERT (Buffer != 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 >; Kinney, 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=1710 >> 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) >> == 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 >> >> >> >> -=-=-=-=-=-= >> 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 ] >> -=-=-=-=-=-= > > > --Boundary_(ID_5lJv2j52fmnaN6Q8X3SNqg) Content-type: text/html; CHARSET=US-ASCII Content-transfer-encoding: quoted-printable Mike,
<= br class=3D"">
I kind of ratholed us on alignment when= Laszlo was more concerned about strict aliasing and the effective type rul= e. Sorry! I don't think your proposal fixes the effective type issue, and a= ctually 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 trie= d to make to Laszlo was I thought you could cast 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 effective type rule and that is th= e bigger point I was trying to make. Given my insomnia I used alignment exa= mples that I understand better. If we try to convert Size to a UINT32 * I t= hink that does trigger the effect type issue Laszlo referenced. So I was on= ly debating the boundary of enforcement of the effect type rule using align= ment as a confusing example. 

I was actually writing a mail to some people that sit on = the C/C++ standards committee that are UB experts to get some clarification= when you sent this mail. I'm concerned I'm conflating behavior with what t= he standard states, and may have some recency bias with solving alignment i= ssues that makes me think the cast should work. 
=
I'm basically asking if this code peda= ntic 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 stric= t aliasing via -fstrict-aliasing, and I'm using the Sys V ABI since th= is 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 h= ave been following this thread.  I think the style
used here to access the 3 array elements t= o build the
24-bit size= value is the best approach.  I prefer this
over adding the union.

I agree th= ere 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 t= he larger
EFI_COMMON_SE= CTION_HEADER structure.  However, we always should
clean up code to not do any read/write ove= rruns without this
typ= e of analysis and the need to keep track of exceptions.

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

UIN= T32
EFIAPI
ReadUnaligned24 (
 IN CONST UINT32   &nb= sp;          *Buffer
 );

UI= NT32
EFIAPI
WriteUnaligned24 (
 OUT UINT32    &n= bsp;            = ;   *Buffer,
 IN  UINT32        &nbs= p;           Value
 );

This API does not get flagged for read overrun issues because
a UINT32 is passed in.  Howev= er, for CPU archs that required aligned
access, the 24-bit value must be read in pieces.  Thi= s is why there
are 2 di= fferent implementations:

IA32/X64
=3D=3D=3D=3D=3D=3D=3D=3D
UINT32
EFIAPI
Re= adUnaligned24 (
 I= N CONST UINT32           =    *Buffer
 )
{
 ASSERT (Buffer !=3D NULL);<= /span>

 return *Buffer & 0xffffff;
}


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

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

Th= e ARM/ARCH64 implementation is clean because it does
not do a read overrun of the 24-bit field. &n= bsp;The IA32/X64
imp= lementation may have an issue because it reads a 32-bit
value and strips the upper 8 bits.<= 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"">
If we apply the same technique to the Size field of
EFI_COMMON_SECTION_HEADER, then the 2= 4-bit value would be
bu= ilt from reading only the 3 bytes of the array.

Best regard= s,

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 P= M
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Gao, Li= ming <liming.gao@inte= l.com>; Kinney, Michael
D <michael.d.kinney@intel.com>
Subject: [edk2-devel] [PATCH 04/10]
MdePkg/PiFirmwareF= ile: 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
individu= ally.

(We can't use a union here, unfortunatel= y, as easily as
with
"EFI_COMMON_SECTION_HEADER= ", given the fields in
"EFI_FFS_FILE_HEADER".)
=
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney &= lt;michael.d.kinne= y@intel.com>
Bugzilla:
https://bugzill= a.tianocore.org/show_bug.cgi?id=3D1710
Signed-off-by: Las= zlo Ersek <lersek@redhat.com>
---
MdePkg/= Include/Pi/PiFirmwareFile.h | 10 +++++++++-
1 file changed, 9= insertions(+), 1 deletion(-)

diff --git a/Mde= Pkg/Include/Pi/PiFirmwareFile.h
b/MdePkg/Include/Pi/PiFirmwar= eFile.h
index 4fce8298d1c0..0668f3fa9af4 100644
--- a/MdePkg/Include/Pi/PiFirmwareFile.h
+++ b/MdePkg/Includ= e/Pi/PiFirmwareFile.h
@@ -174,18 +174,26 @@ typedef struct {<= br class=3D"">  /// If FFS_ATTRIB_LARGE_FILE is not set then
EFI_FFS_FILE_HEADER is used.
  ///
  UINT64         =            ExtendedS= ize;
} EFI_FFS_FILE_HEADER2;

#de= fine IS_FFS_FILE2(FfsFileHeaderPtr) \
    = ;(((((EFI_FFS_FILE_HEADER *) (UINTN)
FfsFileHeaderPtr)->At= tributes) & FFS_ATTRIB_LARGE_FILE)
=3D=3D FFS_ATTRIB_LARG= E_FILE)

+#define FFS_FILE_SIZE_ARRAY(FfsFileHe= aderPtr) \
+    (((EFI_FFS_FILE_HEADER *) (UIN= TN)
(FfsFileHeaderPtr))->Size)
+
+#define FFS_FILE_SIZE_ELEMENT(FfsFileHeaderPtr, Index)
\<= br class=3D"">+    ((UINT32) FFS_FILE_SIZE_ARRAY
(FfsFileHeaderPtr)[(Index)])
+
#define FFS_F= ILE_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))<= br class=3D"">
#define FFS_FILE2_SIZE(FfsFileHeaderPtr) \
    ((UINT32) (((EFI_FFS_FILE_HEADER2 *) (UI= NTN)
FfsFileHeaderPtr)->ExtendedSize))

typedef UINT8 EFI_SECTION_TYPE;

///<= br class=3D"">/// 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 t= o 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



--Boundary_(ID_5lJv2j52fmnaN6Q8X3SNqg)--