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
On 5 July 2017 at 20:07, Linus Torvalds <[email protected]> wrote:
> 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.
>
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.
On Wed, Jul 5, 2017 at 9:07 PM, Linus Torvalds
<[email protected]> wrote:
> 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?
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.
>From what I can tell, there are four main cases of local variables:
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
On Wed, Jul 5, 2017 at 1:40 PM, Ard Biesheuvel
<[email protected]> wrote:
>
> 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
On Wed, Jul 5, 2017 at 11:35 PM, Linus Torvalds
<[email protected]> wrote:
> 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.
This particular example should be handled by
scripts/gcc-plugins/structleak_plugin.c, right?
Arnd
On Wed, Jul 5, 2017 at 12:07 PM, Linus Torvalds
<[email protected]> wrote:
> 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?
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
On Wed, Jul 5, 2017 at 2:48 PM, Arnd Bergmann <[email protected]> wrote:
> On Wed, Jul 5, 2017 at 11:35 PM, Linus Torvalds
> <[email protected]> wrote:
>
>> 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.
>
> This particular example should be handled by
> scripts/gcc-plugins/structleak_plugin.c, right?
Only if struct somestruct _contains_ a __user pointer. I would love to
see this logic expanded, of course. :)
-Kees
--
Kees Cook
Pixel Security
On Wed, Jul 5, 2017 at 2:48 PM, Arnd Bergmann <[email protected]> wrote:
>
> This particular example should be handled by
> scripts/gcc-plugins/structleak_plugin.c, right?
.. 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
On 5 July 2017 at 22:56, Linus Torvalds <[email protected]> wrote:
> On Wed, Jul 5, 2017 at 2:48 PM, Arnd Bergmann <[email protected]> wrote:
>>
>> This particular example should be handled by
>> scripts/gcc-plugins/structleak_plugin.c, right?
>
> .. 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).
>
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.
On Wed, Jul 5, 2017 at 3:27 PM, Ard Biesheuvel
<[email protected]> wrote:
>
> 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]>:
> On Wed, Jul 5, 2017 at 2:48 PM, Arnd Bergmann <[email protected]> wrote:
>>
>> This particular example should be handled by
>> scripts/gcc-plugins/structleak_plugin.c, right?
>
> .. 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.
>
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 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