Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <eba7bae5-8c9b-0b06-137b-4bcbbd9d8923@loongson.cn>
Date:   Thu, 1 Sep 2022 11:02:56 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH v2] LoongArch: Add safer signal handler for TLS access
Content-Language: en-US
To:     WANG Xuerui <kernel@xen0n.name>,
        Huacai Chen <chenhuacai@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Christian Brauner <brauner@kernel.org>
Cc:     Jiaxun Yang <jiaxun.yang@flygoat.com>, loongarch@lists.linux.dev,
        linux-kernel@vger.kernel.org
References: <20220831074846.3565542-1-maobibo@loongson.cn>
 <e805e3c4-ea66-a438-9def-bc90bbffa14e@xen0n.name>
From:   maobibo <maobibo@loongson.cn>
In-Reply-To: <e805e3c4-ea66-a438-9def-bc90bbffa14e@xen0n.name>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 2022/9/1 10:30, WANG Xuerui 写道:
> On 2022/8/31 15:48, 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<======
> 
> Sorry, but what is the concrete use case you have in mind? arch/riscv for example doesn't have set_thread_area but is apparently doing fine. I haven't studied their code in great detail to find out myself though.

We prepare to add loongarch support to risu project, which injects random instructions in user space. The website is:
   https://git.linaro.org/people/peter.maydell/risu.git

After injected instruction is executed, there will be special illegal instruction where signal handler is used to control to capture the whole registers and continue to run. However if random injected instruction modify TP register, signal handler fails to run.

I double that riscv has the same issue.

regards
bibo,mao