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 11:12:23 -0700 Received: by mail-wr1-f67.google.com with SMTP id j9so4060337wrn.6 for ; Thu, 18 Apr 2019 11:12:22 -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=LFLIt65OC7vCXbxWqrnrt+C29oiK7Pw/gYBXSzN3iXA=; b=ZDDP5tVMV9Lo7vChkqOQpV1vJEJgsMwAdLKgiRcZfDhg7tvtJX6VPZOcKqeYXpSMkD 0TiojDyQS8TBdOHgougyUtHFeVjpdfUnV26RKuN/zEyEHUQCH7GPbiIb7sb3IRIpXrh6 wGAv/Fz9bmR5pPHXEJCdCTE2BdWciCRP/goVMKpaSZZLqnsxR4r0tsaWOaAJK98nAJkZ wbnrfS4RIlP0wB9QPUEAsRbqAOjhKls8XMap4H1B4vvxthu8FoZhov5zj3FZCe1IGSwb z/YD/656OallnFz122OXcwC0OscteUhpe8/j6h6ID8eEtzLnMAd8exmVcal/hSmuBcdQ Pb0A== X-Gm-Message-State: APjAAAXuR+yivjtNNUh2+B/O7IoR85xDBZCo2KKLqZDKo8QNryD/SDwZ B9nLw6wvu9qtPhmTcxYNIycKVw== X-Google-Smtp-Source: APXvYqysp8mN4zqVWlu4CRjEGAuKya76z2QDLhythsvXn823eaQQR0HYrdrfK07tHvTSksFetXFSeA== X-Received: by 2002:adf:f64d:: with SMTP id x13mr1223118wrp.298.1555611141465; Thu, 18 Apr 2019 11:12:21 -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 h131sm10628140wmh.1.2019.04.18.11.12.20 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Thu, 18 Apr 2019 11:12:20 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE To: "Kinney, Michael D" , "devel@edk2.groups.io" , "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 20:12:20 +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: 8bit On 4/18/19 7:59 PM, Kinney, Michael D wrote: > Philippe, > > Comments below. > > Thanks, > > Mike > >> -----Original Message----- >> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com] >> Sent: Thursday, April 18, 2019 10:20 AM >> To: devel@edk2.groups.io; Kinney, Michael D >> ; lersek@redhat.com >> Cc: Gao, Liming >> Subject: Re: [edk2-devel] [PATCH 04/10] >> MdePkg/PiFirmwareFile: fix undefined behavior in >> FFS_FILE_SIZE >> >> 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) >> ); >> } >> > > The ARM/AARCH64 implementation of ReadUnaligned16() just > does the byte access which will always work. So not need > to do the 2 modes you suggest above. I should have check that first ;) Thanks for correcting me! > > UINT16 > EFIAPI > ReadUnaligned16 ( > IN CONST UINT16 *Buffer > ) > { > volatile UINT8 LowerByte; > volatile UINT8 HigherByte; > > ASSERT (Buffer != NULL); > > LowerByte = ((UINT8*)Buffer)[0]; > HigherByte = ((UINT8*)Buffer)[1]; > > return (UINT16)(LowerByte | (HigherByte << 8)); > } > >>> 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