linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: "kernel-hardening@lists.openwall.com" 
	<kernel-hardening@lists.openwall.com>,
	PaX Team <pageexec@freemail.hu>, Emese Revfy <re.emese@gmail.com>,
	"AKASHI, Takahiro" <takahiro.akashi@linaro.org>,
	park jinbum <jinb.park7@gmail.com>,
	Daniel Micay <danielmicay@gmail.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Dave Martin <dave.martin@arm.com>
Subject: Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
Date: Tue, 17 Jan 2017 09:56:24 -0800	[thread overview]
Message-ID: <CAGXu5j+GQ5feC=bL4-PKWV9p3e9CeAKKGMfgdwk1=FfnYLMviw@mail.gmail.com> (raw)
In-Reply-To: <20170116115435.GB5908@leverpostej>

On Mon, Jan 16, 2017 at 3:54 AM, Mark Rutland <mark.rutland@arm.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@chromium.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@leverpostej:~/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

      parent reply	other threads:[~2017-01-17 17:56 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-13 22:02 Kees Cook
2017-01-14 10:03 ` PaX Team
2017-01-16 15:24   ` Mark Rutland
2017-01-16 19:08     ` Daniel Micay
2017-01-16 19:30     ` PaX Team
2017-01-17 17:48       ` Mark Rutland
2017-01-17 18:54         ` PaX Team
2017-01-18 10:48           ` Mark Rutland
2017-01-17 17:48   ` Kees Cook
2017-01-16 11:54 ` Mark Rutland
2017-01-16 12:26   ` [kernel-hardening] " Mark Rutland
2017-01-16 19:22   ` PaX Team
2017-01-17 10:42     ` Dave P Martin
     [not found]       ` <587E4FDD.31940.D47F642@pageexec.freemail.hu>
2017-01-17 18:07         ` Dave P Martin
2017-01-17 19:25           ` PaX Team
2017-01-17 22:04             ` Dave P Martin
2017-01-17 17:56   ` Kees Cook [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGXu5j+GQ5feC=bL4-PKWV9p3e9CeAKKGMfgdwk1=FfnYLMviw@mail.gmail.com' \
    --to=keescook@chromium.org \
    --cc=danielmicay@gmail.com \
    --cc=dave.martin@arm.com \
    --cc=jinb.park7@gmail.com \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pageexec@freemail.hu \
    --cc=re.emese@gmail.com \
    --cc=takahiro.akashi@linaro.org \
    --subject='Re: [PATCH] gcc-plugins: Add structleak for more stack initialization' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).