Re: [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time.

[Nix <nix at esperi dot org dot uk>]

On 24 Jun 2016, Florian Weimer said:

> On 06/07/2016 01:06 PM, Nix wrote:
>> From: Nick Alcock <nick.alcock@oracle.com>
>>
>> This one is a bit nasty.  Now that we are initializing TLS earlier for
>> the stack canary's sake, existing memcpy() implementations become
>> problematic.  We can use the multiarch implementations, but they might
>> not always be present, and even if they are present they might not always
>> be in assembler, so might be compiled with stack-protection.  We cannot
>> use posix/memcpy.c without marking both it and */wordcopy.c as non-stack-
>> protected, which for memcpy() of all things seems like a seriously bad
>> idea: if any function in glibc should be stack-protected, it's memcpy()
>> (though stack-protecting the many optimized assembly versions is not done
>> in this patch series).
>
> Some of the pathnames in this explanation appear to be incorrect.

Oh, it's string/memcpy.c, isn't it? Sorry.

> I looked at the memcpy.c and wordcopy.c implementations, and I do not
> see why these functions would need the stack protector. They should
> not have addressable local variables, they should all be in registers.

Now we're depending on the details of compiler register allocation for
security? What if the compiler changes?

> As a result, the implementation should never derive a pointer from the
> current stack frame.

Oh, they almost certainly don't, but my attitude here is that I'm less
smart than the attackers and I should just try to protect everything
possible (that doesn't require horrific pain like ld.so would).

> These functions might be passed pointers into the callers stack frame,

That was my worry. I've had exactly that sort of obviously-problematic
bug in the past, as have we all...

> but the memory looks like this (addresses increase from bottom to top,
> so the stack grows downwards on most architectures (except HPPA)):

You see, there are "except"s creeping in already :) every one is a
possible mistake...

>     :                             :
>     +-----------------------------+
>     |  Caller stack frame         |
>     |                             |
>     |  (end of buf)               |
>     |    :                        |
>     |    :                        |
>     |  char buf[64];              |
>     |                             |
>     +-----------------------------+
>     | Return address              |
>     | memcpy stack frame          |
>     |                             |
>     +-(current top of stack)------+
>     :                             :
>     +=============================+
>     | XXX guard page XXXXXXXXXXXX |
>     :                             :
>
>
> As a result, a buffer overflow will run away from the return address associated with the memcpy activation frame.  To avoid that,

Hm, good point. The caller would need stack-protection, but it's got it.

Do we ever see crashes inside memcpy() from return address overwrite?
You're right, I'm not sure I've ever seen one. (But then, I don't use
HP-PA.)

> Anyway, if the above analysis is right, it should be safe to disable
> stack protector for functions such as memcpy (essentially doing
> manually what -fstack-protector-strong does automatically). This would
> be my preferred approach here.

... which would mean we could drop this patch, too.

-- 
NULL && (void)