From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 18 Apr 2019 02:06:55 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2B6FB3001A70; Thu, 18 Apr 2019 09:06:55 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-179.rdu2.redhat.com [10.10.120.179]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0FCB019C6A; Thu, 18 Apr 2019 09:06:53 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE To: "Kinney, Michael D" , "devel@edk2.groups.io" Cc: "Gao, Liming" References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-5-lersek@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 18 Apr 2019 11:06:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Thu, 18 Apr 2019 09:06:55 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/17/19 20:31, Kinney, Michael D wrote: > Laszlo, > > We should also be able to do a consistent fix without > adding any new unions or macros: > > #define FFS_FILE_SIZE(FfsFileHeaderPtr) ((UINT32)( \ > (((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size[0] ) | \ > (((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size[1] << 8 ) | \ > (((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size[2] << 16 ) )) > > #define SECTION_SIZE(SectionHeaderPtr) ((UINT32)( \ > (((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size[0] ) | \ > (((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size[1] << 8 ) | \ > (((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size[2] << 16 ) )) I'll adopt this (see my other reply), with one small change: I think I'll wrap "SectionHeaderPtr" in an extra set of parens. My main reason for introducing the intermediate macros was that I wanted to cast each UINT8 *individually* to UINT32, before applying the shift: #define FFS_FILE_SIZE_ELEMENT(FfsFileHeaderPtr, Index) \ ((UINT32) FFS_FILE_SIZE_ARRAY (FfsFileHeaderPtr)[(Index)]) #define FFS_FILE_SIZE(FfsFileHeaderPtr) \ ((FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 0) << 0) | \ (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 1) << 8) | \ (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 2) << 16)) - Such casting is generally a good idea because UINT8 is promoted to INT32 as part of the integer promotions, and I consider bit-shifting signed integers bad hygiene (it carries a risk of undefined behavior). - But then I felt that spelling out the same (UINT32) cast on every line of the expanded / flattened version of FFS_FILE_SIZE would make the replacement text simply too wide and hard to read. Hence the helper macros. Now, in this particular case however (i.e. producing 24-bit unsigned integers in the end), you have convinced me that casting to UINT32, as the outermost operation only, is safe enough. Because, we never shift an INT32 to the left by more than 16 bits, and that INT32 (pre-shift) is in [0, UINT8_MAX] inclusive (because it comes from Size[2]). So the result is always representable in an INT32. ... I've now also checked that no invocation of FFS_FILE_SIZE or SECTION_SIZE in edk2 passes an argument with side effects, so evaluating the arguments in the new macros multiple times should be safe. I might add a new comment about this restriction. Thanks! Laszlo