| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABgGipXz77=nyXv9uQ7DKqYH1HkMxxFTbK2qHE202AeXq7Mm-g@mail.gmail.com>
Date: Tue, 11 Jun 2024 13:34:32 +0800
From: Andy Chiu <andy.chiu@...ive.com>
To: Samuel Holland <samuel.holland@...ive.com>
Cc: Palmer Dabbelt <palmer@...belt.com>, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org, Matthew Bystrin <dev.mbstr@...il.com>,
Sami Tolvanen <samitolvanen@...gle.com>
Subject: Re: [PATCH 3/4] riscv: entry: Do not clobber the frame pointer
Hi Samuel,
On Thu, May 30, 2024 at 8:17 AM Samuel Holland
<samuel.holland@...ive.com> wrote:
>
> s0 is reserved for the frame pointer, so it should not be used as a
> temporary register. Clobbering the frame pointer breaks stack traces.
>
> - In handle_exception() and ret_from_exception(), use a2 for the saved
> stack pointer. a2 is chosen because r2 is the stack pointer register.
> - In ret_from_exception(), use s1 for the saved status CSR value. Avoid
> clobbering s1 in the privilege mode check so it does not need to be
> reloaded later in the function.
> - Use s1 and s2 in ret_from_fork() instead of s0 and s1. The entire
> p->thread.s array is zeroed at the beginning of copy_thread(), so the
> registers do not need to be zeroed separately for kernel threads.
>
> Signed-off-by: Samuel Holland <samuel.holland@...ive.com>
> ---
>
> arch/riscv/kernel/entry.S | 29 ++++++++++++++---------------
> arch/riscv/kernel/process.c | 5 ++---
> 2 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index d13d1aad7649..bd1c5621df45 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -58,13 +58,13 @@ SYM_CODE_START(handle_exception)
> */
> li t0, SR_SUM | SR_FS_VS
>
> - REG_L s0, TASK_TI_USER_SP(tp)
> + REG_L a2, TASK_TI_USER_SP(tp)
> csrrc s1, CSR_STATUS, t0
> csrr s2, CSR_EPC
> csrr s3, CSR_TVAL
> csrr s4, CSR_CAUSE
> csrr s5, CSR_SCRATCH
> - REG_S s0, PT_SP(sp)
> + REG_S a2, PT_SP(sp)
> REG_S s1, PT_STATUS(sp)
> REG_S s2, PT_EPC(sp)
> REG_S s3, PT_BADADDR(sp)
> @@ -125,19 +125,19 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
> call riscv_v_context_nesting_end
> #endif
>
> - REG_L s0, PT_STATUS(sp)
> + REG_L s1, PT_STATUS(sp)
> #ifdef CONFIG_RISCV_M_MODE
> /* the MPP value is too large to be used as an immediate arg for addi */
> li t0, SR_MPP
> - and s0, s0, t0
> + and t0, s1, t0
> #else
> - andi s0, s0, SR_SPP
> + andi t0, s1, SR_SPP
> #endif
> - bnez s0, 1f
> + bnez t0, 1f
>
> /* Save unwound kernel stack pointer in thread_info */
> - addi s0, sp, PT_SIZE_ON_STACK
> - REG_S s0, TASK_TI_KERNEL_SP(tp)
> + addi t0, sp, PT_SIZE_ON_STACK
> + REG_S t0, TASK_TI_KERNEL_SP(tp)
>
> /* Save the kernel shadow call stack pointer */
> scs_save_current
> @@ -148,7 +148,6 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
> */
> csrw CSR_SCRATCH, tp
> 1:
> - REG_L a0, PT_STATUS(sp)
> /*
> * The current load reservation is effectively part of the processor's
> * state, in the sense that load reservations cannot be shared between
> @@ -169,7 +168,7 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
> REG_L a2, PT_EPC(sp)
> REG_SC x0, a2, PT_EPC(sp)
>
> - csrw CSR_STATUS, a0
> + csrw CSR_STATUS, s1
> csrw CSR_EPC, a2
>
> REG_L x1, PT_RA(sp)
> @@ -207,13 +206,13 @@ SYM_CODE_START_LOCAL(handle_kernel_stack_overflow)
> REG_S x5, PT_T0(sp)
> save_from_x6_to_x31
>
> - REG_L s0, TASK_TI_KERNEL_SP(tp)
> + REG_L a2, TASK_TI_KERNEL_SP(tp)
> csrr s1, CSR_STATUS
> csrr s2, CSR_EPC
> csrr s3, CSR_TVAL
> csrr s4, CSR_CAUSE
> csrr s5, CSR_SCRATCH
> - REG_S s0, PT_SP(sp)
> + REG_S a2, PT_SP(sp)
> REG_S s1, PT_STATUS(sp)
> REG_S s2, PT_EPC(sp)
> REG_S s3, PT_BADADDR(sp)
> @@ -227,10 +226,10 @@ ASM_NOKPROBE(handle_kernel_stack_overflow)
>
> SYM_CODE_START(ret_from_fork)
> call schedule_tail
> - beqz s0, 1f /* not from kernel thread */
> + beqz s1, 1f /* not from kernel thread */
> /* Call fn(arg) */
> - move a0, s1
> - jalr s0
> + move a0, s2
> + jalr s1
> 1:
> move a0, sp /* pt_regs */
> la ra, ret_from_exception
> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> index e4bc61c4e58a..5512c31e1256 100644
> --- a/arch/riscv/kernel/process.c
> +++ b/arch/riscv/kernel/process.c
> @@ -208,8 +208,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> /* Supervisor/Machine, irqs on: */
> childregs->status = SR_PP | SR_PIE;
>
> - p->thread.s[0] = (unsigned long)args->fn;
> - p->thread.s[1] = (unsigned long)args->fn_arg;
> + p->thread.s[1] = (unsigned long)args->fn;
> + p->thread.s[2] = (unsigned long)args->fn_arg;
> } else {
> *childregs = *(current_pt_regs());
> /* Turn off status.VS */
> @@ -219,7 +219,6 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> if (clone_flags & CLONE_SETTLS)
> childregs->tp = tls;
> childregs->a0 = 0; /* Return value of fork() */
> - p->thread.s[0] = 0;
> }
> p->thread.riscv_v_flags = 0;
> if (has_vector())
> --
> 2.44.1
>
Reviewed-by: Andy Chiu <andy.chiu@...ive.com>
Cheers,
Andy
Powered by blists - more mailing lists