linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "PaX Team" <pageexec@freemail.hu>
To: Dave P Martin <Dave.Martin@arm.com>
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 20:25:49 +0100	[thread overview]
Message-ID: <587E6FBD.21621.DC486DE@pageexec.freemail.hu> (raw)
In-Reply-To: <20170117180746.GN18838@e103592.cambridge.arm.com>

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 :).

> > 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...

  reply	other threads:[~2017-01-17 19:26 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 [this message]
2017-01-17 22:04             ` Dave P Martin
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=587E6FBD.21621.DC486DE@pageexec.freemail.hu \
    --to=pageexec@freemail.hu \
    --cc=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=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).