Re: [PATCH] RFC: Provide a function to reset IFUNC PLTs

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 09/03/23 08:32, Jan Kratochvil wrote:
> On Wed, 08 Mar 2023 21:04:59 +0800, Adhemerval Zanella Netto wrote:
>> Yes, without a stop-the-world scheme where a helper thread sets PR_GET_DUMPABLE 
>> and PTRACE_ATTACH the process can not really be sure that any new thread will not 
>> be created between the time you enumerate the process threads and call the 'freeze'
>> function.
> 
> That "Freezer" class does solve the race of new threads. I also do not see
> a need for ptrace there, it is self-snapshotting/restoring.
> 	https://github.com/openjdk/crac/pull/41/files#diff-aeec57d804d56002f26a85359fc4ac8b48cfc249d57c656a30a63fc6bf3457adR6029

I am not sure how the kernel would enumerate new tasks that are created
while iterating over /proc/self/task.  On closefrom Linux fallback we have
a similar problem, where the code iterates over /proc/self/fd, and everytime
it closes a file descriptor it lseeks back to beginning.  It works because
eventually there will be no more entries on /proc/self/fd, so either you
will need to certify that kernel adds new tasks at the end of the getdents
call (used by readdir, or lseek and keep track of all tasks already signaled.

While it might work on the JVM where you can not fully control who change 
SIGUSR1  disposition (and I am not sure JVM would prevent a JNI call to do so),
so you can't really make it generic without explicit reserve a signal to do so, 
similar to what glibc does for SIGCANCEL and SIGSETXID (used to synchronize 
setuid functions over threads).  Meaning that callers of sigaction can't 
not explicit set such reserved signal.

This is similar to what we do for SIGSETXID, so I think a proper way to
do it would to do always install a new signal handler to this on pthread_create,
on signal handle synchronize with proper async-signal-safe interface
(pthread_mutex_lock is not, you might accomplish with sem_post but most likely
you will need a atomic+futex way similar to a barrier), iterate over all 
dl_stack_used (so the interface can work without access to procfs), issue the 
signal handler or each thread, operate on the maps, then synchronize to resume
threads.  We can't really make it generic without accessing the internal
glibc thread states.

And you will also need to reallocate not only glibc, but potentially *all*
libraries (since ifunc can be used by any function).

> 
> 
>> I really don't think glibc should provide an interface to temporary disable any
>> security hardening, it should always opt-in at either program startup or by
>> building time.  The ifunc mechanism is already full or corner cases and I think
>> adding a runtime mechanism to reset them is *not* a way forward.  
>>
>> As I said, I think CRIU heterogeneity should be handled by masking off the higher 
>> cpu features.  I am not if ARCH_SET_CPUID would a solution here, it means that
>> we will need to handle SIGSEGV in the loader and come up with a sane subset
>> in case of failure (we now have x86_64-vx, so we can use it as default). 
> 
> So the only remaining option is that all the programs will be doing
> setenv("GLIBC_TUNABLES=glibc.cpu.hwcaps=...") and re-exec(). That is
> a peformance kill and definitely not nice compared to any method of an IFUNC
> reset.

Assuming you don't reset env variable on process spawning, you can set it as 
default for the session. Another option would to deploy a glibc built with 
--disable-multi-arch; it will disable ifunc generation.

And IMHO this is way nicer because this IFUNC reset as-is, without a proper 
stop-the-word support, is not safe and adds another corner case for the already
over-complicated ifunc interface.

> 
> 
>> But as Florian has said, fixing on glibc won't work consistently on other
>> libraries that uses cpuid instruction.
> 
> In Java world the other libraries (in general, there are some JNI exceptions)
> do not matter as they are a Java code JIT-compiled by JVM.

And this won't be a Java specific interface, but rather a GNU extension for C
library.  So we must make it as concise as possible, without adding any other
security or undefined behavior.