From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: michael.d.kinney@intel.com) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by groups.io with SMTP; Wed, 17 Apr 2019 10:52:28 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Apr 2019 10:52:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,362,1549958400"; d="scan'208";a="136641302" Received: from orsmsx107.amr.corp.intel.com ([10.22.240.5]) by orsmga006.jf.intel.com with ESMTP; 17 Apr 2019 10:52:27 -0700 Received: from orsmsx152.amr.corp.intel.com (10.22.226.39) by ORSMSX107.amr.corp.intel.com (10.22.240.5) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 17 Apr 2019 10:52:27 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.24]) by ORSMSX152.amr.corp.intel.com ([169.254.8.32]) with mapi id 14.03.0415.000; Wed, 17 Apr 2019 10:52:27 -0700 From: "Michael D Kinney" To: "devel@edk2.groups.io" , "lersek@redhat.com" , "Kinney, Michael D" CC: "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/hQ Date: Wed, 17 Apr 2019 17:52:27 +0000 Message-ID: References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-5-lersek@redhat.com> In-Reply-To: <20190412233128.4756-5-lersek@redhat.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: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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, =20 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 >=20 > 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. >=20 > (We can't use a union here, unfortunately, as easily as > with > "EFI_COMMON_SECTION_HEADER", given the fields in > "EFI_FFS_FILE_HEADER".) >=20 > 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(-) >=20 > 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; >=20 > #define IS_FFS_FILE2(FfsFileHeaderPtr) \ > (((((EFI_FFS_FILE_HEADER *) (UINTN) > FfsFileHeaderPtr)->Attributes) & FFS_ATTRIB_LARGE_FILE) > =3D=3D FFS_ATTRIB_LARGE_FILE) >=20 > +#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)) >=20 > #define FFS_FILE2_SIZE(FfsFileHeaderPtr) \ > ((UINT32) (((EFI_FFS_FILE_HEADER2 *) (UINTN) > FfsFileHeaderPtr)->ExtendedSize)) >=20 > typedef UINT8 EFI_SECTION_TYPE; >=20 > /// > /// Pseudo type. It is used as a wild card when > retrieving sections. > -- > 2.19.1.3.g30247aa5d201 >=20 >=20 >=20 > -=3D-=3D-=3D-=3D-=3D-=3D > Groups.io Links: You receive all messages sent to this > group. >=20 > 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