gcc-plugins updates for v4.13-rc1

Hi,

Please pull these gcc-plugins changes for v4.13-rc1. The big part is the randstruct plugin infrastructure. This is the first of two expected pull requests for randstruct since there are dependencies in other trees that would be easier to merge once those have landed. Notably, the IPC allocation refactoring in -mm, and many trivial merge conflicts across several trees when applying the __randomize_layout annotation. As a result, it seemed like I should send this now since it is relatively self-contained, and once the rest of the trees have landed, send the annotation patches. I'm expecting the final phase of randstruct (automatic struct selection) will land for v4.14, but if its other tree dependencies actually make it for v4.13, I can send that merge request too.

Thanks!

-Kees

The following changes since commit 6f7da290413ba713f0cdd9ff1a2a9bb129ef4f6c:

Linux 4.12 (2017-07-02 16:07:02 -0700)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/gcc-plugins-v4.13-rc1

for you to fetch changes up to d1185a8c5dd21182012e6dd531b00fd72f4d30cb:

Merge branch 'merge/randstruct' into for-next/gcc-plugins (2017-07-04 21:41:31 -0700)

----------------------------------------------------------------
GCC plugin updates:
- typo fix in Kconfig (Jean Delvare)
- randstruct infrastructure

----------------------------------------------------------------
Arnd Bergmann (1):
ARM: Prepare for randomized task_struct

Jean Delvare (1):
Fix English in description of GCC_PLUGIN_STRUCTLEAK

Kees Cook (9):
gcc-plugins: Detail c-common.h location for GCC 4.6
compiler: Add __designated_init annotation
gcc-plugins: Add the randstruct plugin
randstruct: Whitelist struct security_hook_heads cast
randstruct: Whitelist UNIXCB cast
randstruct: Whitelist big_key path struct overloading
randstruct: Whitelist NIU struct page overloading
Merge branch 'for-next/gcc-plugin-infrastructure' into merge/randstruct Merge branch 'merge/randstruct' into for-next/gcc-plugins

 Documentation/dontdiff                        |    2 +
 arch/Kconfig                                  |   41 +-
 arch/arm/include/asm/assembler.h              |    2 +
 arch/arm/kernel/entry-armv.S                  |    5 +-
 arch/arm/mm/proc-macros.S                     |   10 +-
 include/linux/compiler-gcc.h                  |   13 +
 include/linux/compiler.h                      |   12 +
 include/linux/vermagic.h                      |    9 +-
 scripts/Makefile.gcc-plugins                  |    4 +
 scripts/gcc-plugins/.gitignore                |    1 +
 scripts/gcc-plugins/Makefile                  |    8 +
 scripts/gcc-plugins/gcc-common.h              |   12 +
 scripts/gcc-plugins/gen-random-seed.sh        |    8 +
 scripts/gcc-plugins/randomize_layout_plugin.c | 1028 +++++++++++++++++++++++++
 14 files changed, 1146 insertions(+), 9 deletions(-)
 create mode 100644 scripts/gcc-plugins/.gitignore
 create mode 100644 scripts/gcc-plugins/gen-random-seed.sh
 create mode 100644 scripts/gcc-plugins/randomize_layout_plugin.c

Kees Cook
Pixel Security

Hmm. Completely unrelated comment, and this may not be a gcc 'plugin' issue as much as a more general gcc question, but I suspect a plugin could do it.

For the kernel, we already really ignore some of the more idiotic C standard rules that introduce pointless undefined behavior: things like the strict aliasing rules are just insane, and the "overflow is udnefined" is bad too. So we use

-fno-strict-aliasing
-fno-strict-overflow
-fno-delete-null-pointer-checks

to basically say "those optimizations are fundamentally stupid and wrong, and only encourage compilers to generate random code that doesn't actually match the source code".

And I suspect one other undefined behavior is the one we _try_ to warn about, but where the compiler is not always good enough to give valid warnings - uninitialized automatic variables.

Maybe we could have gcc just always initialize variables to zero. Not just static ones, but the automatic variables too. And maybe it wouldn't generate much extra code, since gcc will see the real initialization, and the extra hardening against random behavior will just go away - so this might be one of those cheap things where we just avoid undefined behavior and avoid leaking old stack contents.

Yes, yes, you'd still have the uninitialized variable warning, but that doesn't catch the case where you pass a structure pointer to a helper that is *supposed* to fill it in, but misses a field or just misses padding.

And maybe I'm wrong, and maybe it would generate a lot of really bad extra zeroing and wouldn't be acceptable for most people, but I *think* this might be one of those things where we might get some extra belt and suspenders kind of hardening basically for free..

Comments?

Linus

At the language level, I would be surprised if the compiler exploits this as undefined behaviour, i.e., that it would assume a convenient [for the compiler] fixed value for a variable in cases where it can prove that it has not been initialised. And at the logical level, zero may not always be a suitable default, this highly depends on the particular code sequence.

So while I think it may be useful for robustness, to avoid erratic behavior or exploitable interactions between different parts of the code, my estimation is that it wouldn't make a great deal of difference, given that the logic that allows the compiler to 'see the real initialization' is the same logic that warns us if it is lacking, and so in a warning free build, no init sequences should have been emitted to begin with.

So the issue I think would be good to fix is perhaps best explained by pseudo-code

  int testfn(struct somestruct __user *p)
  {
        struct somestruct a;

        initialize_struct(&a);
        if (copy_to_user(p, &a, sizeof(a)))
                return -EFAULT;
        return 0;

}

which is obviously made-up code, but is not actually entirely unrealistic.

It's fairly common code in various ioctl-like functions, but also in things like the "stat()" system call etc. The thing that initializes a variable is not necessarily visible, and gcc can not warn about the fact that "initialize_struct()" doesn't actually initialize all fields.

Or even if it does initialize all the fields, what about the padding bytes? That doesn't matter in most normal C programs, since by definition the padding bytes aren't used, but for the kernel, it *does* matter when they get copied outside the kernel.

Linus

This particular example should be handled by
scripts/gcc-plugins/structleak_plugin.c, right?

Arnd

Only if struct somestruct _contains_ a __user pointer. I would love to see this logic expanded, of course. :)

-Kees

Kees Cook
Pixel Security

.. probably. But we have a ton of other uses that just pass in "result" pointers (not structs), which admittedly don't have the padding issue, but do have the exact same issue otherwise.

We have those random "initialize to zero by hand", and I wouldn't actually worry about most of the common cases. KASAN will find them anyway.

It tends to be the random odd ioctl-like things that nobody finds because it's only uninitialized for some silly error case that never triggers (or some unusual driver that needs to be loaded).

Linus

So it seems to me that what sets your example apart is that the address of an automatic variable is taken and passed to a function whose implementation may live in another compilation unit. So this goes beyond any inferences the compiler makes from the possible code flow about undefined behavior etc.

The compiler already keeps track of which auto variables have their address taken, so it shouldn't be /that/ hard to come up with a plugin that zero initializes such variables before their address is taken if no such initialization is included in the code.

Yeah. Except one of the issues with the plugin stuff is that people probably don't do any of this normally.

I suspect it would be a really nice *general* gcc extension..

Linus

2017-07-06 0:56 GMT+03:00 Linus Torvalds <[email protected]>:

KASAN doesn't find "use-of-unitialized memory" bugs. It can find only use-after-free
and out-of-bounds accesses.

MemorySanitizer (aka KMSAN) is supposed to detect uses of unitialized memory. It's still in WIP stage, but have some trophies already (just grep for KMSAN in git log)

It sounds useful to me, at least as a compile-time option. My first thought was to move it under CONFIG_UBSAN, which has related undefined-behavior checks. I see that we are disabling the -Wmaybe-uninitialized warning with UBSAN at the moment to shut up lots of warnings introduced by UBSAN, and that could mean one of two things: either gcc gets a lot worse at tracing the state of variables with UBSAN, or UBSAN causes it to warn about anything that it can't prove to be initialized rather than making some reasonable assumptions about calls to external functions.

If the latter is true, it might be enough to just initialize the ones we would warn about with UBSAN.

a) the most common one should be those that gcc can prove to be initialized at the first use. Adding a zero-initialization would be pointless here.
b) Those that gcc can prove to be used uninitialized, and it warns about with -Wuninitialized. This sounds like something that -fsanitize=undefined should handle, but I could not find any information about it actually doing that.
c) The ones that require knowledge of multiple translation units or functions it decided not to inline, so gcc intentionally doesn't warn about them (unlike smatch).
d) The ones that gcc cannot prove to be in any of the above categories (see: halting problem) and warns about with -Wmaybe-uninitialized

The last two seem like candidates for implicit zero-initialization, while for b) one could argue that this should be treated like some other undefined behavior and just trap (e.g. gcc now turns code paths it knows to cause divide-by-zero into a single trapping instruction and skips the math leading up to it). Or you could argue that gcc shouldn't do that for other undefined behavior either.

Arnd

It is, unfortunately, not free. :( There has been a lot of academic research[1] into finding ways to minimize the impact, but given some of the Linux maintainers refusing even zeroing of APIs that pass stack-based structures[2]. Another thing that has been worked on is porting the stackleak gcc plugin from grsecurity to upstream[3]. This does effective clearing of the stack, but it takes a more holistic approach (and for added fun, it does alloca probes too). Like some of the more comprehensive academic attempts, it sees about a 4% hit (but it's doing more...)

I'd love to get the stackleak plugin into upstream (and the work is on-going), but having something try a "lighter" form of this in a gcc plugin would be interesting to experiment with.

-Kees

[1] e.g. https://www.internetsociety.org/sites/default/files/ndss2017_09-2_Lu_paper.pdf
performs only uninitialized on-stack pointer zeroing, and
http://www.cs.vu.nl/~giuffrida/papers/safeinit-ndss-2017.pdf shows <5%
performance hit with optimization for initializing everything
[2] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=getsockname&id=a4467f966f0c70fd232388c05798a84276eef1ef
[3] http://openwall.com/lists/kernel-hardening/2017/06/09/14

Kees Cook
Pixel Security