From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 AA99B222DE122 for ; Wed, 7 Feb 2018 16:39:31 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7D155C036744; Thu, 8 Feb 2018 00:45:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-123.phx2.redhat.com [10.3.117.123]) by smtp.corp.redhat.com (Postfix) with ESMTP id 756D45D737; Thu, 8 Feb 2018 00:45:14 +0000 (UTC) From: Laszlo Ersek To: "Kinney, Michael D" , edk2-devel@lists.01.org Cc: Liming Gao References: <20171219193625.16060-1-michael.d.kinney@intel.com> <656eb64b-3265-f021-ff4f-df2ed6b7c752@redhat.com> Message-ID: Date: Thu, 8 Feb 2018 01:45:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <656eb64b-3265-f021-ff4f-df2ed6b7c752@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 08 Feb 2018 00:45:15 +0000 (UTC) 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: Thu, 08 Feb 2018 00:39:32 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/08/18 01:32, Laszlo Ersek wrote: > On 12/19/17 20:36, Kinney, Michael D wrote: >> From: Sean Brogan >> >> SafeIntLib provides helper functions to prevent integer overflow >> during type conversion, addition, subtraction, and multiplication. > > I clearly cannot review such a huge patch, but I've noticed something > and would like to ask for clarification: > >> +/** >> + INT64 Subtraction >> + >> + Performs the requested operation using the input parameters into a value >> + specified by Result type and stores the converted value into the caller >> + allocated output buffer specified by Result. The caller must pass in a >> + Result buffer that is at least as large as the Result type. >> + >> + If Result is NULL, RETURN_INVALID_PARAMETER is returned. >> + >> + If the requested operation results in an overflow or an underflow condition, >> + then Result is set to INT64_ERROR and RETURN_BUFFER_TOO_SMALL is returned. >> + >> + @param[in] Minuend A number from which another is to be subtracted. >> + @param[in] Subtrahend A number to be subtracted from another >> + @param[out] Result Pointer to the result of subtraction >> + >> + @retval RETURN_SUCCESS Successful subtraction >> + @retval RETURN_BUFFER_TOO_SMALL Underflow >> + @retval RETURN_INVALID_PARAMETER Result is NULL >> +**/ >> +RETURN_STATUS >> +EFIAPI >> +SafeInt64Sub ( >> + IN INT64 Minuend, >> + IN INT64 Subtrahend, >> + OUT INT64 *Result >> + ) >> +{ >> + RETURN_STATUS Status; >> + INT64 SignedResult; >> + >> + if (Result == NULL) { >> + return RETURN_INVALID_PARAMETER; >> + } >> + >> + SignedResult = Minuend - Subtrahend; >> + >> + // >> + // Subtracting a positive number from a positive number never overflows. >> + // Subtracting a negative number from a negative number never overflows. >> + // If you subtract a negative number from a positive number, you expect a positive result. >> + // If you subtract a positive number from a negative number, you expect a negative result. >> + // Overflow if inputs vary in sign and the output does not have the same sign as the first input. >> + // >> + if (((Minuend < 0) != (Subtrahend < 0)) && >> + ((Minuend < 0) != (SignedResult < 0))) { >> + *Result = INT64_ERROR; >> + Status = RETURN_BUFFER_TOO_SMALL; >> + } else { >> + *Result = SignedResult; >> + Status = RETURN_SUCCESS; >> + } >> + >> + return Status; >> +} > > Is our goal to > > (a) catch overflows before the caller goes wrong due to them, or > (b) completely prevent undefined behavior from happening, even inside > SafeInt64Sub()? > > The above implementation may be good for (a), but it's not correct for > (b). The > > SignedResult = Minuend - Subtrahend; > > subtraction invokes undefined behavior if the result cannot be > represented [*]; the rest of the code cannot help. > > Now if we say that such subtractions always occur according to the > "usual two's complement definition", on all architectures that edk2 > targets, and we're sure that no compiler or analysis tool will flag -- > or exploit -- the UB either, then the code is fine; meaning our choice > is (a). > > But, if (b) is our goal, I would replace the current error condition with: > > (((Subtrahend > 0) && (Minuend < MIN_INT64 + Subtrahend)) || > ((Subtrahend < 0) && (Minuend > MAX_INT64 + Subtrahend))) To clarify, I wouldn't just replace the error condition. In addition to that, I would remove the SignedResult helper variable (together with the current subtraction), and calculate & assign *Result = Minuend - Subtrahend; only after the error condition fails (i.e. the subtraction is safe). Thanks, Laszlo > Justification: > > * Subtrahend==0 can never cause overflow > > * Subtrahend>0 can only cause overflow at the negative end, so check > that: (Minuend - Subtrahend < MIN_INT64), mathematically speaking. > In order to write that down in C, add Subtrahend (a positive value) > to both sides, yielding (Minuend < MIN_INT64 + Subtrahend). Here, > (MIN_INT64 + Subtrahend) will never go out of range, because > Subtrahend is positive, and (MIN_INT64 + MAX_INT64) is representable. > > * Subtrahend<0 can only cause overflow at the positive end, so check > that: (Minuend - Subtrahend > MAX_INT64), mathematically speaking. > In order to write that down in C, add Subtrahend (a negative value) > to both sides, yielding (Minuend > MAX_INT64 + Subtrahend). Here, > (MAX_INT64 + Subtrahend) will never go out of range, because > Subtrahend is negative, and (MAX_INT64 + MIN_INT64) is representable. > > ( > > [*] ISO C99 section 6.5 Expressions, p5: "If an exceptional condition > occurs during the evaluation of an expression (that is, if the result is > not mathematically defined or not in the range of representable values > for its type), the behavior is undefined." > > Section 6.2.5 Types, p9 only exempts unsigned integers, "A computation > involving unsigned operands can never overflow, because a result that > cannot be represented by the resulting unsigned integer type is reduced > modulo the number that is one greater than the largest value that can be > represented by the resulting type." > > Note that this is different from conversion, where the computation first > succeeds (= we have a value), and then the value is converted to a type > that causes truncation: section 6.3.1.3 Signed and unsigned integers, > p3: "Otherwise, the new type is signed and the value cannot be > represented in it; either the result is implementation-defined or an > implementation-defined signal is raised." > > In the code above, the expression (Minuend - Subtrahend) can invoke > undefined behavior, there is no conversion (not even as part of the > assignment to SignedResult). > > ) > > Thanks, > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >