[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5j+GQ5feC=bL4-PKWV9p3e9CeAKKGMfgdwk1=FfnYLMviw@mail.gmail.com>
Date: Tue, 17 Jan 2017 09:56:24 -0800
From: Kees Cook <keescook@...omium.org>
To: Mark Rutland <mark.rutland@....com>
Cc: "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, PaX Team <pageexec@...email.hu>,
Emese Revfy <re.emese@...il.com>, "AKASHI, Takahiro" <takahiro.akashi@...aro.org>,
park jinbum <jinb.park7@...il.com>, Daniel Micay <danielmicay@...il.com>,
LKML <linux-kernel@...r.kernel.org>, Dave Martin <dave.martin@....com>
Subject: Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
On Mon, Jan 16, 2017 at 3:54 AM, Mark Rutland <mark.rutland@....com> wrote:
> Hi,
>
> [adding Dave, so retaining full context below]
>
> On Fri, Jan 13, 2017 at 02:02:56PM -0800, Kees Cook wrote:
>> This plugin detects any structures that contain __user attributes and
>> makes sure it is being fulling initialized so that a specific class of
>
> Nit: s/fulling/fully/
Whoops, thanks, fixed.
>> information exposure is eliminated. (For example, the exposure of siginfo
>> in CVE-2013-2141 would have been blocked by this plugin.)
>>
>> Ported from grsecurity/PaX. This version adds a verbose option to the
>> plugin and the Kconfig.
>>
>> Signed-off-by: Kees Cook <keescook@...omium.org>
>> ---
>> arch/Kconfig | 22 +++
>> include/linux/compiler.h | 6 +-
>> scripts/Makefile.gcc-plugins | 4 +
>> scripts/gcc-plugins/structleak_plugin.c | 246 ++++++++++++++++++++++++++++++++
>> 4 files changed, 277 insertions(+), 1 deletion(-)
>> create mode 100644 scripts/gcc-plugins/structleak_plugin.c
>
> I tried giving this a go, but I got the build failure below:
>
> ----
> [mark@...erpostej:~/src/linux]% uselinaro 15.08 make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-
> arch/arm64/Makefile:43: Detected assembler with broken .inst; disassembly will be unreliable
> CHK include/config/kernel.release
> CHK include/generated/uapi/linux/version.h
> CHK include/generated/utsrelease.h
> UPD include/generated/utsrelease.h
> HOSTCXX -fPIC scripts/gcc-plugins/structleak_plugin.o
> scripts/gcc-plugins/structleak_plugin.c: In function ‘int plugin_init(plugin_name_args*, plugin_gcc_version*)’:
> scripts/gcc-plugins/structleak_plugin.c:214:12: error: ‘structleak’ was not declared in this scope
> PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
> ^
Sorry, yes, this depends on the gcc-plugins changes in -next.
> [...]
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index 99839c23d453..f1250ba0b06f 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -410,6 +410,28 @@ config GCC_PLUGIN_LATENT_ENTROPY
>> * https://grsecurity.net/
>> * https://pax.grsecurity.net/
>>
>> +config GCC_PLUGIN_STRUCTLEAK
>> + bool "Force initialization of variables containing userspace addresses"
>> + depends on GCC_PLUGINS
>> + help
>> + This plugin zero-initializes any structures that containing a
>> + __user attribute. This can prevent some classes of information
>> + exposures.
>> +
>> + This plugin was ported from grsecurity/PaX. More information at:
>> + * https://grsecurity.net/
>> + * https://pax.grsecurity.net/
>> +
>> +config GCC_PLUGIN_STRUCTLEAK_VERBOSE
>> + bool "Report initialized variables"
>
> It might be better to say "Report forcefully initialized variables", to make it
> clear that this is only reporting initialization performed by the plugin.
Sounds good, changed.
> [...]
>> + /* these aren't the 0days you're looking for */
>> + if (verbose)
>> + inform(DECL_SOURCE_LOCATION(var),
>> + "userspace variable will be forcibly initialized");
>> +
>> + /* build the initializer expression */
>> + initializer = build_constructor(TREE_TYPE(var), NULL);
>> +
>> + /* build the initializer stmt */
>> + init_stmt = gimple_build_assign(var, initializer);
>> + gsi = gsi_after_labels(single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
>> + gsi_insert_before(&gsi, init_stmt, GSI_NEW_STMT);
>> + update_stmt(init_stmt);
>
> I assume that this is only guaranteed to initialise fields in a struct,
> and not padding, is that correct? I ask due to the issue described in:
>
> https://lwn.net/Articles/417989/
>
> As a further step, it would be interesting to see if we could fix
> padding for __user variables, but that certainly shouldn't hold this
> back.
This spawned a whole thread. :) For non-C11, yeah, a plugin to do this
would be nice. That's already on the KSPP TODO list, but from what I
can tell it either requires walking every variable's structure to
check for sizes and padding or do explicitly add a constructor for
every variable and hope that the optimization phase does the right
thing. ;)
>> +}
>> +
>> +static unsigned int structleak_execute(void)
>> +{
>> + basic_block bb;
>> + unsigned int ret = 0;
>> + tree var;
>> + unsigned int i;
>> +
>> + /* split the first bb where we can put the forced initializers */
>> + gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
>> + bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
>> + if (!single_pred_p(bb)) {
>> + split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
>> + gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
>> + }
>> +
>> + /* enumarate all local variables and forcibly initialize our targets */
>
> Nit: s/enumarate/enumerate/
Fixed.
Thanks for the review!
-Kees
--
Kees Cook
Nexus Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
