Re: A bug in vrp_meet?

2019-03-06 Thread Richard Biener
On Tue, Mar 5, 2019 at 10:36 PM Jeff Law  wrote:
>
> On 3/5/19 7:44 AM, Richard Biener wrote:
>
> > So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> > the MAX_EXPR introduced by folding makes it somewhat ugly.
> >
> > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> >
> > Any ideas how to make it less so?  I can split out making optimize_stmt
> > take a gsi * btw, in case that's a more obvious change and it makes the
> > patch a little smaller.
> >
> > Richard.
> >
> > 2019-03-05  Richard Biener  
> >
> > PR tree-optimization/89595
> > * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> > stmt iterator as reference, take boolean output parameter to
> > indicate whether the stmt was removed and thus the iterator
> > already advanced.
> > (dom_opt_dom_walker::before_dom_children): Re-iterate over
> > stmts created by folding.
> >
> > * gcc.dg/torture/pr89595.c: New testcase.
> >
>
> Well, all the real logic changs are in the before_dom_children method.
> The bits in optimize_stmt are trivial enough to effectively ignore.
>
> I don't see a better way to discover and process statements that are
> created in the bowels of fold_stmt.

I'm not entirely happy so I created the following alternative which
is a bit larger and slower due to the pre-pass clearing the visited flag
but is IMHO easier to follow.  I guess there's plenty of TLC opportunity
here but then I also hope to retire the VN parts of DOM in favor
of the non-iterating RPO-VN code...

So - I'd lean to this variant even though it has the extra loop over stmts,
would you agree?

Bootstrap / regtest running on x86_64-unknown-linux-gnu.

Richard.

2019-03-06  Richard Biener  

PR tree-optimization/89595
* tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
stmt iterator as reference, take boolean output parameter to
indicate whether the stmt was removed and thus the iterator
already advanced.
(dom_opt_dom_walker::before_dom_children): Re-iterate over
stmts created by folding.

* gcc.dg/torture/pr89595.c: New testcase.


fix-pr89595-2
Description: Binary data


Re: About BZ#87210 [RFE] To initialize automatic stack variables

2019-03-06 Thread David Brown
On 06/03/2019 02:50, Segher Boessenkool wrote:
> On Tue, Mar 05, 2019 at 09:36:56PM +0100, David Brown wrote:
>> On 05/03/2019 19:37, Segher Boessenkool wrote:
>>> On Mon, Mar 04, 2019 at 09:45:37PM +0100, David Brown wrote:
 void foo(void) {
 char key[20];
 strcpy(key, "Top secret");
 usekey(key);
 memset(key, 0, sizeof(key));
 {
 typedef struct { char x[20]; } XS;
 XS *p = (XS *) key;
 asm("" : "+m" (*p));
 }
 }
>>>
>>> You need to use "asm volatile" here.  In principle GCC can see all the
>>> outputs of the asm are dead and delete all of the asm, otherwise.  But
>>> you don't need an output for *p (or inout as you wrote), just an input
>>> is fine.
>>
>> There are no outputs from this asm - only an input.
> 
> "+m" is an output.  There is only one colon before it, and besides, "+"
> is only allowed on outputs.
> 

My apologies - I had copied this from the wrong function in my code.
The "+m" makes this an input and an output - which in fact is good
enough here, since it forces the memset to be completed before the empty
asm code is run.  But it does mean the "output" should be used or it
risks being removed by the optimiser (when the "volatile" is omitted).

The asm line should have been:

asm("" :: "m" (*p));

That makes it an input-only asm, which is automatically volatile.  It
may also aid optimisation in some cases, since the compiler knows the
value of "key" is unchanged after the assembly (unlike after the first
version of the asm).



Re: About BZ#87210 [RFE] To initialize automatic stack variables

2019-03-06 Thread Alexander Potapenko via gcc
On Mon, Mar 4, 2019 at 9:46 PM David Brown  wrote:
>
> On 19/02/2019 11:23, P J P wrote:
> > Hello,
> >-> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87210
> >
> > This RFE is about providing gcc option(s) to eliminate information leakage
> > issues from programs. Information leakage via uninitialised memory has 
> > beena chronic/recurring issue across all software. They are found quite 
> > often andmay lead to severe effects if found in system software/kernel, OR 
> > an applicationwhich handles sensitive information.
> > Various projects/efforts are underway to keep such information exposurefrom 
> > happening
> > * STACKLEAK - http://lkml.iu.edu/hypermail/linux/kernel/1810.3/00522.html
> > * KLEAK - https://netbsd.org/gallery/presentations/maxv/kleak.pdf* 
> > https://j00ru.vexillium.org/papers/2018/bochspwn_reloaded.pdf
> > But these are still external corrections to improve specific project 
> > and/orsoftware. It does not help to fix/eliminate all information leakage 
> > issues.
> > Automatic memory initialisation:
> >
> > * https://lists.llvm.org/pipermail/cfe-dev/2018-November/060172.html
> > * https://reviews.llvm.org/D54604
> > It'd be immensely helpful and welcome if gcc(1) could provide 
> > compile/buildtime options to enable/disable - automatic memory 
> > initialisation.
> > Could we please consider it as more viable/useful option?
> > Thank you.---
> >-P J P
> > http://feedmug.com
> >
>
> This strikes me as getting the issue completely backwards.
>
> It is not lack of initialisation of stack variables that leads to
> information leakage - it is a failure to clear the important data left
> behind on the stack.
Well, yes and no.
If we consider only a handful of variables containing important data,
we can probably afford to annotate them to be cleared upon leaving the
function using an attribute you're suggesting below.
(Although humans are really bad at annotating code - such attributes
go out of sync in no time).
But e.g. in the Linux kernel even a leak of a single word or a pointer
can help malicious users to defeat ASLR, stack canaries etc.
Now, if we choose to clear _every_ local variable when we leave the
function, we'll have to memset the whole stack frame, which is costly
(as you mention, we don't want the memset to be optimized away).
At the same time, memsetting locals upon function entry gives us the
same result, but has a bigger optimization potential (most of the time
one won't have to pre-initialize anything, thanks to DSE).

> The problem with information leakage is when you have something like:
>
> void foo(void) {
> char key[20];
> strcpy(key, "Top secret!");
> usekey(&key);
> memset(key, 0, sizeof(key));// optimised away by DSE
> }
>
> void bar(void) {
> foo();
> char stolen_key[40];// Covering "key" and other stack
> steal_key(&stolen_key);
> }
>
>
> Forcing "stolen_key" to be zero initialised does not help anyone -
> options for that just make code slower and hide errors that would occur
> with other compiler options.  The challenge is to make sure /key/ is
> zeroed out after use - no matter what optimisations, and whether or not
> the "memset" is called.
>
> gcc already has mechanisms for handling this.
>
> First, there is a way to tell gcc that something in memory will be read,
> even though it doesn't look like it:
>
> void foo(void) {
>  char key[20];
>  strcpy(key, "Top secret");
>  usekey(key);
>  memset(key, 0, sizeof(key));
>  {
>  typedef struct { char x[20]; } XS;
>  XS *p = (XS *) key;
>  asm("" : "+m" (*p));
>  }
> }
>
> Next, to automate the clearing of the key regardless of how and when the
> function "foo" is exited and whether or not "memset" is called, we can
> use the "cleanup" attribute:
>
> static void clearKey(char (*key)[20]) {
>  memset(key, 0, 20);
>  {
>  typedef struct { char x[20]; } XS;
>  XS *p = (XS *) key;
>  asm("" : "+m" (*p));
>  }
> }
>
> void foo2(void) {
>  char key[20] __attribute__((cleanup(clearKey)));
>  strcpy(key, "Top secret");
>  usekey(key);
> }
>
> This stops information leakage where it should be stopped - once the
> information is no longer used.  Forcing initialisation of stack
> variables would put it in the wrong place, when the stack space is reused.
>
> And the code generated here is as good as it gets - no efficiency is lost.
>
>
> So as far as I can see, gcc has all the bits it needs - it just needs a
> nicer and simpler syntax.  Something like an attribute "secure" which
> will generate and use an appropriately sized zeroing function on scope
> exit.  Ideally, this could be attached to a type as well as a variable.
>
> (I have no idea how simple or difficult this task might be.)
>
>


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergerich