2025-03-14T14:39:24-07:00, Deepak Gupta <de...@rivosinc.com>:
> diff --git a/arch/riscv/include/asm/thread_info.h
> b/arch/riscv/include/asm/thread_info.h
> @@ -62,6 +62,9 @@ struct thread_info {
> long user_sp; /* User stack pointer */
> int cpu;
> unsigned long syscall_work; /* SYSCALL_WORK_ flags */
> +#ifdef CONFIG_RISCV_USER_CFI
> + struct cfi_status user_cfi_state;
> +#endif
I don't think it makes sense to put all the data in thread_info.
kernel_ssp and user_ssp is more than enough and the rest can comfortably
live elsewhere in task_struct.
thread_info is supposed to be as small as possible -- just spanning
multiple cache-lines could be noticeable.
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> @@ -147,6 +147,20 @@ SYM_CODE_START(handle_exception)
>
> REG_L s0, TASK_TI_USER_SP(tp)
> csrrc s1, CSR_STATUS, t0
> + /*
> + * If previous mode was U, capture shadow stack pointer and save it away
> + * Zero CSR_SSP at the same time for sanitization.
> + */
> + ALTERNATIVE("nop; nop; nop; nop",
> + __stringify( \
> + andi s2, s1, SR_SPP; \
> + bnez s2, skip_ssp_save; \
> + csrrw s2, CSR_SSP, x0; \
> + REG_S s2, TASK_TI_USER_SSP(tp); \
> + skip_ssp_save:),
> + 0,
> + RISCV_ISA_EXT_ZICFISS,
> + CONFIG_RISCV_USER_CFI)
(I'd prefer this closer to the user_sp and kernel_sp swap, it's breaking
the flow here. We also already know if we've returned from userspace
or not even without SR_SPP, but reusing the information might tangle
the logic.)
> csrr s2, CSR_EPC
> csrr s3, CSR_TVAL
> csrr s4, CSR_CAUSE
> @@ -236,6 +250,18 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
> csrw CSR_SCRATCH, tp
> +
> + /*
> + * Going back to U mode, restore shadow stack pointer
> + */
Are we? I think we can be just as well returning back to kernel-space.
Similar to how we can enter the exception handler from kernel-space.
> + ALTERNATIVE("nop; nop",
> + __stringify(
> \
> + REG_L s3, TASK_TI_USER_SSP(tp); \
> + csrw CSR_SSP, s3),
> + 0,
> + RISCV_ISA_EXT_ZICFISS,
> + CONFIG_RISCV_USER_CFI)
> +
Thanks.
