diff mbox series

[RFC,v1,02/28] riscv: envcfg save and restore on trap entry/exit

Message ID 20240125062739.1339782-3-debug@rivosinc.com (mailing list archive)
State RFC
Headers show
Series riscv control-flow integrity for usermode | expand

Checks

Context Check Description
conchuod/vmtest-fixes-PR fail merge-conflict

Commit Message

Deepak Gupta Jan. 25, 2024, 6:21 a.m. UTC 
From: Deepak Gupta <debug@rivosinc.com>

envcfg CSR defines enabling bits for cache management instructions and soon
will control enabling for control flow integrity and pointer masking features.

Control flow integrity enabling for forward cfi and backward cfi is controlled
via envcfg and thus need to be enabled on per thread basis.

This patch creates a place holder for envcfg CSR in `thread_info` and adds
logic to save and restore on trap entry and exits.

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 arch/riscv/include/asm/thread_info.h | 1 +
 arch/riscv/kernel/asm-offsets.c      | 1 +
 arch/riscv/kernel/entry.S            | 4 ++++
 3 files changed, 6 insertions(+)

Comments

Stefan O'Rear Jan. 25, 2024, 7:19 a.m. UTC | #1 
On Thu, Jan 25, 2024, at 1:21 AM, debug@rivosinc.com wrote:
> From: Deepak Gupta <debug@rivosinc.com>
>
> envcfg CSR defines enabling bits for cache management instructions and soon
> will control enabling for control flow integrity and pointer masking features.
>
> Control flow integrity enabling for forward cfi and backward cfi is controlled
> via envcfg and thus need to be enabled on per thread basis.
>
> This patch creates a place holder for envcfg CSR in `thread_info` and adds
> logic to save and restore on trap entry and exits.

Should only be "restore"?  I don't see saving.

>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>  arch/riscv/include/asm/thread_info.h | 1 +
>  arch/riscv/kernel/asm-offsets.c      | 1 +
>  arch/riscv/kernel/entry.S            | 4 ++++
>  3 files changed, 6 insertions(+)
>
> diff --git a/arch/riscv/include/asm/thread_info.h 
> b/arch/riscv/include/asm/thread_info.h
> index 574779900bfb..320bc899a63b 100644
> --- a/arch/riscv/include/asm/thread_info.h
> +++ b/arch/riscv/include/asm/thread_info.h
> @@ -57,6 +57,7 @@ struct thread_info {
>  	long			user_sp;	/* User stack pointer */
>  	int			cpu;
>  	unsigned long		syscall_work;	/* SYSCALL_WORK_ flags */
> +	unsigned long envcfg;
>  #ifdef CONFIG_SHADOW_CALL_STACK
>  	void			*scs_base;
>  	void			*scs_sp;
> diff --git a/arch/riscv/kernel/asm-offsets.c 
> b/arch/riscv/kernel/asm-offsets.c
> index a03129f40c46..cdd8f095c30c 100644
> --- a/arch/riscv/kernel/asm-offsets.c
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -39,6 +39,7 @@ void asm_offsets(void)
>  	OFFSET(TASK_TI_PREEMPT_COUNT, task_struct, thread_info.preempt_count);
>  	OFFSET(TASK_TI_KERNEL_SP, task_struct, thread_info.kernel_sp);
>  	OFFSET(TASK_TI_USER_SP, task_struct, thread_info.user_sp);
> +	OFFSET(TASK_TI_ENVCFG, task_struct, thread_info.envcfg);
>  #ifdef CONFIG_SHADOW_CALL_STACK
>  	OFFSET(TASK_TI_SCS_SP, task_struct, thread_info.scs_sp);
>  #endif
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 54ca4564a926..63c3855ba80d 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -129,6 +129,10 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
>  	addi s0, sp, PT_SIZE_ON_STACK
>  	REG_S s0, TASK_TI_KERNEL_SP(tp)
> 
> +	/* restore envcfg bits for current thread */
> +	REG_L s0, TASK_TI_ENVCFG(tp)
> +	csrw CSR_ENVCFG, s0
> +

This is redundant if we're repeatedly processing interrupts or exceptions
within a single task.  We should only be writing envcfg when switching
between tasks or as part of the prctl.

We need to use an ALTERNATIVE for this since the oldest supported hardware
does not have envcfg csrs.

-s

>  	/* Save the kernel shadow call stack pointer */
>  	scs_save_current
> 
> -- 
> 2.43.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Deepak Gupta Jan. 25, 2024, 5:09 p.m. UTC | #2 
On Thu, Jan 25, 2024 at 02:19:29AM -0500, Stefan O'Rear wrote:
>On Thu, Jan 25, 2024, at 1:21 AM, debug@rivosinc.com wrote:
>> From: Deepak Gupta <debug@rivosinc.com>
>>
>> envcfg CSR defines enabling bits for cache management instructions and soon
>> will control enabling for control flow integrity and pointer masking features.
>>
>> Control flow integrity enabling for forward cfi and backward cfi is controlled
>> via envcfg and thus need to be enabled on per thread basis.
>>
>> This patch creates a place holder for envcfg CSR in `thread_info` and adds
>> logic to save and restore on trap entry and exits.
>
>Should only be "restore"?  I don't see saving.

It's always saved in `thread_info` and user mode can't change it.
So no point saving it.

>
>>
>> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>> ---
>>  arch/riscv/include/asm/thread_info.h | 1 +
>>  arch/riscv/kernel/asm-offsets.c      | 1 +
>>  arch/riscv/kernel/entry.S            | 4 ++++
>>  3 files changed, 6 insertions(+)
>>
>> diff --git a/arch/riscv/include/asm/thread_info.h
>> b/arch/riscv/include/asm/thread_info.h
>> index 574779900bfb..320bc899a63b 100644
>> --- a/arch/riscv/include/asm/thread_info.h
>> +++ b/arch/riscv/include/asm/thread_info.h
>> @@ -57,6 +57,7 @@ struct thread_info {
>>  	long			user_sp;	/* User stack pointer */
>>  	int			cpu;
>>  	unsigned long		syscall_work;	/* SYSCALL_WORK_ flags */
>> +	unsigned long envcfg;
>>  #ifdef CONFIG_SHADOW_CALL_STACK
>>  	void			*scs_base;
>>  	void			*scs_sp;
>> diff --git a/arch/riscv/kernel/asm-offsets.c
>> b/arch/riscv/kernel/asm-offsets.c
>> index a03129f40c46..cdd8f095c30c 100644
>> --- a/arch/riscv/kernel/asm-offsets.c
>> +++ b/arch/riscv/kernel/asm-offsets.c
>> @@ -39,6 +39,7 @@ void asm_offsets(void)
>>  	OFFSET(TASK_TI_PREEMPT_COUNT, task_struct, thread_info.preempt_count);
>>  	OFFSET(TASK_TI_KERNEL_SP, task_struct, thread_info.kernel_sp);
>>  	OFFSET(TASK_TI_USER_SP, task_struct, thread_info.user_sp);
>> +	OFFSET(TASK_TI_ENVCFG, task_struct, thread_info.envcfg);
>>  #ifdef CONFIG_SHADOW_CALL_STACK
>>  	OFFSET(TASK_TI_SCS_SP, task_struct, thread_info.scs_sp);
>>  #endif
>> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
>> index 54ca4564a926..63c3855ba80d 100644
>> --- a/arch/riscv/kernel/entry.S
>> +++ b/arch/riscv/kernel/entry.S
>> @@ -129,6 +129,10 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
>>  	addi s0, sp, PT_SIZE_ON_STACK
>>  	REG_S s0, TASK_TI_KERNEL_SP(tp)
>>
>> +	/* restore envcfg bits for current thread */
>> +	REG_L s0, TASK_TI_ENVCFG(tp)
>> +	csrw CSR_ENVCFG, s0
>> +
>
>This is redundant if we're repeatedly processing interrupts or exceptions
>within a single task.  We should only be writing envcfg when switching
>between tasks or as part of the prctl.
>
>We need to use an ALTERNATIVE for this since the oldest supported hardware
>does not have envcfg csrs.

Yeah fixing that in next series. Thanks

>
>-s
>
>>  	/* Save the kernel shadow call stack pointer */
>>  	scs_save_current
>>
>> --
>> 2.43.0
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
Deepak Gupta Jan. 25, 2024, 5:54 p.m. UTC | #3 
On Thu, Jan 25, 2024 at 09:09:14AM -0800, Deepak Gupta wrote:
>On Thu, Jan 25, 2024 at 02:19:29AM -0500, Stefan O'Rear wrote:
>>On Thu, Jan 25, 2024, at 1:21 AM, debug@rivosinc.com wrote:
>>>From: Deepak Gupta <debug@rivosinc.com>
>>>
>>>envcfg CSR defines enabling bits for cache management instructions and soon
>>>will control enabling for control flow integrity and pointer masking features.
>>>
>>>Control flow integrity enabling for forward cfi and backward cfi is controlled
>>>via envcfg and thus need to be enabled on per thread basis.
>>>
>>>This patch creates a place holder for envcfg CSR in `thread_info` and adds
>>>logic to save and restore on trap entry and exits.
>>
>>Should only be "restore"?  I don't see saving.
>
>It's always saved in `thread_info` and user mode can't change it.
>So no point saving it.

Also I'll fix the commit message. I think that's what you were pointing out.

>
>>
>>>
>>>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>>---
>>> arch/riscv/include/asm/thread_info.h | 1 +
>>> arch/riscv/kernel/asm-offsets.c      | 1 +
>>> arch/riscv/kernel/entry.S            | 4 ++++
>>> 3 files changed, 6 insertions(+)
>>>
>>>diff --git a/arch/riscv/include/asm/thread_info.h
>>>b/arch/riscv/include/asm/thread_info.h
>>>index 574779900bfb..320bc899a63b 100644
>>>--- a/arch/riscv/include/asm/thread_info.h
>>>+++ b/arch/riscv/include/asm/thread_info.h
>>>@@ -57,6 +57,7 @@ struct thread_info {
>>> 	long			user_sp;	/* User stack pointer */
>>> 	int			cpu;
>>> 	unsigned long		syscall_work;	/* SYSCALL_WORK_ flags */
>>>+	unsigned long envcfg;
>>> #ifdef CONFIG_SHADOW_CALL_STACK
>>> 	void			*scs_base;
>>> 	void			*scs_sp;
>>>diff --git a/arch/riscv/kernel/asm-offsets.c
>>>b/arch/riscv/kernel/asm-offsets.c
>>>index a03129f40c46..cdd8f095c30c 100644
>>>--- a/arch/riscv/kernel/asm-offsets.c
>>>+++ b/arch/riscv/kernel/asm-offsets.c
>>>@@ -39,6 +39,7 @@ void asm_offsets(void)
>>> 	OFFSET(TASK_TI_PREEMPT_COUNT, task_struct, thread_info.preempt_count);
>>> 	OFFSET(TASK_TI_KERNEL_SP, task_struct, thread_info.kernel_sp);
>>> 	OFFSET(TASK_TI_USER_SP, task_struct, thread_info.user_sp);
>>>+	OFFSET(TASK_TI_ENVCFG, task_struct, thread_info.envcfg);
>>> #ifdef CONFIG_SHADOW_CALL_STACK
>>> 	OFFSET(TASK_TI_SCS_SP, task_struct, thread_info.scs_sp);
>>> #endif
>>>diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
>>>index 54ca4564a926..63c3855ba80d 100644
>>>--- a/arch/riscv/kernel/entry.S
>>>+++ b/arch/riscv/kernel/entry.S
>>>@@ -129,6 +129,10 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
>>> 	addi s0, sp, PT_SIZE_ON_STACK
>>> 	REG_S s0, TASK_TI_KERNEL_SP(tp)
>>>
>>>+	/* restore envcfg bits for current thread */
>>>+	REG_L s0, TASK_TI_ENVCFG(tp)
>>>+	csrw CSR_ENVCFG, s0
>>>+
>>
>>This is redundant if we're repeatedly processing interrupts or exceptions
>>within a single task.  We should only be writing envcfg when switching
>>between tasks or as part of the prctl.
>>
>>We need to use an ALTERNATIVE for this since the oldest supported hardware
>>does not have envcfg csrs.
>
>Yeah fixing that in next series. Thanks
>
>>
>>-s
>>
>>> 	/* Save the kernel shadow call stack pointer */
>>> 	scs_save_current
>>>
>>>--
>>>2.43.0
>>>
>>>
>>>_______________________________________________
>>>linux-riscv mailing list
>>>linux-riscv@lists.infradead.org
>>>http://lists.infradead.org/mailman/listinfo/linux-riscv
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
index 574779900bfb..320bc899a63b 100644
--- a/arch/riscv/include/asm/thread_info.h
+++ b/arch/riscv/include/asm/thread_info.h
@@ -57,6 +57,7 @@  struct thread_info {
 	long			user_sp;	/* User stack pointer */
 	int			cpu;
 	unsigned long		syscall_work;	/* SYSCALL_WORK_ flags */
+	unsigned long envcfg;
 #ifdef CONFIG_SHADOW_CALL_STACK
 	void			*scs_base;
 	void			*scs_sp;
diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index a03129f40c46..cdd8f095c30c 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -39,6 +39,7 @@  void asm_offsets(void)
 	OFFSET(TASK_TI_PREEMPT_COUNT, task_struct, thread_info.preempt_count);
 	OFFSET(TASK_TI_KERNEL_SP, task_struct, thread_info.kernel_sp);
 	OFFSET(TASK_TI_USER_SP, task_struct, thread_info.user_sp);
+	OFFSET(TASK_TI_ENVCFG, task_struct, thread_info.envcfg);
 #ifdef CONFIG_SHADOW_CALL_STACK
 	OFFSET(TASK_TI_SCS_SP, task_struct, thread_info.scs_sp);
 #endif
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 54ca4564a926..63c3855ba80d 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -129,6 +129,10 @@  SYM_CODE_START_NOALIGN(ret_from_exception)
 	addi s0, sp, PT_SIZE_ON_STACK
 	REG_S s0, TASK_TI_KERNEL_SP(tp)
 
+	/* restore envcfg bits for current thread */
+	REG_L s0, TASK_TI_ENVCFG(tp)
+	csrw CSR_ENVCFG, s0
+
 	/* Save the kernel shadow call stack pointer */
 	scs_save_current