From: Laszlo Ersek <lersek@redhat.com>
To: Bret Barkelew <Bret.Barkelew@microsoft.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
Sean Brogan <sean.brogan@microsoft.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"Gao, Liming" <liming.gao@intel.com>
Subject: Re: [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance
Date: Tue, 13 Feb 2018 18:29:45 +0100 [thread overview]
Message-ID: <753b0fa9-cd72-ae78-d810-4ee39f4535a5@redhat.com> (raw)
In-Reply-To: <DM5PR2101MB093655672D809EC195855A4BEFF60@DM5PR2101MB0936.namprd21.prod.outlook.com>
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
next prev parent reply other threads:[~2018-02-13 17:23 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-19 19:36 [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance Kinney, Michael D
2018-01-18 8:09 ` Sean Brogan
2018-01-19 7:48 ` Gao, Liming
2018-02-08 0:32 ` Laszlo Ersek
2018-02-08 0:45 ` Laszlo Ersek
2018-02-13 12:23 ` Laszlo Ersek
2018-02-13 16:17 ` Kinney, Michael D
2018-02-13 16:56 ` Bret Barkelew
2018-02-13 17:15 ` Andrew Fish
2018-02-13 17:55 ` Laszlo Ersek
2018-02-13 17:29 ` Laszlo Ersek [this message]
2018-02-13 18:19 ` Bret Barkelew
2018-02-13 17:18 ` Ard Biesheuvel
2018-02-13 17:51 ` Laszlo Ersek
2018-02-13 17:55 ` Ard Biesheuvel
2018-02-13 18:03 ` Laszlo Ersek
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=753b0fa9-cd72-ae78-d810-4ee39f4535a5@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