| Message ID | 20250314-v5_user_cfi_series-v12-19-e51202b53138@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv control-flow integrity for usermode | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
2025-03-14T14:39:38-07:00, Deepak Gupta <debug@rivosinc.com>: > Expose a new register type NT_RISCV_USER_CFI for risc-v cfi status and > state. Intentionally both landing pad and shadow stack status and state > are rolled into cfi state. Creating two different NT_RISCV_USER_XXX would > not be useful and wastage of a note type. Enabling or disabling of feature > is not allowed via ptrace set interface. However setting `elp` state or > setting shadow stack pointer are allowed via ptrace set interface. It is > expected `gdb` might have use to fixup `elp` state or `shadow stack` > pointer. > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > --- > arch/riscv/include/uapi/asm/ptrace.h | 18 ++++++++ > arch/riscv/kernel/ptrace.c | 83 ++++++++++++++++++++++++++++++++++++ > include/uapi/linux/elf.h | 1 + > 3 files changed, 102 insertions(+) > > diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h > index 659ea3af5680..e6571fba8a8a 100644 > --- a/arch/riscv/include/uapi/asm/ptrace.h > +++ b/arch/riscv/include/uapi/asm/ptrace.h > @@ -131,6 +131,24 @@ struct __sc_riscv_cfi_state { > unsigned long ss_ptr; /* shadow stack pointer */ > }; > > +struct __cfi_status { > + /* indirect branch tracking state */ > + __u64 lp_en : 1; > + __u64 lp_lock : 1; > + __u64 elp_state : 1; > + > + /* shadow stack status */ > + __u64 shstk_en : 1; > + __u64 shstk_lock : 1; I remember there was deep hatred towards bitfields in the Linux community, have things changes? > + __u64 rsvd : sizeof(__u64) - 5; I think you meant "64 - 5". > +}; > + > +struct user_cfi_state { > + struct __cfi_status cfi_status; > + __u64 shstk_ptr; > +}; > + > #endif /* __ASSEMBLY__ */ > > #endif /* _UAPI_ASM_RISCV_PTRACE_H */ > diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c > @@ -224,6 +297,16 @@ static const struct user_regset riscv_user_regset[] = { > .set = tagged_addr_ctrl_set, > }, > #endif > +#ifdef CONFIG_RISCV_USER_CFI > + [REGSET_CFI] = { > + .core_note_type = NT_RISCV_USER_CFI, > + .align = sizeof(__u64), > + .n = sizeof(struct user_cfi_state) / sizeof(__u64), > + .size = sizeof(__u64), Why not `size = sizeof(struct user_cfi_state)` and `n = 1`? > + .regset_get = riscv_cfi_get, > + .set = riscv_cfi_set, > + }, > +#endif [I haven't yet reviewed if a new register is the right thing to do nor looked at the rest of the patch.]
On Thu, Mar 20, 2025 at 3:24 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote: > > 2025-03-14T14:39:38-07:00, Deepak Gupta <debug@rivosinc.com>: > > Expose a new register type NT_RISCV_USER_CFI for risc-v cfi status and > > state. Intentionally both landing pad and shadow stack status and state > > are rolled into cfi state. Creating two different NT_RISCV_USER_XXX would > > not be useful and wastage of a note type. Enabling or disabling of feature > > is not allowed via ptrace set interface. However setting `elp` state or > > setting shadow stack pointer are allowed via ptrace set interface. It is > > expected `gdb` might have use to fixup `elp` state or `shadow stack` > > pointer. > > > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > > --- > > arch/riscv/include/uapi/asm/ptrace.h | 18 ++++++++ > > arch/riscv/kernel/ptrace.c | 83 ++++++++++++++++++++++++++++++++++++ > > include/uapi/linux/elf.h | 1 + > > 3 files changed, 102 insertions(+) > > > > diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h > > index 659ea3af5680..e6571fba8a8a 100644 > > --- a/arch/riscv/include/uapi/asm/ptrace.h > > +++ b/arch/riscv/include/uapi/asm/ptrace.h > > @@ -131,6 +131,24 @@ struct __sc_riscv_cfi_state { > > unsigned long ss_ptr; /* shadow stack pointer */ > > }; > > > > +struct __cfi_status { > > + /* indirect branch tracking state */ > > + __u64 lp_en : 1; > > + __u64 lp_lock : 1; > > + __u64 elp_state : 1; > > + > > + /* shadow stack status */ > > + __u64 shstk_en : 1; > > + __u64 shstk_lock : 1; > > I remember there was deep hatred towards bitfields in the Linux > community, have things changes? hmm. I didn't know about the strong hatred. Although I can see lots of examples of this pattern in existing kernel code. No strong feelings on my side, I can change this and have it single 64bit field and accessed via bitmasks. > > > + __u64 rsvd : sizeof(__u64) - 5; > > I think you meant "64 - 5". eeh. bad bug. thanks. > > > +}; > > + > > +struct user_cfi_state { > > + struct __cfi_status cfi_status; > > + __u64 shstk_ptr; > > +}; > > + > > #endif /* __ASSEMBLY__ */ > > > > #endif /* _UAPI_ASM_RISCV_PTRACE_H */ > > diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c > > @@ -224,6 +297,16 @@ static const struct user_regset riscv_user_regset[] = { > > .set = tagged_addr_ctrl_set, > > }, > > #endif > > +#ifdef CONFIG_RISCV_USER_CFI > > + [REGSET_CFI] = { > > + .core_note_type = NT_RISCV_USER_CFI, > > + .align = sizeof(__u64), > > + .n = sizeof(struct user_cfi_state) / sizeof(__u64), > > + .size = sizeof(__u64), > > Why not `size = sizeof(struct user_cfi_state)` and `n = 1`? yeah another good catch. Should write a kselftest against it, so that it can be caught. > > > + .regset_get = riscv_cfi_get, > > + .set = riscv_cfi_set, > > + }, > > +#endif > > [I haven't yet reviewed if a new register is the right thing to do nor > looked at the rest of the patch.]
2025-03-20T16:09:12-07:00, Deepak Gupta <debug@rivosinc.com>: > On Thu, Mar 20, 2025 at 3:24 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote: >> 2025-03-14T14:39:38-07:00, Deepak Gupta <debug@rivosinc.com>: >> > Expose a new register type NT_RISCV_USER_CFI for risc-v cfi status and >> > state. Intentionally both landing pad and shadow stack status and state >> > are rolled into cfi state. Creating two different NT_RISCV_USER_XXX would >> > not be useful and wastage of a note type. Enabling or disabling of feature >> > is not allowed via ptrace set interface. However setting `elp` state or >> > setting shadow stack pointer are allowed via ptrace set interface. It is >> > expected `gdb` might have use to fixup `elp` state or `shadow stack` >> > pointer. >> > >> > Signed-off-by: Deepak Gupta <debug@rivosinc.com> >> > --- >> > diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h >> > index 659ea3af5680..e6571fba8a8a 100644 >> > @@ -131,6 +131,24 @@ struct __sc_riscv_cfi_state { >> > unsigned long ss_ptr; /* shadow stack pointer */ >> > }; >> > >> > +struct __cfi_status { >> > + /* indirect branch tracking state */ >> > + __u64 lp_en : 1; >> > + __u64 lp_lock : 1; >> > + __u64 elp_state : 1; >> > + >> > + /* shadow stack status */ >> > + __u64 shstk_en : 1; >> > + __u64 shstk_lock : 1; >> >> I remember there was deep hatred towards bitfields in the Linux >> community, have things changes? > > hmm. I didn't know about the strong hatred. There is a good reason for it. :) The C standard left important behavior as implementation-specific (by mistake, I hope). I do like bitfields, but you have to be extra careful when working with them. > Although I can see lots of examples of this pattern in existing kernel code. > No strong feelings on my side, I can change this and have it single 64bit field > and accessed via bitmasks. This is uapi and bitfields do not specify the internal representation. A program compiled at a different time can see completely different order of the bitfields, so the uapi would break. We cannot use bitfields here.
diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h index 659ea3af5680..e6571fba8a8a 100644 --- a/arch/riscv/include/uapi/asm/ptrace.h +++ b/arch/riscv/include/uapi/asm/ptrace.h @@ -131,6 +131,24 @@ struct __sc_riscv_cfi_state { unsigned long ss_ptr; /* shadow stack pointer */ }; +struct __cfi_status { + /* indirect branch tracking state */ + __u64 lp_en : 1; + __u64 lp_lock : 1; + __u64 elp_state : 1; + + /* shadow stack status */ + __u64 shstk_en : 1; + __u64 shstk_lock : 1; + + __u64 rsvd : sizeof(__u64) - 5; +}; + +struct user_cfi_state { + struct __cfi_status cfi_status; + __u64 shstk_ptr; +}; + #endif /* __ASSEMBLY__ */ #endif /* _UAPI_ASM_RISCV_PTRACE_H */ diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c index ea67e9fb7a58..df8b7c6ab671 100644 --- a/arch/riscv/kernel/ptrace.c +++ b/arch/riscv/kernel/ptrace.c @@ -19,6 +19,7 @@ #include <linux/regset.h> #include <linux/sched.h> #include <linux/sched/task_stack.h> +#include <asm/usercfi.h> enum riscv_regset { REGSET_X, @@ -31,6 +32,9 @@ enum riscv_regset { #ifdef CONFIG_RISCV_ISA_SUPM REGSET_TAGGED_ADDR_CTRL, #endif +#ifdef CONFIG_RISCV_USER_CFI + REGSET_CFI, +#endif }; static int riscv_gpr_get(struct task_struct *target, @@ -184,6 +188,75 @@ static int tagged_addr_ctrl_set(struct task_struct *target, } #endif +#ifdef CONFIG_RISCV_USER_CFI +static int riscv_cfi_get(struct task_struct *target, + const struct user_regset *regset, + struct membuf to) +{ + struct user_cfi_state user_cfi; + struct pt_regs *regs; + + regs = task_pt_regs(target); + + user_cfi.cfi_status.lp_en = is_indir_lp_enabled(target); + user_cfi.cfi_status.lp_lock = is_indir_lp_locked(target); + user_cfi.cfi_status.elp_state = (regs->status & SR_ELP); + + user_cfi.cfi_status.shstk_en = is_shstk_enabled(target); + user_cfi.cfi_status.shstk_lock = is_shstk_locked(target); + user_cfi.shstk_ptr = get_active_shstk(target); + + return membuf_write(&to, &user_cfi, sizeof(user_cfi)); +} + +/* + * Does it make sense to allowing enable / disable of cfi via ptrace? + * Not allowing enable / disable / locking control via ptrace for now. + * Setting shadow stack pointer is allowed. GDB might use it to unwind or + * some other fixup. Similarly gdb might want to suppress elp and may want + * to reset elp state. + */ +static int riscv_cfi_set(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + int ret; + struct user_cfi_state user_cfi; + struct pt_regs *regs; + + regs = task_pt_regs(target); + + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &user_cfi, 0, -1); + if (ret) + return ret; + + /* + * Not allowing enabling or locking shadow stack or landing pad + * There is no disabling of shadow stack or landing pad via ptrace + * rsvd field should be set to zero so that if those fields are needed in future + */ + if (user_cfi.cfi_status.lp_en || user_cfi.cfi_status.lp_lock || + user_cfi.cfi_status.shstk_en || user_cfi.cfi_status.shstk_lock || + !user_cfi.cfi_status.rsvd) + return -EINVAL; + + /* If lpad is enabled on target and ptrace requests to set / clear elp, do that */ + if (is_indir_lp_enabled(target)) { + if (user_cfi.cfi_status.elp_state) /* set elp state */ + regs->status |= SR_ELP; + else + regs->status &= ~SR_ELP; /* clear elp state */ + } + + /* If shadow stack enabled on target, set new shadow stack pointer */ + if (is_shstk_enabled(target)) + set_active_shstk(target, user_cfi.shstk_ptr); + + return 0; +} +#endif + static const struct user_regset riscv_user_regset[] = { [REGSET_X] = { .core_note_type = NT_PRSTATUS, @@ -224,6 +297,16 @@ static const struct user_regset riscv_user_regset[] = { .set = tagged_addr_ctrl_set, }, #endif +#ifdef CONFIG_RISCV_USER_CFI + [REGSET_CFI] = { + .core_note_type = NT_RISCV_USER_CFI, + .align = sizeof(__u64), + .n = sizeof(struct user_cfi_state) / sizeof(__u64), + .size = sizeof(__u64), + .regset_get = riscv_cfi_get, + .set = riscv_cfi_set, + }, +#endif }; static const struct user_regset_view riscv_user_native_view = { diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h index b44069d29cec..b9daed4ab780 100644 --- a/include/uapi/linux/elf.h +++ b/include/uapi/linux/elf.h @@ -452,6 +452,7 @@ typedef struct elf64_shdr { #define NT_RISCV_CSR 0x900 /* RISC-V Control and Status Registers */ #define NT_RISCV_VECTOR 0x901 /* RISC-V vector registers */ #define NT_RISCV_TAGGED_ADDR_CTRL 0x902 /* RISC-V tagged address control (prctl()) */ +#define NT_RISCV_USER_CFI 0x903 /* RISC-V shadow stack state */ #define NT_LOONGARCH_CPUCFG 0xa00 /* LoongArch CPU config registers */ #define NT_LOONGARCH_CSR 0xa01 /* LoongArch control and status registers */ #define NT_LOONGARCH_LSX 0xa02 /* LoongArch Loongson SIMD Extension registers */
Expose a new register type NT_RISCV_USER_CFI for risc-v cfi status and state. Intentionally both landing pad and shadow stack status and state are rolled into cfi state. Creating two different NT_RISCV_USER_XXX would not be useful and wastage of a note type. Enabling or disabling of feature is not allowed via ptrace set interface. However setting `elp` state or setting shadow stack pointer are allowed via ptrace set interface. It is expected `gdb` might have use to fixup `elp` state or `shadow stack` pointer. Signed-off-by: Deepak Gupta <debug@rivosinc.com> --- arch/riscv/include/uapi/asm/ptrace.h | 18 ++++++++ arch/riscv/kernel/ptrace.c | 83 ++++++++++++++++++++++++++++++++++++ include/uapi/linux/elf.h | 1 + 3 files changed, 102 insertions(+)