From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 56E3821CF1CEA for ; Tue, 13 Feb 2018 09:23:57 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 836E240FB646; Tue, 13 Feb 2018 17:29:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-17.rdu2.redhat.com [10.10.120.17]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6C744946A9; Tue, 13 Feb 2018 17:29:46 +0000 (UTC) To: Bret Barkelew , "Kinney, Michael D" , Sean Brogan Cc: "edk2-devel@lists.01.org" , "Gao, Liming" References: <20171219193625.16060-1-michael.d.kinney@intel.com> <656eb64b-3265-f021-ff4f-df2ed6b7c752@redhat.com> <868954f3-f368-073c-9e62-d11440e719c9@redhat.com> From: Laszlo Ersek Message-ID: <753b0fa9-cd72-ae78-d810-4ee39f4535a5@redhat.com> Date: Tue, 13 Feb 2018 18:29:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 13 Feb 2018 17:29:47 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 13 Feb 2018 17:29:47 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Feb 2018 17:23:58 -0000 Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/13/18 17:56, Bret Barkelew wrote: > In response to the original question, I would content that our goal > should be "a". We should be allowing universal detection of errors > without the caller having to carry this detection code itself. OK. The question is how the detection is implemented internally. Is that implementation (in edk2) allowed to rely on behavior that the ISO C standard leaves undefined, and -- consequently -- compilers might exploit for code generation? > The analog would be the safe string functions: if a buffer overflow > occurs, they don't find a way to "fix" the operation, but they > faithfully report an error. Precisely. Correspondingly, translated to the safe string functions, my question becomes: Is the implementation of the safe string functions allowed to employ buffer overflow internally, and detect the overflow after the fact? > As such, I believe from my review that these functions work as > intended. Please let me quote the function again, from "MdePkg/Library/BaseSafeIntLib/SafeIntLib.c": 3831 RETURN_STATUS 3832 EFIAPI 3833 SafeInt64Sub ( 3834 IN INT64 Minuend, 3835 IN INT64 Subtrahend, 3836 OUT INT64 *Result 3837 ) 3838 { 3839 RETURN_STATUS Status; 3840 INT64 SignedResult; 3841 3842 if (Result == NULL) { 3843 return RETURN_INVALID_PARAMETER; 3844 } 3845 3846 SignedResult = Minuend - Subtrahend; 3847 3848 // 3849 // Subtracting a positive number from a positive number never overflows. 3850 // Subtracting a negative number from a negative number never overflows. 3851 // If you subtract a negative number from a positive number, you expect a positive result. 3852 // If you subtract a positive number from a negative number, you expect a negative result. 3853 // Overflow if inputs vary in sign and the output does not have the same sign as the first input. 3854 // 3855 if (((Minuend < 0) != (Subtrahend < 0)) && 3856 ((Minuend < 0) != (SignedResult < 0))) { 3857 *Result = INT64_ERROR; 3858 Status = RETURN_BUFFER_TOO_SMALL; 3859 } else { 3860 *Result = SignedResult; 3861 Status = RETURN_SUCCESS; 3862 } 3863 3864 return Status; 3865 } On line 3846, integer overflow is possible. If that happens, not only is the resultant value of "SignedResult" undefined, but the behavior of the rest of the function is undefined, according to ISO C. In other words, just because we move the checking into a library function, we cannot check *after the fact*. The subtraction on line 3846 can invoke undefined behavior *first*, and we check only afterwards, starting on line 3855. Here's an analogy. Various C compilers regularly equate "undefined behavior" with "never happens" (which is a valid interpretation of the ISO C standard for the compiler to make). For example, given the code int f(int *x) { *x = 3; if (x == NULL) { return 1; } return 0; } the compiler may compile f() to unconditionally return 0, such as: int f(int *x) { *x = 3; return 0; } Because, the (x == NULL) branch would depend on undefined behavior invoked by the assignment to *x. Given that the (*x = 3) assignment is undefined for (x==NULL), according to ISO C, the subsequent (x == NULL) check can be taken as constant false, and eliminated. Similarly, a sufficiently smart compiler may assume that the subtraction on line 3846 will never overflow. (Because, such an overflow would be undefined behavior.) Consequently, it may deduce that the overflow checks, *after the fact*, evaluate to constant false, and can be eliminated. It might compile the function as in: { RETURN_STATUS Status; INT64 SignedResult; if (Result == NULL) { return RETURN_INVALID_PARAMETER; } SignedResult = Minuend - Subtrahend; *Result = SignedResult; Status = RETURN_SUCCESS; return Status; } I apoligize if I'm unclear; I really don't know how to put it better. The subtraction on line 3846 runs the risk of undefined behavior, and the checks starting on line 3855 are too late. Thanks Laszlo