grsecurity forums

Hi,

I'm looking into a "PAX: size overflow" error from ZFSOnLinux:

https://github.com/zfsonlinux/zfs/issues/2505

I have a naive test program demonstrating the problem which bugs out with a "size overflow" when compiled with gcc -O (4.6.3 and 4.7.2), but doesn't error when compiled without optimisation:

Code: Select all

$ gcc -fplugin=linux/tools/gcc/size_overflow_plugin/size_overflow_plugin.so test.c -o test -O 2> /dev/null && ./test
SIZE_OVERFLOW: size overflow detected in function main test.c:25 cicus.8_14 min, count: 2

$ gcc -fplugin=linux/tools/gcc/size_overflow_plugin/size_overflow_plugin.so test.c -o test 2> /dev/null && ./test
exit ok


This is using grsecurity-3.0-3.14.12.

The test program, based on the example in https://forums.grsecurity.net/viewtopic.php?f=7&t=3043

Code: Select all

#include <stdint.h>
#include <stdio.h>

extern void *malloc(size_t size) __attribute__((size_overflow(1)));

void * __attribute__((size_overflow(1))) coolmalloc(size_t size)
{
  return malloc(size);
}

void report_size_overflow(const char *file, unsigned int line, const char *func, const char *ssa_name)
{
  printf("SIZE_OVERFLOW: size overflow detected in function %s %s:%u %s", func, file, line, ssa_name);
  fflush(stdout);
  _exit(1);
}

#define P2ROUNDUP_TYPED(x, align, type) \
  (-(-(type)(x) & -(type)(align)))

int main(int argc, char *argv[])
{
  size_t namesize = strlen(argv[0]);
  namesize = P2ROUNDUP_TYPED(namesize, sizeof (uint64_t), size_t);
  coolmalloc(20 + namesize);
  printf("exit ok\n");
}


Is this my error (first time I've looked at grsecurity), an error in the code, an error in gcc, or an error in grsecurity?


Cheers,

Chris

this is a sort of false positive vs. 'bad' code as it relies on integer overflow to produce the desired results (e.g., the unary minus operator makes no sense on an unsigned type since the resulting negative value cannot be represented in the unsigned type and it's only the C integer conversion rules that make this work but that doesn't make the code any more readable/nice/etc). in situations like this we recommend rewriting the 'smart' code to a form that doesn't rely on overflow.

Aha, I see the issue now, and I understand the benefits of clear code over "smart" code - thanks.

On a technical level, given that C has standards-defined behaviour for negating unsigned quantities (which behaviour this code relies upon), would it be reasonable (albeit perhaps difficult) to have the grsecurity overflow detector deal with unsigned quantities "properly"?

the problem is that by the time a plugin gets to look at the internal representation of the source code it's too late, it can't tell whether the given construct that relies on integer overflow was intentionally used by the programmer or it was something the compiler introduced during canonicalization and other transformations or it's an actual potential error (we'd want to instrument only the latter of these). in other words, if we detected these constructs and excluded them from the runtime instrumentation then we might as well not do anything, unfortunately. note that the size overflow plugin specifically looks for size related computations only, not all integer operations, so we think that it's a reasonable expectation that such size computation not rely on overflow at all and that's why we recommend rewriting the code in cases like this. future code auditors and the plugin will thank you later .

edit: i forgot to mention but one typical problem with this kind of code is that it's not actually safe, in this case it doesn't work for signed types yet there's nothing to enforce this requirement but programmer awareness and we all know how well that tends to work out in practice...

Just to be clear, I accept that the code itself can be seen as problematical for various reasons.

On a technical level I'm interested in why this code in particular triggers the overflow warning. It's my understanding that the warning is about an actual detected overflow rather than a potential overflow. If that's the case...

If the macro is expanded out into explicit assignments we get:

Code: Select all

  size_t namesize = strlen(argv[0]);
  size_t a = -(size_t)namesize;
  size_t b = -(size_t)sizeof(uint64_t);
  size_t c = a & b;
  size_t d = -c;
  size_t e = 20 + d;
  coolmalloc(e);


This still generates an overflow warning at the coolmalloc() line, with no obvious (to me) overflow present.

And oddly... In the test provided, argv[0]="./test", so namesize=6. If I change "namesize = strlen(argv[0])" to "namesize = 6", there's no overflow warning. Similarly, if it's changed to "namesize = strlen("./test")", there's no overflow warning. Or if the "size_t"s are changed to "int"s, there's no overflow warning. Or if "e = 20 + d" is changed to "e = d" there's no overflow warning.

What's actually going on here?

as i hinted at it in my previous post, the key to know what's going on with the source code and consequently what the plugin works with is the internal representation of the compiler, in this case the GIMPLE one will do. compile your code with -fdump-tree-all and look at the resulting files. the nrv one is about the last pass before lowering to RTL and represents the result of all the optimizations in GIMPLE. you'll see then how the compiler does constant propagation and dead code elimination (among others) for some of your cases and how it produces integer overflow based calculations for others. also change your example so that the result of coolmalloc is actually used (say pass it to printf) otherwise DCE will have a field day .

Oh hey, that's pretty cool!

OK, I can see how the various code versions are optimised differently giving different results. At this point I'm not going to delve deeper to see if the "failing" code might be differently optimised to have it not fail, that's too far beyond my current knowledge. (I did port gcc to a new o/s at one point, but gcc was much, much simpler back in the mid 80s!)

Thanks for indulging my interest!

i can think of two similar ways to do this (type casts omitted for readability):

Code: Select all

(x & (align - 1)) ? (x | (align - 1)) + 1 : x

or

Code: Select all

(x & (align - 1)) ? (x + align - (x & (align - 1))) : x

both compile into one more insn at the asm level with a conditional jump inside, so it's not ideal but not that bad either, presumably the memory allocation path takes a whole lot more cycles than what these would add. there's also a branch-less version of this if some higher level logic ensures that x can never be 0 (which would underflow and be caught by the plugin):

Code: Select all

((x - 1) | (align - 1)) + 1

if overallocating when x is already aligned isn't a problem then this simplifies to

Code: Select all

(x | (align - 1)) + 1

and this works even for x=0.