Messages in this thread Patch in this message |  | | From | Xi Wang <> | Subject | [PATCH RFC] exec: avoid possible undefined behavior in count() | Date | Sun, 6 Jan 2013 00:29:05 -0500 |
| |
The tricky problem is this check:
if (i++ >= max)
icc (mis)optimizes this check as:
if (++i > max)
The check now becomes a no-op since max is MAX_ARG_STRINGS (0x7FFFFFFF).
This is "allowed" by the C standard, assuming i++ never overflows, because signed integer overflow is undefined behavior. This optimization effectively reverts the previous commit 362e6663ef ("exec.c, compat.c: fix count(), compat_count() bounds checking") that tries to fix the check.
This patch simply moves ++ after the check.
Signed-off-by: Xi Wang <xi.wang@gmail.com> --- Not sure how many people are using Intel's icc to compiled the kernel. Some projects like LinuxDNA did.
The kernel uses gcc's -fno-strict-overflow to disable this optimization. icc probably doesn't recognize the option.
To illustrate the problem, try this simple program:
int count(int i, int max) { if (i++ >= max) { __builtin_trap(); return -1; } return i; }
#include <stdio.h> #include <stdlib.h>
int main(int argc, char **argv) { int x = atoi(argv[1]); int max = atoi(argv[2]); printf("%d %d %d\n", x, max, count(x, max)); }
$ gcc -O2 t.c $ ./a.out 2147483647 2147483647 Illegal instruction (core dumped)
$ icc -O2 t.c $ ./a.out 2147483647 2147483647 2147483647 2147483647 -2147483648
There's no difference whether we add -fno-strict-overflow or not. --- fs/exec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/exec.c b/fs/exec.c index 18c45ca..20df02c 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -434,8 +434,9 @@ static int count(struct user_arg_ptr argv, int max) if (IS_ERR(p)) return -EFAULT; - if (i++ >= max) + if (i >= max) return -E2BIG; + ++i; if (fatal_signal_pending(current)) return -ERESTARTNOHAND; -- 1.7.10.4
|  |