From: Laszlo Ersek <lersek@redhat.com>
Date: Thu, 15 Feb 2018 13:47:11 +0000 (+0100)
Subject: MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Sub()
X-Git-Tag: edk2-stable201903~2357
X-Git-Url: https://git.proxmox.com/?p=mirror_edk2.git;a=commitdiff_plain;h=54c7728a04658b09ea1a50b9e35e838fde166003

MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Sub()

The subtraction in the assignment

  SignedResult = Minuend - Subtrahend;

is performed with unchecked INT64 operands. According to ISO C, if the
mathematical result of signed integer subtraction 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 subtraction if it is safe.

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Tested-by: Michael D Kinney <michael.d.kinney@intel.com>
---

diff --git a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
index d846160ba0..8e857927b0 100644
--- a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
+++ b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
@@ -3837,27 +3837,55 @@ SafeInt64Sub (
   )
 {
   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.
+  // * A Subtrahend of zero can never cause underflow or overflow.
+  //
+  // * A positive Subtrahend can only cause underflow. The underflow condition
+  //   is:
+  //
+  //     (Minuend - Subtrahend) < MIN_INT64
+  //
+  //   Adding Subtrahend to both sides yields
+  //
+  //     Minuend < (MIN_INT64 + Subtrahend)
+  //
+  //   This condition can be coded directly in C because the RHS will neither
+  //   underflow nor overflow. That is due to the starting condition:
+  //
+  //     0 < Subtrahend <= MAX_INT64
+  //
+  //   Adding MIN_INT64 to all three sides yields
+  //
+  //     MIN_INT64 < (MIN_INT64 + Subtrahend) <= (MIN_INT64 + MAX_INT64) = -1
   //
-  if (((Minuend < 0) != (Subtrahend < 0)) &&
-      ((Minuend < 0) != (SignedResult < 0))) {
+  // * A negative Subtrahend can only cause overflow. The overflow condition is
+  //
+  //     (Minuend - Subtrahend) > MAX_INT64
+  //
+  //   Adding Subtrahend to both sides yields
+  //
+  //     Minuend > (MAX_INT64 + Subtrahend)
+  //
+  //   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 <= Subtrahend < 0
+  //
+  //   Adding MAX_INT64 to all three sides yields
+  //
+  //     -1 = (MAX_INT64 + MIN_INT64) <= (MAX_INT64 + Subtrahend) < MAX_INT64
+  //
+  if (((Subtrahend > 0) && (Minuend < (MIN_INT64 + Subtrahend))) ||
+      ((Subtrahend < 0) && (Minuend > (MAX_INT64 + Subtrahend)))) {
     *Result = INT64_ERROR;
     Status = RETURN_BUFFER_TOO_SMALL;
   } else {
-    *Result = SignedResult;
+    *Result = Minuend - Subtrahend;
     Status = RETURN_SUCCESS;
   }