2022-09-02 10:57:31

by Bibo Mao

[permalink] [raw]
Subject: [PATCH v3] LoongArch: Add safer signal handler for TLS access

LoongArch uses general purpose register R2 as thread pointer(TP)
register, signal hanlder also uses TP register to access variables
in TLS area, such as errno and variable in TLS.

If GPR R2 is modified with wrong value, signal handler still uses
the wrong TP register, so signal hanlder is insafe to access TLS
variable.

This patch adds one arch specific syscall set_thread_area, and
restore previoud TP value before signal handler, so that signal
handler is safe to access TLS variable.

It passes to run with the following test case.
=======8<======
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <signal.h>
#include <pthread.h>
#include <asm/ucontext.h>
#include <asm/sigcontext.h>

#define ILL_INSN ".word 0x000001f0"
static inline long test_sigill(unsigned long fid)
{
register long ret __asm__("$r4");
register unsigned long fun __asm__("$r4") = fid;

__asm__ __volatile__("move $r2, $r0 \r\n");
__asm__ __volatile__(
ILL_INSN
: "=r" (ret)
: "r" (fun)
: "memory"
);

return ret;
}

static void set_sigill_handler(void (*fn) (int, siginfo_t *, void *))
{
struct sigaction sa;
memset(&sa, 0, sizeof(struct sigaction));

sa.sa_sigaction = fn;
sa.sa_flags = SA_SIGINFO;
sigemptyset(&sa.sa_mask);
if (sigaction(SIGILL, &sa, 0) != 0) {
perror("sigaction");
}
}

void catch_sig(int sig, siginfo_t *si, void *vuc)
{
struct ucontext *uc = vuc;
register unsigned long tls __asm__("$r2");

uc->uc_mcontext.sc_pc +=4;
uc->uc_mcontext.sc_regs[2] = tls;
printf("catched signal %d\n", sig);
}

void *print_message_function( void *ptr )
{
char *message;
message = (char *) ptr;
printf("%s \n", message);
test_sigill(1);
}

void pthread_test(void)
{
pthread_t thread1, thread2;
char *message1 = "Thread 1";
char *message2 = "Thread 2";
int iret1, iret2;

iret1 = pthread_create( &thread1, NULL, print_message_function,
(void*) message1);
iret2 = pthread_create( &thread2, NULL, print_message_function,
(void*) message2);
pthread_join( thread1, NULL);
pthread_join( thread2, NULL);
printf("Thread 1 returns: %d\n",iret1);
printf("Thread 2 returns: %d\n",iret2);
exit(0);
}

void exec_test(void) {
test_sigill(1);
}

void main() {
register unsigned long tls __asm__("$r2");
int val;

val = syscall(244, tls);
set_sigill_handler(&catch_sig);
pthread_test();
//exec_test();
return;
}
=======8<======

Signed-off-by: Bibo Mao <[email protected]>
---
v2->v3:
- Use current_thread_info rather than task_thread_info(current)
v1->v2:
- Clear TP value in clone function if CLONE_SETTLS is not set
---
arch/loongarch/include/asm/unistd.h | 1 +
arch/loongarch/include/uapi/asm/unistd.h | 2 ++
arch/loongarch/kernel/process.c | 10 +++++++++-
arch/loongarch/kernel/signal.c | 5 +++++
arch/loongarch/kernel/syscall.c | 9 +++++++++
5 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/loongarch/include/asm/unistd.h b/arch/loongarch/include/asm/unistd.h
index cfddb0116a8c..1581624f0115 100644
--- a/arch/loongarch/include/asm/unistd.h
+++ b/arch/loongarch/include/asm/unistd.h
@@ -9,3 +9,4 @@
#include <uapi/asm/unistd.h>

#define NR_syscalls (__NR_syscalls)
+__SYSCALL(__NR_set_thread_area, sys_set_thread_area)
diff --git a/arch/loongarch/include/uapi/asm/unistd.h b/arch/loongarch/include/uapi/asm/unistd.h
index b344b1f91715..ceb8c4cec505 100644
--- a/arch/loongarch/include/uapi/asm/unistd.h
+++ b/arch/loongarch/include/uapi/asm/unistd.h
@@ -4,3 +4,5 @@
#define __ARCH_WANT_SYS_CLONE3

#include <asm-generic/unistd.h>
+
+#define __NR_set_thread_area (__NR_arch_specific_syscall + 0)
diff --git a/arch/loongarch/kernel/process.c b/arch/loongarch/kernel/process.c
index 660492f064e7..488bbe2e8c99 100644
--- a/arch/loongarch/kernel/process.c
+++ b/arch/loongarch/kernel/process.c
@@ -88,6 +88,7 @@ void start_thread(struct pt_regs *regs, unsigned long pc, unsigned long sp)
clear_used_math();
regs->csr_era = pc;
regs->regs[3] = sp;
+ current_thread_info()->tp_value = 0;
}

void exit_thread(struct task_struct *tsk)
@@ -152,6 +153,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
childregs->csr_crmd = p->thread.csr_crmd;
childregs->csr_prmd = p->thread.csr_prmd;
childregs->csr_ecfg = p->thread.csr_ecfg;
+ task_thread_info(p)->tp_value = 0;
return 0;
}

@@ -176,8 +178,14 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
clear_tsk_thread_flag(p, TIF_LSX_CTX_LIVE);
clear_tsk_thread_flag(p, TIF_LASX_CTX_LIVE);

- if (clone_flags & CLONE_SETTLS)
+ /*
+ * record tls val for cloned threads
+ * clear it for forked process
+ */
+ if (clone_flags & CLONE_SETTLS) {
childregs->regs[2] = tls;
+ task_thread_info(p)->tp_value = tls;
+ }

return 0;
}
diff --git a/arch/loongarch/kernel/signal.c b/arch/loongarch/kernel/signal.c
index 7f4889df4a17..2d88630e2a05 100644
--- a/arch/loongarch/kernel/signal.c
+++ b/arch/loongarch/kernel/signal.c
@@ -480,6 +480,9 @@ static int setup_rt_frame(void *sig_return, struct ksignal *ksig,
*
* c0_era point to the signal handler, $r3 (sp) points to
* the struct rt_sigframe.
+ *
+ * Use recorded TP to signal handler if exists since $r2 may be
+ * corrupted already.
*/
regs->regs[4] = ksig->sig;
regs->regs[5] = (unsigned long) &frame->rs_info;
@@ -487,6 +490,8 @@ static int setup_rt_frame(void *sig_return, struct ksignal *ksig,
regs->regs[3] = (unsigned long) frame;
regs->regs[1] = (unsigned long) sig_return;
regs->csr_era = (unsigned long) ksig->ka.sa.sa_handler;
+ if (current_thread_info()->tp_value)
+ regs->regs[2] = current_thread_info()->tp_value;

DEBUGP("SIG deliver (%s:%d): sp=0x%p pc=0x%lx ra=0x%lx\n",
current->comm, current->pid,
diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c
index 3fc4211db989..330d2aeadc02 100644
--- a/arch/loongarch/kernel/syscall.c
+++ b/arch/loongarch/kernel/syscall.c
@@ -29,6 +29,15 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len, unsigned long,
return ksys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
}

+SYSCALL_DEFINE1(set_thread_area, unsigned long, addr)
+{
+ struct pt_regs *regs = current_pt_regs();
+
+ regs->regs[2] = addr;
+ current_thread_info()->tp_value = addr;
+ return 0;
+}
+
void *sys_call_table[__NR_syscalls] = {
[0 ... __NR_syscalls - 1] = sys_ni_syscall,
#include <asm/unistd.h>
--
2.31.1


2022-09-08 16:29:35

by WANG Xuerui

[permalink] [raw]
Subject: Re: [PATCH v3] LoongArch: Add safer signal handler for TLS access

Hi,

On 9/2/22 17:59, Mao Bibo wrote:
> LoongArch uses general purpose register R2 as thread pointer(TP)
> register, signal hanlder also uses TP register to access variables
> in TLS area, such as errno and variable in TLS.
>
> If GPR R2 is modified with wrong value, signal handler still uses
> the wrong TP register, so signal hanlder is insafe to access TLS
> variable.
>
> This patch adds one arch specific syscall set_thread_area, and
> restore previoud TP value before signal handler, so that signal
> handler is safe to access TLS variable.
>
> It passes to run with the following test case.
> =======8<======
> #define _GNU_SOURCE
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <string.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <signal.h>
> #include <pthread.h>
> #include <asm/ucontext.h>
> #include <asm/sigcontext.h>
>
> #define ILL_INSN ".word 0x000001f0"
> static inline long test_sigill(unsigned long fid)
> {
> register long ret __asm__("$r4");
> register unsigned long fun __asm__("$r4") = fid;
>
> __asm__ __volatile__("move $r2, $r0 \r\n");
> __asm__ __volatile__(
> ILL_INSN
> : "=r" (ret)
> : "r" (fun)
> : "memory"
> );
>
> return ret;
> }
>
> static void set_sigill_handler(void (*fn) (int, siginfo_t *, void *))
> {
> struct sigaction sa;
> memset(&sa, 0, sizeof(struct sigaction));
>
> sa.sa_sigaction = fn;
> sa.sa_flags = SA_SIGINFO;
> sigemptyset(&sa.sa_mask);
> if (sigaction(SIGILL, &sa, 0) != 0) {
> perror("sigaction");
> }
> }
>
> void catch_sig(int sig, siginfo_t *si, void *vuc)
> {
> struct ucontext *uc = vuc;
> register unsigned long tls __asm__("$r2");
>
> uc->uc_mcontext.sc_pc +=4;
> uc->uc_mcontext.sc_regs[2] = tls;
> printf("catched signal %d\n", sig);
> }
>
> void *print_message_function( void *ptr )
> {
> char *message;
> message = (char *) ptr;
> printf("%s \n", message);
> test_sigill(1);
> }
>
> void pthread_test(void)
> {
> pthread_t thread1, thread2;
> char *message1 = "Thread 1";
> char *message2 = "Thread 2";
> int iret1, iret2;
>
> iret1 = pthread_create( &thread1, NULL, print_message_function,
> (void*) message1);
> iret2 = pthread_create( &thread2, NULL, print_message_function,
> (void*) message2);
> pthread_join( thread1, NULL);
> pthread_join( thread2, NULL);
> printf("Thread 1 returns: %d\n",iret1);
> printf("Thread 2 returns: %d\n",iret2);
> exit(0);
> }
>
> void exec_test(void) {
> test_sigill(1);
> }
>
> void main() {
> register unsigned long tls __asm__("$r2");
> int val;
>
> val = syscall(244, tls);
> set_sigill_handler(&catch_sig);
> pthread_test();
> //exec_test();
> return;
> }
> =======8<======
>
> Signed-off-by: Bibo Mao <[email protected]>
> ---
> v2->v3:
> - Use current_thread_info rather than task_thread_info(current)
> v1->v2:
> - Clear TP value in clone function if CLONE_SETTLS is not set
> ---
> arch/loongarch/include/asm/unistd.h | 1 +
> arch/loongarch/include/uapi/asm/unistd.h | 2 ++
> arch/loongarch/kernel/process.c | 10 +++++++++-
> arch/loongarch/kernel/signal.c | 5 +++++
> arch/loongarch/kernel/syscall.c | 9 +++++++++
> 5 files changed, 26 insertions(+), 1 deletion(-)

So here we're trying to accommodate for ABI violations from user-mode
programs, no matter whether they come from fuzzing, or any other black
magic; but being user-space, theoretically set_thread_area could also be
fuzzed, and this "additional" layer of defense then breaks down. And if
this theoretical case is deemed uninteresting, remember the case you're
demonstrating is also found by fuzzing and not found in normal working
user programs, then the whole scenario should be theoretical and
uninteresting as well...

BTW I've also thrown this problem into a riscv discussion group, and
they pointed out that e.g. sigaltstack is already available for
guaranteeing at least some working state when a signal comes. And riscv
also doesn't have set_thread_area which is most likely why loongarch
isn't getting one; further, looking at manpages of
{get,set}_thread_area, it may be the case that both riscv and loongarch
have the TP in user-visible ISA state, so a syscall is not needed for
manipulating it anyway. Again it's equally easy to mess the hidden state
of set_thread_area as $tp itself. Or put it differently, if
set_thread_area is necessary/good for loongarch, you should probably add
it to riscv as well -- both will benefit.

--
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/