From: Laszlo Ersek <lersek@redhat.com>
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>,
Liming Gao <liming.gao@intel.com>,
Michael D Kinney <michael.d.kinney@intel.com>,
Sean Brogan <sean.brogan@microsoft.com>
Subject: [PATCH 2/4] MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Add()
Date: Thu, 15 Feb 2018 19:36:36 +0100 [thread overview]
Message-ID: <20180215183638.18578-3-lersek@redhat.com> (raw)
In-Reply-To: <20180215183638.18578-1-lersek@redhat.com>
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 <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>
---
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
next prev parent reply other threads:[~2018-02-15 18:30 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-15 18:36 [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult Laszlo Ersek
2018-02-15 18:36 ` [PATCH 1/4] MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Sub() Laszlo Ersek
2018-02-15 18:36 ` Laszlo Ersek [this message]
2018-02-15 18:36 ` [PATCH 3/4] MdePkg/BaseSafeIntLib: clean up parentheses in MIN_INT64_MAGNITUDE Laszlo Ersek
2018-02-15 18:36 ` [PATCH 4/4] MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Mult() Laszlo Ersek
2018-02-16 11:28 ` [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult Ard Biesheuvel
2018-02-16 20:44 ` Laszlo Ersek
2018-02-16 18:11 ` Kinney, Michael D
2018-02-16 20:49 ` Laszlo Ersek
2018-02-17 3:07 ` Kinney, Michael D
2018-02-21 11:00 ` Laszlo Ersek
2018-02-21 18:10 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180215183638.18578-3-lersek@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox