linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Micay <danielmicay@gmail.com>
To: Mark Rutland <mark.rutland@arm.com>, PaX Team <pageexec@freemail.hu>
Cc: kernel-hardening@lists.openwall.com,
	Kees Cook <keescook@chromium.org>,
	Emese Revfy <re.emese@gmail.com>,
	"AKASHI, Takahiro" <takahiro.akashi@linaro.org>,
	park jinbum <jinb.park7@gmail.com>,
	linux-kernel@vger.kernel.org, spender@grsecurity.net
Subject: Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
Date: Mon, 16 Jan 2017 14:08:17 -0500	[thread overview]
Message-ID: <1484593697.1277.7.camel@gmail.com> (raw)
In-Reply-To: <20170116152425.GG5908@leverpostej>

[-- Attachment #1: Type: text/plain, Size: 3964 bytes --]

On Mon, 2017-01-16 at 15:24 +0000, Mark Rutland wrote:
> Hi,
> 
> On Sat, Jan 14, 2017 at 11:03:14AM +0100, PaX Team wrote:
> > On 13 Jan 2017 at 14:02, Kees Cook wrote:
> > 
> > > +config GCC_PLUGIN_STRUCTLEAK_VERBOSE
> > > +	bool "Report initialized variables"
> > > +	depends on GCC_PLUGIN_STRUCTLEAK
> > > +	depends on !COMPILE_TEST
> > > +	help
> > > +	  This option will cause a warning to be printed each
> > > time the
> > > +	  structleak plugin finds a variable it thinks needs to
> > > be
> > > +	  initialized. Since not all existing initializers are
> > > detected
> > > +	  by the plugin, this can produce false positive
> > > warnings.
> > 
> > there are no false positives, a variable either has a constructor or
> > it does not ;)
> 
> ... or it has no constructor, but is clobbered by a memset before it
> is
> possibly copied. ;)
> 
> For example:
> 
> arch/arm64/kernel/fpsimd.c: In function 'do_fpsimd_exc':
> arch/arm64/kernel/fpsimd.c:106:12: note: userspace variable will be
> forcibly initialized
>   siginfo_t info;
> 
> ... where the code looks like:
> 
> void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs)
> {
> 	siginfo_t info;
> 	unsigned int si_code = 0;
> 
> 	if (esr & FPEXC_IOF)
> 		si_code = FPE_FLTINV;
> 	else if (esr & FPEXC_DZF)
> 		si_code = FPE_FLTDIV;
> 	else if (esr & FPEXC_OFF)
> 		si_code = FPE_FLTOVF;
> 	else if (esr & FPEXC_UFF)
> 		si_code = FPE_FLTUND;
> 	else if (esr & FPEXC_IXF)
> 		si_code = FPE_FLTRES;
> 
> 	memset(&info, 0, sizeof(info));
> 	info.si_signo = SIGFPE;
> 	info.si_code = si_code;
> 	info.si_addr = (void __user *)instruction_pointer(regs);
> 
> 	send_sig_info(SIGFPE, &info, current);
> }
> 
> ... so it's clear to a human that info is initialised prior to use,
> though not by an explicit field initializer.

It's obvious to the compiler too, not just a human. The compiler can
optimize it out when it's so clearly not required, although translation
units will get in the way without LTO. There's existing sophisticated
analysis for automatic initialization with full coverage. That's part of
why I think it makes sense to have a toggle for heuristics vs. full
coverage (incl. non-function-scope locals, that's just a heuristic too).

> > > +/* unused C type flag in all versions 4.5-6 */
> > > +#define TYPE_USERSPACE(TYPE) TYPE_LANG_FLAG_5(TYPE)
> > 
> > FYI, this is a sort of abuse/hack of tree flags and should not be
> > implemented this
> > way in the upstream kernel as it's a finite resource and needs
> > careful verification
> > against all supported gcc versions (these flags are meant for
> > language fronteds, i
> > kinda got lucky to have a few of them unusued but it's not a robust
> > future-proof
> > approach). instead an attribute should be used to mark these types.
> > whether that
> > can/should be __user itself is a good question since that's another
> > hack where the
> > plugin 'hijacks' a sparse address space atttribute (for which gcc
> > 4.6+ has its own
> > facilities and that the checker gcc plugin makes use of thus it's
> > not compatible
> > with structleak as is).
> 
> To me, it seems that the __user annotation can only be an indicator of
> an issue by chance. We have structures with __user pointers in structs
> that will never be copied to userspace, and conversely we have structs
> that don't contain a __user field, but will be copied to userspace.
> 
> Maybe it happens that structs in more complex systems are more likely
> to
> contain some __user pointer. Was that part of the rationale?
> 
> I wonder if there's any analysis we can do of data passing into
> copy_to_user() and friends. I guess we can't follow the data flow
> across
> compilation units, but we might be able to follow it well enough if we
> added a new attribute that described whether data was to be copied to
> userspace.
> 
> Thanks,
> Mark.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 866 bytes --]

  reply	other threads:[~2017-01-16 19:08 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 [this message]
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

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=1484593697.1277.7.camel@gmail.com \
    --to=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).