Re: [PATCH v12 05/28] riscv: usercfi state for task and save/restore of CSR_SSP on trap entry/exit

[Deepak Gupta <debug@rivosinc.com>]

On Thu, Apr 24, 2025 at 02:16:32PM +0200, Radim Krčmář wrote:
>2025-04-23T17:23:56-07:00, Deepak Gupta <debug@rivosinc.com>:
>> On Thu, Apr 10, 2025 at 01:04:39PM +0200, Radim Krčmář wrote:
>>>2025-03-14T14:39:24-07:00, Deepak Gupta <debug@rivosinc.com>:
>>>> 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.)
>>
>> If CSR_SCRATCH was 0, then we would be coming from kernel else flow goes
>> to `.Lsave_context`. If we were coming from kernel mode, then eventually
>> flow merges to `.Lsave_context`.
>>
>> So we will be saving CSR_SSP on all kernel -- > kernel trap handling. That
>> would be unnecessary. IIRC, this was one of the first review comments in
>> early RFC series of these patch series (to not touch CSR_SSP un-necessarily)
>>
>> We can avoid that by ensuring when we branch by determining if we are coming
>> from user to something like `.Lsave_ssp` which eventually merges into
>> ".Lsave_context". And if we were coming from kernel then we would branch to
>> `.Lsave_context` and thus skipping ssp save logic. But # of branches it
>> introduces in early exception handling is equivalent to what current patches
>> do. So I don't see any value in doing that.
>>
>> Let me know if I am missing something.
>
>Right, it's hard to avoid the extra branches.
>
>I think we could modify the entry point (STVEC), so we start at
>different paths based on kernel/userspace trap and only jump once to the
>common code, like:
>
>  SYM_CODE_START(handle_exception_kernel)
>    /* kernel setup magic */
>    j handle_exception_common
>  SYM_CODE_START(handle_exception_user)
>    /* userspace setup magic */
>  handle_exception_common:

Hmm... This can be done. But then it would require to constantly modify `stvec`
When you're going back to user mode, you would have to write `stvec` with addr
of `handle_exception_user`. But then you can easily get a NMI. It can become
ugly. Needs much more thought and on first glance feels error prone.

Only if we have an extension that allows different trap address depending on
mode you're coming from (arm does that, right?, I think x86 FRED also does
that)
>
>This is not a suggestion for this series.  I would be perfectly happy
>with just a cleaner code.
>
>Would it be possible to hide the ALTERNATIVE ugliness behind a macro and
>move it outside the code block that saves pt_regs?

Sure, I'll do something about it.

>
>Thanks.