linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave P Martin <Dave.Martin@arm.com>
To: PaX Team <pageexec@freemail.hu>
Cc: Kees Cook <keescook@chromium.org>,
	Mark Rutland <mark.rutland@arm.com>,
	<kernel-hardening@lists.openwall.com>,
	Emese Revfy <re.emese@gmail.com>,
	"AKASHI, Takahiro" <takahiro.akashi@linaro.org>,
	park jinbum <jinb.park7@gmail.com>,
	Daniel Micay <danielmicay@gmail.com>,
	<linux-kernel@vger.kernel.org>, <spender@grsecurity.net>
Subject: Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
Date: Tue, 17 Jan 2017 22:04:20 +0000	[thread overview]
Message-ID: <20170117220420.GQ18838@e103592.cambridge.arm.com> (raw)
In-Reply-To: <587E6FBD.21621.DC486DE@pageexec.freemail.hu>

On Tue, Jan 17, 2017 at 08:25:49PM +0100, PaX Team wrote:
> On 17 Jan 2017 at 18:07, Dave P Martin wrote:
>
> > On Tue, Jan 17, 2017 at 06:09:49PM +0100, PaX Team wrote:
> > > On 17 Jan 2017 at 10:42, Dave P Martin wrote:
> > >
> > > > This can be read with the interpretation you suggest, but the wording
> > > > doesn't seem rock-solid.  For the kernel, I guess it's sufficient if
> > > > GCC commits to this interpretation though.
> > >
> > > note that i'm not a language lawyer, merely an interpreter, so take
> > > the above with a grain of salt.
> >
> > Me neither, but actually I think this interpretation doesn't make sense.
> > The implication seems to be that padding initialisation of initialised
> > aggregates with automatic storage is guaranteed _only_ if the
> > initialiser is incomplete.
>
> however illogical it sounds, that's actually what is implied by the standard
> and gcc behaves this way. my test case with a bit more verbose output than
> your example (gcc shows in comments what each assignment corresponds to):
>
> ------ cut --------
> gcc -O2 -x c -S -fverbose-asm -o - - <<EOF
> struct s1 {
>         char c;
>         int l;
> };
>
> void f(struct s1*);
>
> void g(void)
> {
>         struct s1 s1 = {};
>         struct s1 s2 = { .c = 1};
>         struct s1 s3 = { .l = 2};
>         struct s1 s4 = { .c = 3, .l = 4};
>
>         f(&s1);
>         f(&s2);
>         f(&s3);
>         f(&s4);
> }
> EOF
> ------ cut --------
>
> > This may or may not be a bug, and may or may not have been fixed in a
> > more recent version.
>
> i tested 4.5.4, 6.3 and 7.0 from about a month ago on amd64 and had
> consistent results as above. i think it's not a bug but an intentional
> optimization but i don't feel like digging it out from the gcc sources :).

Interesting.  So I guess we're probably stuck with it, good or bad.

Do you know the rationale behind the inconsistency?

> > > both examples would be handled by the plugin if the types involved already
> > > have a __user annotated field (as neither foo2 nor tv are initialized, only
> > > assigned to), otherwise it'd need manual annotation and/or more static analysis.
> >
> > Do you see any false positives here?  I would expect a fair number of
> > structures that contain __user pointers but that are never copied
> > wholesale to userspace, but I've not reviewed in detail.
>
> false positive depends on what you consider a goal ;). for me the structleak
> plugin had to serve one specific purpose (initialize a local variable that
> would otherwise leak unintended kernel stack content back to userland), everything
> else didn't matter. now if you want to move the goalpost and initialize those
> and only those variables that get copied to userland but aren't otherwise
> guaranteed to be completely initialized then you'll need something much smarter
> than the current structleak plugin as it doesn't serve that purpose and will
> both overinitialize and fail to initialize affected variables...

Sure -- the hope was that structleak might cover this sort of thing
without too much extra effort, but it sounds less realistic now.

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

  reply	other threads:[~2017-01-17 22:04 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 [this message]
2017-01-17 17:56   ` Kees Cook

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=20170117220420.GQ18838@e103592.cambridge.arm.com \
    --to=dave.martin@arm.com \
    --cc=danielmicay@gmail.com \
    --cc=jinb.park7@gmail.com \
    --cc=keescook@chromium.org \
    --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=spender@grsecurity.net \
    --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).