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 A71F921CF1D09 for ; Thu, 15 Feb 2018 10:30:52 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 24DF08182D27; Thu, 15 Feb 2018 18:36:45 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-84.rdu2.redhat.com [10.10.120.84]) by smtp.corp.redhat.com (Postfix) with ESMTP id 07060200BC16; Thu, 15 Feb 2018 18:36:43 +0000 (UTC) From: Laszlo Ersek To: edk2-devel-01 Cc: Bret Barkelew , Liming Gao , Michael D Kinney , Sean Brogan Date: Thu, 15 Feb 2018 19:36:36 +0100 Message-Id: <20180215183638.18578-3-lersek@redhat.com> In-Reply-To: <20180215183638.18578-1-lersek@redhat.com> References: <20180215183638.18578-1-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 15 Feb 2018 18:36:45 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 15 Feb 2018 18:36:45 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: [PATCH 2/4] MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Add() 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, 15 Feb 2018 18:30:53 -0000 The addition in the assignment SignedResult = Augend + Addend; is performed with unchecked INT64 operands. According to ISO C, if the mathematical result of signed integer addition cannot be represented in the result type, the behavior is undefined. (Refer to ISO C99 6.5p5. 6.2.5p9 only exempts unsigned integers, and 6.3.1.3p3 does not apply because it treats the conversion of integers that have been successfully evaluated first.) Replace the after-the-fact result checking with checks on the operands, and only perform the addition if it is safe. Cc: Bret Barkelew Cc: Liming Gao Cc: Michael D Kinney Cc: Sean Brogan Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek --- MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 56 ++++++++++++++++---- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c index 8e857927b067..56d97cf65601 100644 --- a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c +++ b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c @@ -3631,26 +3631,62 @@ SafeInt64Add ( ) { RETURN_STATUS Status; - INT64 SignedResult; if (Result == NULL) { return RETURN_INVALID_PARAMETER; } - SignedResult = Augend + Addend; - // - // Adding positive to negative never overflows. - // If you add two positive numbers, you expect a positive result. - // If you add two negative numbers, you expect a negative result. - // Overflow if inputs are the same sign and output is not that sign. + // * An Addend of zero can never cause underflow or overflow. // - if (((Augend < 0) == (Addend < 0)) && - ((Augend < 0) != (SignedResult < 0))) { + // * A positive Addend can only cause overflow. The overflow condition is + // + // (Augend + Addend) > MAX_INT64 + // + // Subtracting Addend from both sides yields + // + // Augend > (MAX_INT64 - Addend) + // + // This condition can be coded directly in C because the RHS will neither + // underflow nor overflow. That is due to the starting condition: + // + // 0 < Addend <= MAX_INT64 + // + // Multiplying all three sides by (-1) yields + // + // 0 > (-Addend) >= (-MAX_INT64) + // + // Adding MAX_INT64 to all three sides yields + // + // MAX_INT64 > (MAX_INT64 - Addend) >= 0 + // + // * A negative Addend can only cause underflow. The underflow condition is + // + // (Augend + Addend) < MIN_INT64 + // + // Subtracting Addend from both sides yields + // + // Augend < (MIN_INT64 - Addend) + // + // This condition can be coded directly in C because the RHS will neither + // underflow nor overflow. That is due to the starting condition: + // + // MIN_INT64 <= Addend < 0 + // + // Multiplying all three sides by (-1) yields + // + // (-MIN_INT64) >= (-Addend) > 0 + // + // Adding MIN_INT64 to all three sides yields + // + // 0 >= (MIN_INT64 - Addend) > MIN_INT64 + // + if (((Addend > 0) && (Augend > (MAX_INT64 - Addend))) || + ((Addend < 0) && (Augend < (MIN_INT64 - Addend)))) { *Result = INT64_ERROR; Status = RETURN_BUFFER_TOO_SMALL; } else { - *Result = SignedResult; + *Result = Augend + Addend; Status = RETURN_SUCCESS; } -- 2.14.1.3.gb7cf6e02401b