diff mbox series

[v3,22/29] riscv sigcontext: adding cfi state field in sigcontext

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

Commit Message

Deepak Gupta April 3, 2024, 11:35 p.m. UTC
Shadow stack needs to be saved and restored on signal delivery and signal
return.

sigcontext embedded in ucontext is extendible. Adding cfi state in there
which can be used to save cfi state before signal delivery and restore
cfi state on sigreturn

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 arch/riscv/include/uapi/asm/sigcontext.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Andy Chiu May 24, 2024, 9:46 a.m. UTC | #1
Hi Deepak,

On Thu, Apr 4, 2024 at 7:42 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
> Shadow stack needs to be saved and restored on signal delivery and signal
> return.
>
> sigcontext embedded in ucontext is extendible. Adding cfi state in there
> which can be used to save cfi state before signal delivery and restore
> cfi state on sigreturn
>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>  arch/riscv/include/uapi/asm/sigcontext.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/riscv/include/uapi/asm/sigcontext.h b/arch/riscv/include/uapi/asm/sigcontext.h
> index cd4f175dc837..5ccdd94a0855 100644
> --- a/arch/riscv/include/uapi/asm/sigcontext.h
> +++ b/arch/riscv/include/uapi/asm/sigcontext.h
> @@ -21,6 +21,10 @@ struct __sc_riscv_v_state {
>         struct __riscv_v_ext_state v_state;
>  } __attribute__((aligned(16)));
>
> +struct __sc_riscv_cfi_state {
> +       unsigned long ss_ptr;   /* shadow stack pointer */
> +       unsigned long rsvd;             /* keeping another word reserved in case we need it */
> +};
>  /*
>   * Signal context structure
>   *
> @@ -29,6 +33,7 @@ struct __sc_riscv_v_state {
>   */
>  struct sigcontext {
>         struct user_regs_struct sc_regs;
> +       struct __sc_riscv_cfi_state sc_cfi_state;

I am concerned about this change as this could potentially break uabi.
Let's say there is a pre-CFI program running on this kernel. It
receives a signal so the kernel lays out the sig-stack as presented in
this structure. If the program accesses sc_fpregs, it would now get
sc_cfi_state. As the offset has changed, and the pre-CFI program has
not been re-compiled.

>         union {
>                 union __riscv_fp_state sc_fpregs;
>                 struct __riscv_extra_ext_header sc_extdesc;
> --
> 2.43.2
>

There may be two ways to deal with this. One is to use a different
signal ABI for CFI-enabled programs. This may complicate the user
space because new programs will have to determine whether it should
use the CFI-ABI at run time. Another way is to follow what Vector does
for signal stack. It adds a way to introduce new extensions on signal
stack without impacting ABI.

Please let me know if I misunderstand anything, thanks.

Cheers,
Andy
Deepak Gupta May 24, 2024, 7:11 p.m. UTC | #2
On Fri, May 24, 2024 at 05:46:16PM +0800, Andy Chiu wrote:
>Hi Deepak,
>
>On Thu, Apr 4, 2024 at 7:42 AM Deepak Gupta <debug@rivosinc.com> wrote:
>>
>> Shadow stack needs to be saved and restored on signal delivery and signal
>> return.
>>
>> sigcontext embedded in ucontext is extendible. Adding cfi state in there
>> which can be used to save cfi state before signal delivery and restore
>> cfi state on sigreturn
>>
>> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>> ---
>>  arch/riscv/include/uapi/asm/sigcontext.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/riscv/include/uapi/asm/sigcontext.h b/arch/riscv/include/uapi/asm/sigcontext.h
>> index cd4f175dc837..5ccdd94a0855 100644
>> --- a/arch/riscv/include/uapi/asm/sigcontext.h
>> +++ b/arch/riscv/include/uapi/asm/sigcontext.h
>> @@ -21,6 +21,10 @@ struct __sc_riscv_v_state {
>>         struct __riscv_v_ext_state v_state;
>>  } __attribute__((aligned(16)));
>>
>> +struct __sc_riscv_cfi_state {
>> +       unsigned long ss_ptr;   /* shadow stack pointer */
>> +       unsigned long rsvd;             /* keeping another word reserved in case we need it */
>> +};
>>  /*
>>   * Signal context structure
>>   *
>> @@ -29,6 +33,7 @@ struct __sc_riscv_v_state {
>>   */
>>  struct sigcontext {
>>         struct user_regs_struct sc_regs;
>> +       struct __sc_riscv_cfi_state sc_cfi_state;
>
>I am concerned about this change as this could potentially break uabi.
>Let's say there is a pre-CFI program running on this kernel. It
>receives a signal so the kernel lays out the sig-stack as presented in
>this structure. If the program accesses sc_fpregs, it would now get
>sc_cfi_state. As the offset has changed, and the pre-CFI program has
>not been re-compiled.

Yeah this is a problem if program was built with older kernel/old toolchain
(or cfi unaware toolchain). Thanks.

>
>>         union {
>>                 union __riscv_fp_state sc_fpregs;
>>                 struct __riscv_extra_ext_header sc_extdesc;
>> --
>> 2.43.2
>>
>
>There may be two ways to deal with this. One is to use a different
>signal ABI for CFI-enabled programs. This may complicate the user
>space because new programs will have to determine whether it should
>use the CFI-ABI at run time. Another way is to follow what Vector does
>for signal stack. It adds a way to introduce new extensions on signal
>stack without impacting ABI.
>
>Please let me know if I misunderstand anything, thanks.

I think following how vector does would be cleaner.
Let me munch on this a little bit.

>
>Cheers,
>Andy
diff mbox series

Patch

diff --git a/arch/riscv/include/uapi/asm/sigcontext.h b/arch/riscv/include/uapi/asm/sigcontext.h
index cd4f175dc837..5ccdd94a0855 100644
--- a/arch/riscv/include/uapi/asm/sigcontext.h
+++ b/arch/riscv/include/uapi/asm/sigcontext.h
@@ -21,6 +21,10 @@  struct __sc_riscv_v_state {
 	struct __riscv_v_ext_state v_state;
 } __attribute__((aligned(16)));
 
+struct __sc_riscv_cfi_state {
+	unsigned long ss_ptr;	/* shadow stack pointer */
+	unsigned long rsvd;		/* keeping another word reserved in case we need it */
+};
 /*
  * Signal context structure
  *
@@ -29,6 +33,7 @@  struct __sc_riscv_v_state {
  */
 struct sigcontext {
 	struct user_regs_struct sc_regs;
+	struct __sc_riscv_cfi_state sc_cfi_state;
 	union {
 		union __riscv_fp_state sc_fpregs;
 		struct __riscv_extra_ext_header sc_extdesc;