This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: --enable-stack-protector for glibc, v4, now with arm


On 3 Mar 2016, Adhemerval Zanella outgrape:

> On 03-03-2016 14:34, Nix wrote:
>> OK, if it worked that well not only on 64-bit ARM but also on a platform
>> I never went near, I think we can say all the major holes are covered!
>> (I honestly expected a bit of per-arch protection to be needed on most
>> arches, as was needed on SPARC and x86_64 both -- but none was needed on
>> 32-bit ARM either, so maybe I was being too paranoid.)
>
> I thought too, although based on the patchset the level of arch-specific
> hackery is still limited on some specific implementations.
>
> I see also there are some arch specific implementations that issues
> stack allocation (for instance strcspn for powerpc64) which won't have
> instrumented code.  However I do not see how to accomplish unless it
> it is being explicit added on such implementations.  I do no see this
> as an impeding for patch, but I think it might require some notes in
> documentation.

Yeah, there are plenty of those. All the accelerated assembler
implementations of everything will not get stack-protection, at least
not yet, which includes a great many string functions. But this is a
start, and the remaining pieces can probably be filled in piecemeal by
someone who knows the assembler in question :)

>> I'm not sure which is preferable. I guess if the fortification hooks
>> aren't overrideable, neither should these be.  BUt then, we can't really
>> use __fortify_fail() as a model, because glibc *implements*
>> fortification, so it contains all the calls to __fortify_fail() itself:
>> the same is not true of stack-protection, where random user programs can
>> and will also call __stack_chk_fail().
>
> I am not sure either, although imho I would prefer to not allow user to
> override *internal* GLIBC stack overrun.  So I am inclined in de-PLTify
> internal libc.so usage of __stack_chk_fail().

OK, I'll make that change. (My worry is that people using things like X
servers, where if it dies the whole system might end up more or less
locking up, might want to replace __stack_chk_fail() in order to
possibly switch stacks and return the hardware to a sane state. This is
less relevant now that most X servers don't pound on the hardware
directly, though...)

>> Yeah, that would work. __stack_chk_fail would still be there, and
>> callable by everyone else, and overrideable for users outside of glibc
>> (because __stack_chk_fail() still *has* a PLT). Of course, if any users
>> do override it, they'll see rather inconsistent behaviour, because libc
>> will still use the old one, but libresolv, for example, will use the
>> overridden one! Is that inconsistency something that we should just
>> ignore?
>
> One option is add a private symbol in libc.so, say __stack_chk_fail_glibc,
> and add the alias for other libraries to this symbol.  This won't prevent
> an user to really override with non-default options (like implementing
> the __stack_chk_fail_glibc as well).

Yeah.

> Another option will be to link each library with stack_chk_fail.o and
> pulling a local copy of it and thus preventing overriding. 

I was trying to avoid that sort of duplication. It seemed to be a recipe
for trouble later on -- but it *does* have that one advantage.

I am neutral on this, pick one :)

>> and looking at the assembler in
>> sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S, it is obvious
>> enough even to someone with my extremely minimal x86 asm experience that
>> this thing is not exactly feeling constrained by the normal way one
>> calls functions: the fast unlock path is calling
>> __pthread_mutex_unlock_usercnt() without having at any point adjusted
>> %esp downwards or anything, it does that afterwards on a slow path --
>> essentially the call is overlaid on top of the stack frame of
>> pthread_cond_timedwait() stack frame, like a weird handrolled sibcall
>> which returns conentionally or something. I'm not surprised that adding
>> a canary to something called like *that* makes things explode: I suspect
>> that adding any extra local variables to
>> __pthread_mutex_unlock_usercnt() that weren't optimized out would do the
>> same thing. (If so, there should probably be a warning comment in that
>> function!)
>> 
>> __pthread_mutex_cond_lock_adjust() can also be called in a similar
>> fashion (jumping via labels 1, 2, 5): __pthread_mutex_cond_lock() is
>> also called the same way but the function returns immediately
>> afterwards, and it also gets called more conventionally, so I guess no
>> harm is done there.)
>> 
>> Now if anyone who's actually done more than a trivial amount of x86
>> assembler in the last fifteen years could confirm that, I'd be very
>> happy... since it's quite possible I'm misreading it all. :)
>
> IMHO I would just prefer to get rid of this specific assembly 
> implementation.  In fact there are some patches that do intend to do
> it (new cond var from Tovarlds and my cancellation refactor) and your
> work is another reason to avoid large reimplementation in assembly
> with a very strong performance reason to do so.

I'd guess the strong performance reason was probably to make the
uncontended path blazingly fast (look at it, it's hardly any
instructions at all). Isn't that the whole point of futexes?

Anyway, I'm not ditching the implementation in this patch series!

-- 
NULL && (void)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]