Hi Thomas,
On Fri, May 04, 2018 at 05:52:57AM +0100, Thomas Preudhomme wrote:
> >> As mentionned in the ticket this was my first thought but this means
> >> making the pattern aware of all the possible way the address could be
> >> access (PIC Vs non-PIC, Arm Vs Thumb-2 Vs Thumb-1) to decide how many
> >> scratch registers are needed. I'd rather reuse the existing pattern as
> >> much as possible to make sure they are well tested. Ideally I wanted a
> >> way to mark a REG RTX so that it is never spilled and such that the
> >> mark is propagated when the register is moved to another register or
> >> propagated. But that is a bigger change so decided it should be an
> >> improvement for later but needed another solution right now.
> >
> > How would that work, esp. for pseudos? If too many regs have such a
> > mark then the compiler will have to sorry() or similar, not a good
> > thing at all.
>
> I'm missing something, there should be the same amount of pseudo with that
> mark as there is scratch in the new pattern doing memory address load(s) +
> set / check. I'm guessing this is not as easy to achieve as it sounds.
But this pattern is expanded all the way at the beginning of the RTL
pipeline, so you'll need to prevent anything copying this. And if any
other pattern wants to use this do-not-spill feature as well, you'll
have a problem no matter what.
> >> By the way about making sure the address is not left in a register, I
> >> have a question regarding the current stack_protect_set and
> >> stack_protect_check pattern and their requirements to have register
> >> cleared afterwards: why is that necessary? Currently not all registers
> >> are cleared and the guard is available in the canari before it is
> >> overwritten anyway so I don't see how clearing the register adds any
> >> extra security. What sort of attack is it protecting against?
> >
> > From md.texi:
> >
> > @item @samp{stack_protect_set}
> > This pattern, if defined, moves a @code{ptr_mode} value from the memory
> > in operand 1 to the memory in operand 0 without leaving the value in
> > a register afterward. This is to avoid leaking the value some place
> > that an attacker might use to rewrite the stack guard slot after
> > having clobbered it.
> >
> > (etc.)
>
> I've read that doc but what I don't understand is why the guard value being
> leaked in a register would be a problem if modified. The pattern as they
> are guarantee the guard is always reloaded from its canonical location
> (e.g. TLS var). Because the patterns do not represent in RTL what they do
> the compiler could not reuse the value left in a register. Are we worrying
> about optimization the assembler could do?
>
> > Having the canary in a global variable makes it a lot easier for exploit
> > code to access it then if it is e.g. in TLS data. Actually leaking a
> > pointer to it would make it extra easy...
>
> If an attacker can execute code to access and modify the guard, why would
> s/he bother doing a stack overflow instead of just executing the code he
> wants to directly?
The issue is leaking the value so the user can observe it, and then when
overwriting the stack write the expected value to the cookie again, so
that the protection isn't triggered.
You don't necessarily need to execute code of your choice to overwrite
a memory location of your choice, fwiw. SSP does not prevent all attacks,
just very many.
Segher
