From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.85.221.67, mailfrom: philmd@redhat.com) Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by groups.io with SMTP; Thu, 18 Apr 2019 10:20:06 -0700 Received: by mail-wr1-f67.google.com with SMTP id w18so3847421wrv.11 for ; Thu, 18 Apr 2019 10:20:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=NpzPZlW9TK90Y98nboD/7F4jGMiygM6JiBYWMqqkuGs=; b=BKcu1kxN817tyi/JONEBEv2PQyXPx7vP9J5EKkOeWZa20qwfQHK56RIp/k2yIcVjk+ WkFxW12VU+qX9SzAvhhIl1lKMw4XnASlPzf7dIXS9lxLh+O9/PUTvgcQA5flxPp1a1yA K0EWIYJtyEkZ53EcuS7H2j8zph/LgFzTK6vmekCMGCQ3weFASkp/QrJEsNq+xZ8GL23a YZnZjUPAYkErD3q9qH7Lc9vJODCGMbCweYit2knb4qK+zaTGQc4M0EQImb0sTEkzQ2Sx LcxdmVUKXsHosxfASmUMbtsKaPfd4SyXtaG6tjBd+hV91QkdamOYi85A4/0DpFwbUlrz PKNg== X-Gm-Message-State: APjAAAVk7ywJ+76OqIWp8kht1SfnRovzgho4kEXnFy27zNfR74C1ri3G KFGlQSTqDoecjww2ZSp8IOfClQ== X-Google-Smtp-Source: APXvYqwuRWFRnybrsPLYiR/1IKDhaTUDtbR8gC8Jy5dAVOo5hjQNsTKU+Yn5XDdnYym4IvoRnbLdzA== X-Received: by 2002:adf:f8c1:: with SMTP id f1mr53956998wrq.151.1555608004673; Thu, 18 Apr 2019 10:20:04 -0700 (PDT) Return-Path: Received: from [192.168.1.33] (193.red-88-21-103.staticip.rima-tde.net. [88.21.103.193]) by smtp.gmail.com with ESMTPSA id u17sm3798359wmu.36.2019.04.18.10.20.03 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Thu, 18 Apr 2019 10:20:04 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE To: devel@edk2.groups.io, michael.d.kinney@intel.com, "lersek@redhat.com" Cc: "Gao, Liming" References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-5-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: Date: Thu, 18 Apr 2019 19:20:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Michael, On 4/17/19 7:52 PM, 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. This ARM implementation assumes Buffer is halfword-aligned OR the microarchitectures supports unaligned halfword access. The 3x 8-bit accesses macro looks simpler than adding a 16-bit alignment check on Buffer, such: if (Buffer & 1) { return (UINT32)( ((UINT8*)Buffer)[0] | (ReadUnaligned16 ((UINT16*)&(((UINT8*)Buffer)[1])) << 8) ); } else { return (UINT32)( ReadUnaligned16 ((UINT16*)Buffer) | (((UINT8*)Buffer)[2] << 16) ); } > 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] >> -=-=-=-=-=-= > > > >