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; Wed, 17 Apr 2019 07:59:46 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 31A5F307D84D; Wed, 17 Apr 2019 14:59:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-180.rdu2.redhat.com [10.10.123.180]) by smtp.corp.redhat.com (Postfix) with ESMTP id 989C05D704; Wed, 17 Apr 2019 14:59:44 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE To: Andrew Fish Cc: devel@edk2.groups.io, Jordan Justen , Mike Kinney , Liming Gao References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-3-lersek@redhat.com> <155522637661.21857.4743822681286277764@jljusten-skl> <3bbbb85e-5557-d99b-1c3b-50a844455d20@redhat.com> <155540548458.13612.11281694046292591090@jljusten-skl> <413ac018-bcf2-f510-00d0-33315974a3c2@redhat.com> <155544052538.15733.153410443320244157@jljusten-skl> <940201E3-0EDB-40B8-8680-CDE68DA0FD06@apple.com> <71e4f508-75f2-79a7-967e-d7a6a0e34341@redhat.com> From: "Laszlo Ersek" Message-ID: <3a4d9e9b-ffc2-3034-dc20-29b665524b75@redhat.com> Date: Wed, 17 Apr 2019 16:59:41 +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.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Wed, 17 Apr 2019 14:59:46 +0000 (UTC) Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/17/19 13:44, Andrew Fish wrote: > Sorry I digressed into the C specification discussion, and did not > deal with the patch in general. My point is the original code is legal > C code. If you lookup CWE-119 it is written as a restriction on what > the C language allows. > > As I mentioned casting to specific alignment is legal, as is defining > a structure that is pragma pack(1) that can make a UINT32 not be 4 > byte aligned. Thus the cast created a legal UINT32 value. A cast to > *(UINT32 *) is different that a cast to (UINT32 *). The rules you > quote a triggered by the = and not the cast. > > Thus this is undefined behavior in C: > UINT32 *Ub = (UINT32 *)gSection.Sec.Size; > Size = *Ub & 0x00ffffff; > > And this is not Undefined behavior: > UINT32 NotUb = *(UINT32 *)gSection.Sec.Size & 0x00ffffff; I agree the 2nd snippet may not be UB due to alignment violations *alone*. It is still UB due to violating the effective type rules. > I also had a hard time interpreting what C spec was saying, but > talking to the people who write the compiler and ubsan cleared it up > for me. It also makes sense when you think about it. If you tell the > compiler *(UINT32 *) it can know to generate byte reads if the > hardware requires aligned access. If you do a (UINT32 *) that new > pointer no longer carries the information about the alignment > requirement. Thus the *(UINT32 *) cast is like making a packed > structure. Yes, I think I'm clear on how the alignment information is carried around, and when it is lost. In your first example above, due to us forming a separate (standalone) pointer, we lose the alignment information, and then the assignment is undefined due to an alignment violation. While in the second example, the alignment information is not lost, and the assignment is not undefined on an alignment basis *alone*. However: the second assignment is *still* undefined, because it violates the effective type rules. Here's a more direct example for the same: STATIC UINT64 mUint64; int main(void) { UINT16 *Uint16Ptr; Uint16Ptr = (UINT16 *)&mUint64; *Uint16Ptr = 1; return 0; } The assignment to (*Uint16Ptr) is fine from the alignment perspective, but it is nevertheless undefined, because it breaks the effective type rules. Namely, UINT16 (the type used for the access) is not compatible with UINT64 (the effective type of mUint64). Normally, we don't care about this situation in edk2 -- in fact we write loads of code like the above --, but we get away with that only because we force the toolchains to ignore the effective type rules. For GCC in particular, the option is "-fno-strict-aliasing". The only reason I've posted this patch is that "cppcheck" (invoked as a part of "RH covscan") doesn't care about "-fno-strict-aliasing". And while "cppcheck" happens to report the overrun, and not the type punning, the way to remove the warning is to adhere to all the C rules in the expression, even though we have "-fno-strict-aliasing" in place. > I agree the union is a good solution to CWE-119 and it better matches > the alignment requirement in the PI spec. Thank you. I'll wait a bit longer to see if Jordan accepts this 02/10 patch based on the most recent comments, and whether Liming or Mike accepts 04/10. If Jordan remains unconvinced on SECTION_SIZE (in this 02/10 patch), and Liming or Mike are fine with 04/10, I can rework 02/10 to follow 04/10. If Jordan remains unconvinced but Mike or Liming prefers the union, then we have a stalemate and I'll abandon the patch set. Thanks, Laszlo