mbox series

[v4,00/30] riscv control-flow integrity for usermode

Message ID 20240912231650.3740732-1-debug@rivosinc.com
Headers show
Series riscv control-flow integrity for usermode | expand

Message

Deepak Gupta Sept. 12, 2024, 11:16 p.m. UTC
v4 for cpu assisted riscv user mode control flow integrity.
zicfiss and zicfilp [1] are ratified riscv CPU extensions.

v3 [2] was sent in April this year for riscv usermode control
flow integrity enabling.

To get more information on zicfilp and zicfiss riscv CPU extensions,
patch series adds documentation for `zicfilp` and `zicfiss`
Documentation/arch/riscv/zicfiss.rst
Documentation/arch/riscv/zicfilp.rst

Additionally, spec can be obtained from [1].

How to test this series
=======================

Toolchain
---------
$ git clone git@github.com:sifive/riscv-gnu-toolchain.git -b cfi-dev
$ riscv-gnu-toolchain/configure --prefix=<path-to-where-to-build> --with-arch=rv64gc_zicfilp_zicfiss --enable-linux --disable-gdb  --with-extra-multilib-test="rv64gc_zicfilp_zicfiss-lp64d:-static"
$ make -j$(nproc)

Qemu
----
$ git clone git@github.com:deepak0414/qemu.git -b zicfilp_zicfiss_ratified_master_july11
$ cd qemu
$ mkdir build
$ cd build
$ ../configure --target-list=riscv64-softmmu
$ make -j$(nproc)

Opensbi
-------
$ git clone git@github.com:deepak0414/opensbi.git -b cfi_spec_split_opensbi
$ make CROSS_COMPILE=<your riscv toolchain> -j$(nproc) PLATFORM=generic

Linux
-----
Running defconfig is fine. CFI is enabled by default if the toolchain
supports it.

$ make ARCH=riscv CROSS_COMPILE=<path-to-cfi-riscv-gnu-toolchain>/build/bin/riscv64-unknown-linux-gnu- -j$(nproc) defconfig
$ make ARCH=riscv CROSS_COMPILE=<path-to-cfi-riscv-gnu-toolchain>/build/bin/riscv64-unknown-linux-gnu- -j$(nproc)

Running
-------

Modify your qemu command to have:
-bios <path-to-cfi-opensbi>/build/platform/generic/firmware/fw_dynamic.bin
-cpu rv64,zicfilp=true,zicfiss=true,zimop=true,zcmop=true


vDSO related Opens (in the flux)
=================================

I am listing these opens for laying out plan and what to expect in future
patch sets. And of course for the sake of discussion.

Shadow stack and landing pad enabling in vDSO
----------------------------------------------
vDSO must have shadow stack and landing pad support compiled in for task
to have shadow stack and landing pad support. This patch series doesn't
enable that (yet). Enabling shadow stack support in vDSO should be
straight forward (intend to do that in next versions of patch set). Enabling
landing pad support in vDSO requires some collaboration with toolchain folks
to follow a single label scheme for all object binaries. This is necessary to
ensure that all indirect call-sites are setting correct label and target landing
pads are decorated with same label scheme.


How many vDSOs
---------------
Shadow stack instructions are carved out of zimop (may be operations) and if CPU
doesn't implement zimop, they're illegal instructions. Kernel could be running on
a CPU which may or may not implement zimop. And thus kernel will have to carry 2
different vDSOs and expose the appropriate one depending on whether CPU implements
zimop or not.

[1] - https://github.com/riscv/riscv-cfi
[2] - https://lore.kernel.org/lkml/20240403234054.2020347-1-debug@rivosinc.com/

---
changelog
---------

v4
--
- rebased on 6.11-rc6
- envcfg: Converged with Samuel Holland's patches for envcfg management on per-
thread basis.
- vma_is_shadow_stack is renamed to is_vma_shadow_stack
- picked up Mark Brown's `ARCH_HAS_USER_SHADOW_STACK` patch
- signal context: using extended context management to maintain compatibility.
- fixed `-Wmissing-prototypes` compiler warnings for prctl functions
- Documentation fixes and amending typos.

v3
--
envcfg:
logic to pick up base envcfg had a bug where `ENVCFG_CBZE` could have been
picked on per task basis, even though CPU didn't implement it. Fixed in
this series.

dt-bindings:
As suggested, split into separate commit. fixed the messaging that spec is
in public review

arch_is_shadow_stack change:
arch_is_shadow_stack changed to vma_is_shadow_stack

hwprobe:
zicfiss / zicfilp if present will get enumerated in hwprobe

selftests:
As suggested, added object and binary filenames to .gitignore
Selftest binary anyways need to be compiled with cfi enabled compiler which
will make sure that landing pad and shadow stack are enabled. Thus removed
separate enable/disable tests. Cleaned up tests a bit.

v2
--

- Using config `CONFIG_RISCV_USER_CFI`, kernel support for riscv control flow
integrity for user mode programs can be compiled in the kernel.

- Enabling of control flow integrity for user programs is left to user runtime

- This patch series introduces arch agnostic `prctls` to enable shadow stack
and indirect branch tracking. And implements them on riscv.

Deepak Gupta (25):
  mm: helper `is_shadow_stack_vma` to check shadow stack vma
  riscv/Kconfig: enable HAVE_EXIT_THREAD for riscv
  riscv: zicfilp / zicfiss in dt-bindings (extensions.yaml)
  riscv: zicfiss / zicfilp enumeration
  riscv: zicfiss / zicfilp extension csr and bit definitions
  riscv: usercfi state for task and save/restore of CSR_SSP on trap
    entry/exit
  riscv/mm : ensure PROT_WRITE leads to VM_READ | VM_WRITE
  riscv mm: manufacture shadow stack pte
  riscv mmu: teach pte_mkwrite to manufacture shadow stack PTEs
  riscv mmu: write protect and shadow stack
  riscv/mm: Implement map_shadow_stack() syscall
  riscv/shstk: If needed allocate a new shadow stack on clone
  prctl: arch-agnostic prctl for indirect branch tracking
  riscv: Implements arch agnostic shadow stack prctls
  riscv: Implements arch agnostic indirect branch tracking prctls
  riscv/traps: Introduce software check exception
  riscv sigcontext: cfi state struct definition for sigcontext
  riscv signal: save and restore of shadow stack for signal
  riscv/kernel: update __show_regs to print shadow stack register
  riscv/ptrace: riscv cfi status and state via ptrace and in core files
  riscv/hwprobe: zicfilp / zicfiss enumeration in hwprobe
  riscv: create a config for shadow stack and landing pad instr support
  riscv: Documentation for landing pad / indirect branch tracking
  riscv: Documentation for shadow stack on riscv
  kselftest/riscv: kselftest for user mode cfi

Mark Brown (2):
  mm: Introduce ARCH_HAS_USER_SHADOW_STACK
  prctl: arch-agnostic prctl for shadow stack

Samuel Holland (3):
  riscv: Enable cbo.zero only when all harts support Zicboz
  riscv: Add support for per-thread envcfg CSR values
  riscv: Call riscv_user_isa_enable() only on the boot hart

 Documentation/arch/riscv/zicfilp.rst          | 104 ++++
 Documentation/arch/riscv/zicfiss.rst          | 169 ++++++
 .../devicetree/bindings/riscv/extensions.yaml |  12 +
 arch/riscv/Kconfig                            |  20 +
 arch/riscv/include/asm/asm-prototypes.h       |   1 +
 arch/riscv/include/asm/cpufeature.h           |  15 +-
 arch/riscv/include/asm/csr.h                  |  16 +
 arch/riscv/include/asm/entry-common.h         |   2 +
 arch/riscv/include/asm/hwcap.h                |   2 +
 arch/riscv/include/asm/mman.h                 |  24 +
 arch/riscv/include/asm/pgtable.h              |  30 +-
 arch/riscv/include/asm/processor.h            |   2 +
 arch/riscv/include/asm/switch_to.h            |   8 +
 arch/riscv/include/asm/thread_info.h          |   4 +
 arch/riscv/include/asm/usercfi.h              | 142 +++++
 arch/riscv/include/uapi/asm/hwprobe.h         |   2 +
 arch/riscv/include/uapi/asm/ptrace.h          |  18 +
 arch/riscv/include/uapi/asm/sigcontext.h      |   3 +
 arch/riscv/kernel/Makefile                    |   2 +
 arch/riscv/kernel/asm-offsets.c               |   4 +
 arch/riscv/kernel/cpufeature.c                |  13 +-
 arch/riscv/kernel/entry.S                     |  29 +
 arch/riscv/kernel/process.c                   |  32 +-
 arch/riscv/kernel/ptrace.c                    |  83 +++
 arch/riscv/kernel/signal.c                    |  62 ++-
 arch/riscv/kernel/smpboot.c                   |   2 -
 arch/riscv/kernel/suspend.c                   |   4 +-
 arch/riscv/kernel/sys_hwprobe.c               |   2 +
 arch/riscv/kernel/sys_riscv.c                 |  10 +
 arch/riscv/kernel/traps.c                     |  38 ++
 arch/riscv/kernel/usercfi.c                   | 506 ++++++++++++++++++
 arch/riscv/mm/init.c                          |   2 +-
 arch/riscv/mm/pgtable.c                       |  17 +
 arch/x86/Kconfig                              |   1 +
 fs/proc/task_mmu.c                            |   2 +-
 include/linux/cpu.h                           |   4 +
 include/linux/mm.h                            |  12 +-
 include/uapi/asm-generic/mman.h               |   1 +
 include/uapi/linux/elf.h                      |   1 +
 include/uapi/linux/prctl.h                    |  48 ++
 kernel/sys.c                                  |  60 +++
 mm/Kconfig                                    |   6 +
 mm/gup.c                                      |   2 +-
 mm/internal.h                                 |   2 +-
 mm/mmap.c                                     |   1 +
 tools/testing/selftests/riscv/Makefile        |   2 +-
 tools/testing/selftests/riscv/cfi/.gitignore  |   3 +
 tools/testing/selftests/riscv/cfi/Makefile    |  10 +
 .../testing/selftests/riscv/cfi/cfi_rv_test.h |  83 +++
 .../selftests/riscv/cfi/riscv_cfi_test.c      |  82 +++
 .../testing/selftests/riscv/cfi/shadowstack.c | 362 +++++++++++++
 .../testing/selftests/riscv/cfi/shadowstack.h |  37 ++
 52 files changed, 2079 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/arch/riscv/zicfilp.rst
 create mode 100644 Documentation/arch/riscv/zicfiss.rst
 create mode 100644 arch/riscv/include/asm/mman.h
 create mode 100644 arch/riscv/include/asm/usercfi.h
 create mode 100644 arch/riscv/kernel/usercfi.c
 create mode 100644 tools/testing/selftests/riscv/cfi/.gitignore
 create mode 100644 tools/testing/selftests/riscv/cfi/Makefile
 create mode 100644 tools/testing/selftests/riscv/cfi/cfi_rv_test.h
 create mode 100644 tools/testing/selftests/riscv/cfi/riscv_cfi_test.c
 create mode 100644 tools/testing/selftests/riscv/cfi/shadowstack.c
 create mode 100644 tools/testing/selftests/riscv/cfi/shadowstack.h

Comments

Rob Herring Sept. 13, 2024, 12:18 a.m. UTC | #1
On Thu, 12 Sep 2024 16:16:26 -0700, Deepak Gupta wrote:
> Make an entry for cfi extensions in extensions.yaml.
> 
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>  .../devicetree/bindings/riscv/extensions.yaml        | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/riscv/extensions.yaml:367:75: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/riscv/extensions.yaml:368:13: [error] syntax error: expected <block end>, but found '<scalar>' (syntax)
./Documentation/devicetree/bindings/riscv/extensions.yaml:373:75: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/riscv/extensions.yaml:374:13: [warning] wrong indentation: expected 10 but found 12 (indentation)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/extensions.yaml: ignoring, error parsing file
make[2]: *** Deleting file 'Documentation/devicetree/bindings/riscv/extensions.example.dts'
Documentation/devicetree/bindings/riscv/extensions.yaml:368:13: did not find expected key
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/riscv/extensions.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/cpus.example.dtb: cpu@0: False schema does not allow {'clock-frequency': 0, 'compatible': ['sifive,rocket0', 'riscv'], 'device_type': ['cpu'], 'i-cache-block-size': 64, 'i-cache-sets': 128, 'i-cache-size': 16384, 'reg': [[0]], 'riscv,isa-base': ['rv64i'], 'riscv,isa-extensions': [1761635584, 1627415296], 'interrupt-controller': {'#interrupt-cells': 1, 'compatible': ['riscv,cpu-intc'], 'interrupt-controller': True}, '$nodename': ['cpu@0']}
	from schema $id: http://devicetree.org/schemas/riscv/cpus.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/cpus.example.dtb: cpu@0: Unevaluated properties are not allowed ('riscv,isa-base', 'riscv,isa-extensions' were unexpected)
	from schema $id: http://devicetree.org/schemas/riscv/cpus.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/cpus.example.dtb: cpu@1: False schema does not allow {'clock-frequency': 0, 'compatible': ['sifive,rocket0', 'riscv'], 'd-cache-block-size': 64, 'd-cache-sets': 64, 'd-cache-size': 32768, 'd-tlb-sets': 1, 'd-tlb-size': 32, 'device_type': ['cpu'], 'i-cache-block-size': 64, 'i-cache-sets': 64, 'i-cache-size': 32768, 'i-tlb-sets': 1, 'i-tlb-size': 32, 'mmu-type': ['riscv,sv39'], 'reg': [[1]], 'tlb-split': True, 'riscv,isa-base': ['rv64i'], 'riscv,isa-extensions': [1761635584, 1627416064, 1677746944], 'interrupt-controller': {'#interrupt-cells': 1, 'compatible': ['riscv,cpu-intc'], 'interrupt-controller': True}, '$nodename': ['cpu@1']}
	from schema $id: http://devicetree.org/schemas/riscv/cpus.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/cpus.example.dtb: cpu@1: Unevaluated properties are not allowed ('riscv,isa-base', 'riscv,isa-extensions' were unexpected)
	from schema $id: http://devicetree.org/schemas/riscv/cpus.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/cpus.example.dtb: cpu@0: False schema does not allow {'device_type': ['cpu'], 'reg': [[0]], 'compatible': ['riscv'], 'mmu-type': ['riscv,sv48'], 'riscv,isa-base': ['rv64i'], 'riscv,isa-extensions': [1761635584, 1627416064, 1677746944], 'interrupt-controller': {'#interrupt-cells': 1, 'interrupt-controller': True, 'compatible': ['riscv,cpu-intc']}, '$nodename': ['cpu@0']}
	from schema $id: http://devicetree.org/schemas/riscv/cpus.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/cpus.example.dtb: cpu@0: Unevaluated properties are not allowed ('riscv,isa-base', 'riscv,isa-extensions' were unexpected)
	from schema $id: http://devicetree.org/schemas/riscv/cpus.yaml#
./Documentation/devicetree/bindings/riscv/extensions.yaml:368:13: did not find expected key
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1432: dt_binding_check] Error 2
make: *** [Makefile:224: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240912231650.3740732-8-debug@rivosinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Carlos Bilbao Sept. 13, 2024, 3:51 p.m. UTC | #2
On 9/12/24 18:16, Deepak Gupta wrote:

> From: Mark Brown <broonie@kernel.org>
>
> Since multiple architectures have support for shadow stacks and we need to
> select support for this feature in several places in the generic code
> provide a generic config option that the architectures can select.
>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Reviewed-by: Deepak Gupta <debug@rivosinc.com>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>  arch/x86/Kconfig   | 1 +
>  fs/proc/task_mmu.c | 2 +-
>  include/linux/mm.h | 2 +-
>  mm/Kconfig         | 6 ++++++
>  4 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 007bab9f2a0e..320e1f411163 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1957,6 +1957,7 @@ config X86_USER_SHADOW_STACK
>  	depends on AS_WRUSS
>  	depends on X86_64
>  	select ARCH_USES_HIGH_VMA_FLAGS
> +	select ARCH_HAS_USER_SHADOW_STACK
>  	select X86_CET
>  	help
>  	  Shadow stack protection is a hardware feature that detects function
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 5f171ad7b436..0ea49725f524 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -984,7 +984,7 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
>  #ifdef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
>  		[ilog2(VM_UFFD_MINOR)]	= "ui",
>  #endif /* CONFIG_HAVE_ARCH_USERFAULTFD_MINOR */
> -#ifdef CONFIG_X86_USER_SHADOW_STACK
> +#ifdef CONFIG_ARCH_HAS_USER_SHADOW_STACK
>  		[ilog2(VM_SHADOW_STACK)] = "ss",
>  #endif
>  #ifdef CONFIG_64BIT
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 147073601716..e39796ea17db 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -346,7 +346,7 @@ extern unsigned int kobjsize(const void *objp);
>  #endif
>  #endif /* CONFIG_ARCH_HAS_PKEYS */
>  
> -#ifdef CONFIG_X86_USER_SHADOW_STACK
> +#ifdef CONFIG_ARCH_HAS_USER_SHADOW_STACK
>  /*
>   * VM_SHADOW_STACK should not be set with VM_SHARED because of lack of
>   * support core mm.
> diff --git a/mm/Kconfig b/mm/Kconfig
> index b72e7d040f78..3167be663bca 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -1263,6 +1263,12 @@ config IOMMU_MM_DATA
>  config EXECMEM
>  	bool
>  
> +config ARCH_HAS_USER_SHADOW_STACK
> +	bool
> +	help
> +	  The architecture has hardware support for userspace shadow call
> +          stacks (eg, x86 CET, arm64 GCS or RISC-V Zicfiss).
> +
>  source "mm/damon/Kconfig"


Reviewed-by: Carlos Bilbao <carlos.bilbao.osdev@gmail.com>


>  
>  endmenu


Thanks, Carlos
Andy Chiu Sept. 13, 2024, 7:25 p.m. UTC | #3
Hi Deepak,

Deepak Gupta <debug@rivosinc.com> 於 2024年9月13日 週五 上午1:20寫道:
>
> Save shadow stack pointer in sigcontext structure while delivering signal.
> Restore shadow stack pointer from sigcontext on sigreturn.
>
> As part of save operation, kernel uses `ssamoswap` to save snapshot of
> current shadow stack on shadow stack itself (can be called as a save
> token). During restore on sigreturn, kernel retrieves token from top of
> shadow stack and validates it. This allows that user mode can't arbitrary
> pivot to any shadow stack address without having a token and thus provide
> strong security assurance between signaly delivery and sigreturn window.
>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> Suggested-by: Andy Chiu <andy.chiu@sifive.com>
> ---
>  arch/riscv/include/asm/usercfi.h | 19 ++++++++++
>  arch/riscv/kernel/signal.c       | 62 +++++++++++++++++++++++++++++++-
>  arch/riscv/kernel/usercfi.c      | 57 +++++++++++++++++++++++++++++
>  3 files changed, 137 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/usercfi.h b/arch/riscv/include/asm/usercfi.h
> index 20a9102cce51..d5050a5df26c 100644
> --- a/arch/riscv/include/asm/usercfi.h
> +++ b/arch/riscv/include/asm/usercfi.h
> @@ -8,6 +8,7 @@
>  #ifndef __ASSEMBLY__
>  #include <linux/types.h>
>  #include <linux/prctl.h>
> +#include <linux/errno.h>
>
>  struct task_struct;
>  struct kernel_clone_args;
> @@ -35,6 +36,9 @@ bool is_shstk_locked(struct task_struct *task);
>  bool is_shstk_allocated(struct task_struct *task);
>  void set_shstk_lock(struct task_struct *task);
>  void set_shstk_status(struct task_struct *task, bool enable);
> +unsigned long get_active_shstk(struct task_struct *task);
> +int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr);
> +int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr);
>  bool is_indir_lp_enabled(struct task_struct *task);
>  bool is_indir_lp_locked(struct task_struct *task);
>  void set_indir_lp_status(struct task_struct *task, bool enable);
> @@ -96,6 +100,21 @@ static inline void set_shstk_status(struct task_struct *task, bool enable)
>
>  }
>
> +static inline int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr)
> +{
> +       return -EINVAL;
> +}
> +
> +static inline int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr)
> +{
> +       return -EINVAL;
> +}
> +
> +static inline unsigned long get_active_shstk(struct task_struct *task)
> +{
> +       return 0;
> +}
> +
>  static inline bool is_indir_lp_enabled(struct task_struct *task)
>  {
>         return false;
> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> index dcd282419456..7d5c1825650f 100644
> --- a/arch/riscv/kernel/signal.c
> +++ b/arch/riscv/kernel/signal.c
> @@ -22,6 +22,7 @@
>  #include <asm/vector.h>
>  #include <asm/csr.h>
>  #include <asm/cacheflush.h>
> +#include <asm/usercfi.h>
>
>  unsigned long signal_minsigstksz __ro_after_init;
>
> @@ -153,6 +154,16 @@ static long restore_sigcontext(struct pt_regs *regs,
>         void __user *sc_ext_ptr = &sc->sc_extdesc.hdr;
>         __u32 rsvd;
>         long err;
> +       unsigned long ss_ptr = 0;
> +       struct __sc_riscv_cfi_state __user *sc_cfi = NULL;
> +
> +       sc_cfi = (struct __sc_riscv_cfi_state *)
> +                ((unsigned long) sc_ext_ptr + sizeof(struct __riscv_ctx_hdr));
> +
> +       if (has_vector() && riscv_v_vstate_query(regs))
> +               sc_cfi = (struct __sc_riscv_cfi_state *)
> +                        ((unsigned long) sc_cfi + riscv_v_sc_size);
> +
>         /* sc_regs is structured the same as the start of pt_regs */
>         err = __copy_from_user(regs, &sc->sc_regs, sizeof(sc->sc_regs));
>         if (unlikely(err))
> @@ -172,6 +183,24 @@ static long restore_sigcontext(struct pt_regs *regs,
>         if (unlikely(rsvd))
>                 return -EINVAL;
>
> +       /*
> +        * Restore shadow stack as a form of token stored on shadow stack itself as a safe
> +        * way to restore.
> +        * A token on shadow gives following properties
> +        *      - Safe save and restore for shadow stack switching. Any save of shadow stack
> +        *        must have had saved a token on shadow stack. Similarly any restore of shadow
> +        *        stack must check the token before restore. Since writing to shadow stack with
> +        *        address of shadow stack itself is not easily allowed. A restore without a save
> +        *        is quite difficult for an attacker to perform.
> +        *      - A natural break. A token in shadow stack provides a natural break in shadow stack
> +        *        So a single linear range can be bucketed into different shadow stack segments.
> +        *        sspopchk will detect the condition and fault to kernel as sw check exception.
> +        */
> +       if (is_shstk_enabled(current)) {
> +               err |= __copy_from_user(&ss_ptr, &sc_cfi->ss_ptr, sizeof(unsigned long));
> +               err |= restore_user_shstk(current, ss_ptr);
> +       }
> +
>         while (!err) {
>                 __u32 magic, size;
>                 struct __riscv_ctx_hdr __user *head = sc_ext_ptr;
> @@ -215,6 +244,10 @@ static size_t get_rt_frame_size(bool cal_all)
>                 if (cal_all || riscv_v_vstate_query(task_pt_regs(current)))
>                         total_context_size += riscv_v_sc_size;
>         }
> +
> +       if (is_shstk_enabled(current))
> +               total_context_size += sizeof(struct __sc_riscv_cfi_state);
> +
>         /*
>          * Preserved a __riscv_ctx_hdr for END signal context header if an
>          * extension uses __riscv_extra_ext_header
> @@ -276,18 +309,40 @@ static long setup_sigcontext(struct rt_sigframe __user *frame,
>  {
>         struct sigcontext __user *sc = &frame->uc.uc_mcontext;
>         struct __riscv_ctx_hdr __user *sc_ext_ptr = &sc->sc_extdesc.hdr;
> +       unsigned long ss_ptr = 0;
> +       struct __sc_riscv_cfi_state __user *sc_cfi = NULL;
>         long err;
>
> +       sc_cfi = (struct __sc_riscv_cfi_state *) (sc_ext_ptr + 1);
> +

Is it intended that cfi sigcontext does not follow the sigcontext rule
setup by Vector? It seems like there is no extension header (struct
__riscv_ctx_hdr) defined for cfi sigcontext here. If the sigcontext is
directly appended to the signal stack, the user may not be able to
recognize the meaning without defining a new ABI.

BTW, I have sent a patch[1] that refactor setup_sigcontext so it'd be
easier for future extensions to expand on the signal stack.

>         /* sc_regs is structured the same as the start of pt_regs */
>         err = __copy_to_user(&sc->sc_regs, regs, sizeof(sc->sc_regs));
>         /* Save the floating-point state. */
>         if (has_fpu())
>                 err |= save_fp_state(regs, &sc->sc_fpregs);
>         /* Save the vector state. */
> -       if (has_vector() && riscv_v_vstate_query(regs))
> +       if (has_vector() && riscv_v_vstate_query(regs)) {
>                 err |= save_v_state(regs, (void __user **)&sc_ext_ptr);
> +               sc_cfi = (struct __sc_riscv_cfi_state *) ((unsigned long) sc_cfi + riscv_v_sc_size);
> +       }
>         /* Write zero to fp-reserved space and check it on restore_sigcontext */
>         err |= __put_user(0, &sc->sc_extdesc.reserved);
> +       /*
> +        * Save a pointer to shadow stack itself on shadow stack as a form of token.
> +        * A token on shadow gives following properties
> +        *      - Safe save and restore for shadow stack switching. Any save of shadow stack
> +        *        must have had saved a token on shadow stack. Similarly any restore of shadow
> +        *        stack must check the token before restore. Since writing to shadow stack with
> +        *        address of shadow stack itself is not easily allowed. A restore without a save
> +        *        is quite difficult for an attacker to perform.
> +        *      - A natural break. A token in shadow stack provides a natural break in shadow stack
> +        *        So a single linear range can be bucketed into different shadow stack segments. Any
> +        *        sspopchk will detect the condition and fault to kernel as sw check exception.
> +        */
> +       if (is_shstk_enabled(current)) {
> +               err |= save_user_shstk(current, &ss_ptr);
> +               err |= __put_user(ss_ptr, &sc_cfi->ss_ptr);
> +       }
>         /* And put END __riscv_ctx_hdr at the end. */
>         err |= __put_user(END_MAGIC, &sc_ext_ptr->magic);
>         err |= __put_user(END_HDR_SIZE, &sc_ext_ptr->size);
> @@ -345,6 +400,11 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
>  #ifdef CONFIG_MMU
>         regs->ra = (unsigned long)VDSO_SYMBOL(
>                 current->mm->context.vdso, rt_sigreturn);
> +
> +       /* if bcfi is enabled x1 (ra) and x5 (t0) must match. not sure if we need this? */
> +       if (is_shstk_enabled(current))
> +               regs->t0 = regs->ra;
> +
>  #else
>         /*
>          * For the nommu case we don't have a VDSO.  Instead we push two
> diff --git a/arch/riscv/kernel/usercfi.c b/arch/riscv/kernel/usercfi.c
> index 8da509afdbe9..40c32258b6ec 100644
> --- a/arch/riscv/kernel/usercfi.c
> +++ b/arch/riscv/kernel/usercfi.c
> @@ -52,6 +52,11 @@ void set_active_shstk(struct task_struct *task, unsigned long shstk_addr)
>         task->thread_info.user_cfi_state.user_shdw_stk = shstk_addr;
>  }
>
> +unsigned long get_active_shstk(struct task_struct *task)
> +{
> +       return task->thread_info.user_cfi_state.user_shdw_stk;
> +}
> +
>  void set_shstk_status(struct task_struct *task, bool enable)
>  {
>         task->thread_info.user_cfi_state.ubcfi_en = enable ? 1 : 0;
> @@ -164,6 +169,58 @@ static int create_rstor_token(unsigned long ssp, unsigned long *token_addr)
>         return 0;
>  }
>
> +/*
> + * Save user shadow stack pointer on shadow stack itself and return pointer to saved location
> + * returns -EFAULT if operation was unsuccessful
> + */
> +int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr)
> +{
> +       unsigned long ss_ptr = 0;
> +       unsigned long token_loc = 0;
> +       int ret = 0;
> +
> +       if (saved_shstk_ptr == NULL)
> +               return -EINVAL;
> +
> +       ss_ptr = get_active_shstk(tsk);
> +       ret = create_rstor_token(ss_ptr, &token_loc);
> +
> +       if (!ret) {
> +               *saved_shstk_ptr = token_loc;
> +               set_active_shstk(tsk, token_loc);
> +       }
> +
> +       return ret;
> +}
> +
> +/*
> + * Restores user shadow stack pointer from token on shadow stack for task `tsk`
> + * returns -EFAULT if operation was unsuccessful
> + */
> +int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr)
> +{
> +       unsigned long token = 0;
> +
> +       token = amo_user_shstk((unsigned long __user *)shstk_ptr, 0);
> +
> +       if (token == -1)
> +               return -EFAULT;
> +
> +       /* invalid token, return EINVAL */
> +       if ((token - shstk_ptr) != SHSTK_ENTRY_SIZE) {
> +               pr_info_ratelimited(
> +                               "%s[%d]: bad restore token in %s: pc=%p sp=%p, token=%p, shstk_ptr=%p\n",
> +                               tsk->comm, task_pid_nr(tsk), __func__,
> +                               (void *)(task_pt_regs(tsk)->epc), (void *)(task_pt_regs(tsk)->sp),
> +                               (void *)token, (void *)shstk_ptr);
> +               return -EINVAL;
> +       }
> +
> +       /* all checks passed, set active shstk and return success */
> +       set_active_shstk(tsk, token);
> +       return 0;
> +}
> +
>  static unsigned long allocate_shadow_stack(unsigned long addr, unsigned long size,
>                                 unsigned long token_offset,
>                                 bool set_tok)
> --
> 2.45.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

- [1]: https://lore.kernel.org/all/20240628-dev-signal-refactor-v1-1-0c391b260261@sifive.com/

Thanks,
Andy
Deepak Gupta Sept. 16, 2024, 10:03 p.m. UTC | #4
On Fri, Sep 13, 2024 at 09:25:57PM +0200, Andy Chiu wrote:
>Hi Deepak,
>
>Deepak Gupta <debug@rivosinc.com> 於 2024年9月13日 週五 上午1:20寫道:
>>
>> Save shadow stack pointer in sigcontext structure while delivering signal.
>> Restore shadow stack pointer from sigcontext on sigreturn.
>>
>> As part of save operation, kernel uses `ssamoswap` to save snapshot of
>> current shadow stack on shadow stack itself (can be called as a save
>> token). During restore on sigreturn, kernel retrieves token from top of
>> shadow stack and validates it. This allows that user mode can't arbitrary
>> pivot to any shadow stack address without having a token and thus provide
>> strong security assurance between signaly delivery and sigreturn window.
>>
>> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>> Suggested-by: Andy Chiu <andy.chiu@sifive.com>
>> ---
>>  arch/riscv/include/asm/usercfi.h | 19 ++++++++++
>>  arch/riscv/kernel/signal.c       | 62 +++++++++++++++++++++++++++++++-
>>  arch/riscv/kernel/usercfi.c      | 57 +++++++++++++++++++++++++++++
>>  3 files changed, 137 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/include/asm/usercfi.h b/arch/riscv/include/asm/usercfi.h
>> index 20a9102cce51..d5050a5df26c 100644
>> --- a/arch/riscv/include/asm/usercfi.h
>> +++ b/arch/riscv/include/asm/usercfi.h
>> @@ -8,6 +8,7 @@
>>  #ifndef __ASSEMBLY__
>>  #include <linux/types.h>
>>  #include <linux/prctl.h>
>> +#include <linux/errno.h>
>>
>>  struct task_struct;
>>  struct kernel_clone_args;
>> @@ -35,6 +36,9 @@ bool is_shstk_locked(struct task_struct *task);
>>  bool is_shstk_allocated(struct task_struct *task);
>>  void set_shstk_lock(struct task_struct *task);
>>  void set_shstk_status(struct task_struct *task, bool enable);
>> +unsigned long get_active_shstk(struct task_struct *task);
>> +int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr);
>> +int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr);
>>  bool is_indir_lp_enabled(struct task_struct *task);
>>  bool is_indir_lp_locked(struct task_struct *task);
>>  void set_indir_lp_status(struct task_struct *task, bool enable);
>> @@ -96,6 +100,21 @@ static inline void set_shstk_status(struct task_struct *task, bool enable)
>>
>>  }
>>
>> +static inline int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr)
>> +{
>> +       return -EINVAL;
>> +}
>> +
>> +static inline int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr)
>> +{
>> +       return -EINVAL;
>> +}
>> +
>> +static inline unsigned long get_active_shstk(struct task_struct *task)
>> +{
>> +       return 0;
>> +}
>> +
>>  static inline bool is_indir_lp_enabled(struct task_struct *task)
>>  {
>>         return false;
>> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
>> index dcd282419456..7d5c1825650f 100644
>> --- a/arch/riscv/kernel/signal.c
>> +++ b/arch/riscv/kernel/signal.c
>> @@ -22,6 +22,7 @@
>>  #include <asm/vector.h>
>>  #include <asm/csr.h>
>>  #include <asm/cacheflush.h>
>> +#include <asm/usercfi.h>
>>
>>  unsigned long signal_minsigstksz __ro_after_init;
>>
>> @@ -153,6 +154,16 @@ static long restore_sigcontext(struct pt_regs *regs,
>>         void __user *sc_ext_ptr = &sc->sc_extdesc.hdr;
>>         __u32 rsvd;
>>         long err;
>> +       unsigned long ss_ptr = 0;
>> +       struct __sc_riscv_cfi_state __user *sc_cfi = NULL;
>> +
>> +       sc_cfi = (struct __sc_riscv_cfi_state *)
>> +                ((unsigned long) sc_ext_ptr + sizeof(struct __riscv_ctx_hdr));
>> +
>> +       if (has_vector() && riscv_v_vstate_query(regs))
>> +               sc_cfi = (struct __sc_riscv_cfi_state *)
>> +                        ((unsigned long) sc_cfi + riscv_v_sc_size);
>> +
>>         /* sc_regs is structured the same as the start of pt_regs */
>>         err = __copy_from_user(regs, &sc->sc_regs, sizeof(sc->sc_regs));
>>         if (unlikely(err))
>> @@ -172,6 +183,24 @@ static long restore_sigcontext(struct pt_regs *regs,
>>         if (unlikely(rsvd))
>>                 return -EINVAL;
>>
>> +       /*
>> +        * Restore shadow stack as a form of token stored on shadow stack itself as a safe
>> +        * way to restore.
>> +        * A token on shadow gives following properties
>> +        *      - Safe save and restore for shadow stack switching. Any save of shadow stack
>> +        *        must have had saved a token on shadow stack. Similarly any restore of shadow
>> +        *        stack must check the token before restore. Since writing to shadow stack with
>> +        *        address of shadow stack itself is not easily allowed. A restore without a save
>> +        *        is quite difficult for an attacker to perform.
>> +        *      - A natural break. A token in shadow stack provides a natural break in shadow stack
>> +        *        So a single linear range can be bucketed into different shadow stack segments.
>> +        *        sspopchk will detect the condition and fault to kernel as sw check exception.
>> +        */
>> +       if (is_shstk_enabled(current)) {
>> +               err |= __copy_from_user(&ss_ptr, &sc_cfi->ss_ptr, sizeof(unsigned long));
>> +               err |= restore_user_shstk(current, ss_ptr);
>> +       }
>> +
>>         while (!err) {
>>                 __u32 magic, size;
>>                 struct __riscv_ctx_hdr __user *head = sc_ext_ptr;
>> @@ -215,6 +244,10 @@ static size_t get_rt_frame_size(bool cal_all)
>>                 if (cal_all || riscv_v_vstate_query(task_pt_regs(current)))
>>                         total_context_size += riscv_v_sc_size;
>>         }
>> +
>> +       if (is_shstk_enabled(current))
>> +               total_context_size += sizeof(struct __sc_riscv_cfi_state);
>> +
>>         /*
>>          * Preserved a __riscv_ctx_hdr for END signal context header if an
>>          * extension uses __riscv_extra_ext_header
>> @@ -276,18 +309,40 @@ static long setup_sigcontext(struct rt_sigframe __user *frame,
>>  {
>>         struct sigcontext __user *sc = &frame->uc.uc_mcontext;
>>         struct __riscv_ctx_hdr __user *sc_ext_ptr = &sc->sc_extdesc.hdr;
>> +       unsigned long ss_ptr = 0;
>> +       struct __sc_riscv_cfi_state __user *sc_cfi = NULL;
>>         long err;
>>
>> +       sc_cfi = (struct __sc_riscv_cfi_state *) (sc_ext_ptr + 1);
>> +
>
>Is it intended that cfi sigcontext does not follow the sigcontext rule
>setup by Vector? It seems like there is no extension header (struct
>__riscv_ctx_hdr) defined for cfi sigcontext here. If the sigcontext is
>directly appended to the signal stack, the user may not be able to
>recognize the meaning without defining a new ABI.

Hmm... I didn't realize that struct `struct __riscv_ctx_hdr` is strongly
tied to vector state. I was under the impression that any new extended
state addition would require this header to be present.

cfi sigcontenxt doesn't need any ABI between user and kernel here. We need
this space so that kernel can save a pointer to shadow stack token on signal
delivery. Once sigreturn happens, kernel will use the same pointer, verify
the token saved on shadow stack and restore shadow stack for user mode.
At no point in this scheme, user mode is required to perform any action.

All that is needed is that user mode doesn't accidenly trample at this offset.

Since I was under the impression that `struct __riscv_ctx_hdr` is there for
context extension and must be present for any state beyond `sc_regs`, I assumed
that I must make space for this header (even if vector state is not present).

>
>BTW, I have sent a patch[1] that refactor setup_sigcontext so it'd be
>easier for future extensions to expand on the signal stack.

I can adopt to this, although its orthogonal to what we are discussing here.

>
>>         /* sc_regs is structured the same as the start of pt_regs */
>>         err = __copy_to_user(&sc->sc_regs, regs, sizeof(sc->sc_regs));
>>         /* Save the floating-point state. */
>>         if (has_fpu())
>>                 err |= save_fp_state(regs, &sc->sc_fpregs);
>>         /* Save the vector state. */
>> -       if (has_vector() && riscv_v_vstate_query(regs))
>> +       if (has_vector() && riscv_v_vstate_query(regs)) {
>>                 err |= save_v_state(regs, (void __user **)&sc_ext_ptr);
>> +               sc_cfi = (struct __sc_riscv_cfi_state *) ((unsigned long) sc_cfi + riscv_v_sc_size);
>> +       }
>>         /* Write zero to fp-reserved space and check it on restore_sigcontext */
>>         err |= __put_user(0, &sc->sc_extdesc.reserved);
>> +       /*
>> +        * Save a pointer to shadow stack itself on shadow stack as a form of token.
>> +        * A token on shadow gives following properties
>> +        *      - Safe save and restore for shadow stack switching. Any save of shadow stack
>> +        *        must have had saved a token on shadow stack. Similarly any restore of shadow
>> +        *        stack must check the token before restore. Since writing to shadow stack with
>> +        *        address of shadow stack itself is not easily allowed. A restore without a save
>> +        *        is quite difficult for an attacker to perform.
>> +        *      - A natural break. A token in shadow stack provides a natural break in shadow stack
>> +        *        So a single linear range can be bucketed into different shadow stack segments. Any
>> +        *        sspopchk will detect the condition and fault to kernel as sw check exception.
>> +        */
>> +       if (is_shstk_enabled(current)) {
>> +               err |= save_user_shstk(current, &ss_ptr);
>> +               err |= __put_user(ss_ptr, &sc_cfi->ss_ptr);
>> +       }
>>         /* And put END __riscv_ctx_hdr at the end. */
>>         err |= __put_user(END_MAGIC, &sc_ext_ptr->magic);
>>         err |= __put_user(END_HDR_SIZE, &sc_ext_ptr->size);
>> @@ -345,6 +400,11 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
>>  #ifdef CONFIG_MMU
>>         regs->ra = (unsigned long)VDSO_SYMBOL(
>>                 current->mm->context.vdso, rt_sigreturn);
>> +
>> +       /* if bcfi is enabled x1 (ra) and x5 (t0) must match. not sure if we need this? */
>> +       if (is_shstk_enabled(current))
>> +               regs->t0 = regs->ra;
>> +
>>  #else
>>         /*
>>          * For the nommu case we don't have a VDSO.  Instead we push two
>> diff --git a/arch/riscv/kernel/usercfi.c b/arch/riscv/kernel/usercfi.c
>> index 8da509afdbe9..40c32258b6ec 100644
>> --- a/arch/riscv/kernel/usercfi.c
>> +++ b/arch/riscv/kernel/usercfi.c
>> @@ -52,6 +52,11 @@ void set_active_shstk(struct task_struct *task, unsigned long shstk_addr)
>>         task->thread_info.user_cfi_state.user_shdw_stk = shstk_addr;
>>  }
>>
>> +unsigned long get_active_shstk(struct task_struct *task)
>> +{
>> +       return task->thread_info.user_cfi_state.user_shdw_stk;
>> +}
>> +
>>  void set_shstk_status(struct task_struct *task, bool enable)
>>  {
>>         task->thread_info.user_cfi_state.ubcfi_en = enable ? 1 : 0;
>> @@ -164,6 +169,58 @@ static int create_rstor_token(unsigned long ssp, unsigned long *token_addr)
>>         return 0;
>>  }
>>
>> +/*
>> + * Save user shadow stack pointer on shadow stack itself and return pointer to saved location
>> + * returns -EFAULT if operation was unsuccessful
>> + */
>> +int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr)
>> +{
>> +       unsigned long ss_ptr = 0;
>> +       unsigned long token_loc = 0;
>> +       int ret = 0;
>> +
>> +       if (saved_shstk_ptr == NULL)
>> +               return -EINVAL;
>> +
>> +       ss_ptr = get_active_shstk(tsk);
>> +       ret = create_rstor_token(ss_ptr, &token_loc);
>> +
>> +       if (!ret) {
>> +               *saved_shstk_ptr = token_loc;
>> +               set_active_shstk(tsk, token_loc);
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/*
>> + * Restores user shadow stack pointer from token on shadow stack for task `tsk`
>> + * returns -EFAULT if operation was unsuccessful
>> + */
>> +int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr)
>> +{
>> +       unsigned long token = 0;
>> +
>> +       token = amo_user_shstk((unsigned long __user *)shstk_ptr, 0);
>> +
>> +       if (token == -1)
>> +               return -EFAULT;
>> +
>> +       /* invalid token, return EINVAL */
>> +       if ((token - shstk_ptr) != SHSTK_ENTRY_SIZE) {
>> +               pr_info_ratelimited(
>> +                               "%s[%d]: bad restore token in %s: pc=%p sp=%p, token=%p, shstk_ptr=%p\n",
>> +                               tsk->comm, task_pid_nr(tsk), __func__,
>> +                               (void *)(task_pt_regs(tsk)->epc), (void *)(task_pt_regs(tsk)->sp),
>> +                               (void *)token, (void *)shstk_ptr);
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* all checks passed, set active shstk and return success */
>> +       set_active_shstk(tsk, token);
>> +       return 0;
>> +}
>> +
>>  static unsigned long allocate_shadow_stack(unsigned long addr, unsigned long size,
>>                                 unsigned long token_offset,
>>                                 bool set_tok)
>> --
>> 2.45.0
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>- [1]: https://lore.kernel.org/all/20240628-dev-signal-refactor-v1-1-0c391b260261@sifive.com/
>
>Thanks,
>Andy
Andy Chiu Sept. 17, 2024, 10:03 p.m. UTC | #5
Deepak Gupta <debug@rivosinc.com> 於 2024年9月17日 週二 上午12:03寫道:
>
> On Fri, Sep 13, 2024 at 09:25:57PM +0200, Andy Chiu wrote:
> >Hi Deepak,
> >
> >Deepak Gupta <debug@rivosinc.com> 於 2024年9月13日 週五 上午1:20寫道:
> >>
> >> Save shadow stack pointer in sigcontext structure while delivering signal.
> >> Restore shadow stack pointer from sigcontext on sigreturn.
> >>
> >> As part of save operation, kernel uses `ssamoswap` to save snapshot of
> >> current shadow stack on shadow stack itself (can be called as a save
> >> token). During restore on sigreturn, kernel retrieves token from top of
> >> shadow stack and validates it. This allows that user mode can't arbitrary
> >> pivot to any shadow stack address without having a token and thus provide
> >> strong security assurance between signaly delivery and sigreturn window.
> >>
> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> >> Suggested-by: Andy Chiu <andy.chiu@sifive.com>
> >> ---
> >>  arch/riscv/include/asm/usercfi.h | 19 ++++++++++
> >>  arch/riscv/kernel/signal.c       | 62 +++++++++++++++++++++++++++++++-
> >>  arch/riscv/kernel/usercfi.c      | 57 +++++++++++++++++++++++++++++
> >>  3 files changed, 137 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/riscv/include/asm/usercfi.h b/arch/riscv/include/asm/usercfi.h
> >> index 20a9102cce51..d5050a5df26c 100644
> >> --- a/arch/riscv/include/asm/usercfi.h
> >> +++ b/arch/riscv/include/asm/usercfi.h
> >> @@ -8,6 +8,7 @@
> >>  #ifndef __ASSEMBLY__
> >>  #include <linux/types.h>
> >>  #include <linux/prctl.h>
> >> +#include <linux/errno.h>
> >>
> >>  struct task_struct;
> >>  struct kernel_clone_args;
> >> @@ -35,6 +36,9 @@ bool is_shstk_locked(struct task_struct *task);
> >>  bool is_shstk_allocated(struct task_struct *task);
> >>  void set_shstk_lock(struct task_struct *task);
> >>  void set_shstk_status(struct task_struct *task, bool enable);
> >> +unsigned long get_active_shstk(struct task_struct *task);
> >> +int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr);
> >> +int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr);
> >>  bool is_indir_lp_enabled(struct task_struct *task);
> >>  bool is_indir_lp_locked(struct task_struct *task);
> >>  void set_indir_lp_status(struct task_struct *task, bool enable);
> >> @@ -96,6 +100,21 @@ static inline void set_shstk_status(struct task_struct *task, bool enable)
> >>
> >>  }
> >>
> >> +static inline int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr)
> >> +{
> >> +       return -EINVAL;
> >> +}
> >> +
> >> +static inline int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr)
> >> +{
> >> +       return -EINVAL;
> >> +}
> >> +
> >> +static inline unsigned long get_active_shstk(struct task_struct *task)
> >> +{
> >> +       return 0;
> >> +}
> >> +
> >>  static inline bool is_indir_lp_enabled(struct task_struct *task)
> >>  {
> >>         return false;
> >> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> >> index dcd282419456..7d5c1825650f 100644
> >> --- a/arch/riscv/kernel/signal.c
> >> +++ b/arch/riscv/kernel/signal.c
> >> @@ -22,6 +22,7 @@
> >>  #include <asm/vector.h>
> >>  #include <asm/csr.h>
> >>  #include <asm/cacheflush.h>
> >> +#include <asm/usercfi.h>
> >>
> >>  unsigned long signal_minsigstksz __ro_after_init;
> >>
> >> @@ -153,6 +154,16 @@ static long restore_sigcontext(struct pt_regs *regs,
> >>         void __user *sc_ext_ptr = &sc->sc_extdesc.hdr;
> >>         __u32 rsvd;
> >>         long err;
> >> +       unsigned long ss_ptr = 0;
> >> +       struct __sc_riscv_cfi_state __user *sc_cfi = NULL;
> >> +
> >> +       sc_cfi = (struct __sc_riscv_cfi_state *)
> >> +                ((unsigned long) sc_ext_ptr + sizeof(struct __riscv_ctx_hdr));
> >> +
> >> +       if (has_vector() && riscv_v_vstate_query(regs))
> >> +               sc_cfi = (struct __sc_riscv_cfi_state *)
> >> +                        ((unsigned long) sc_cfi + riscv_v_sc_size);
> >> +
> >>         /* sc_regs is structured the same as the start of pt_regs */
> >>         err = __copy_from_user(regs, &sc->sc_regs, sizeof(sc->sc_regs));
> >>         if (unlikely(err))
> >> @@ -172,6 +183,24 @@ static long restore_sigcontext(struct pt_regs *regs,
> >>         if (unlikely(rsvd))
> >>                 return -EINVAL;
> >>
> >> +       /*
> >> +        * Restore shadow stack as a form of token stored on shadow stack itself as a safe
> >> +        * way to restore.
> >> +        * A token on shadow gives following properties
> >> +        *      - Safe save and restore for shadow stack switching. Any save of shadow stack
> >> +        *        must have had saved a token on shadow stack. Similarly any restore of shadow
> >> +        *        stack must check the token before restore. Since writing to shadow stack with
> >> +        *        address of shadow stack itself is not easily allowed. A restore without a save
> >> +        *        is quite difficult for an attacker to perform.
> >> +        *      - A natural break. A token in shadow stack provides a natural break in shadow stack
> >> +        *        So a single linear range can be bucketed into different shadow stack segments.
> >> +        *        sspopchk will detect the condition and fault to kernel as sw check exception.
> >> +        */
> >> +       if (is_shstk_enabled(current)) {
> >> +               err |= __copy_from_user(&ss_ptr, &sc_cfi->ss_ptr, sizeof(unsigned long));
> >> +               err |= restore_user_shstk(current, ss_ptr);
> >> +       }
> >> +
> >>         while (!err) {
> >>                 __u32 magic, size;
> >>                 struct __riscv_ctx_hdr __user *head = sc_ext_ptr;
> >> @@ -215,6 +244,10 @@ static size_t get_rt_frame_size(bool cal_all)
> >>                 if (cal_all || riscv_v_vstate_query(task_pt_regs(current)))
> >>                         total_context_size += riscv_v_sc_size;
> >>         }
> >> +
> >> +       if (is_shstk_enabled(current))
> >> +               total_context_size += sizeof(struct __sc_riscv_cfi_state);
> >> +
> >>         /*
> >>          * Preserved a __riscv_ctx_hdr for END signal context header if an
> >>          * extension uses __riscv_extra_ext_header
> >> @@ -276,18 +309,40 @@ static long setup_sigcontext(struct rt_sigframe __user *frame,
> >>  {
> >>         struct sigcontext __user *sc = &frame->uc.uc_mcontext;
> >>         struct __riscv_ctx_hdr __user *sc_ext_ptr = &sc->sc_extdesc.hdr;
> >> +       unsigned long ss_ptr = 0;
> >> +       struct __sc_riscv_cfi_state __user *sc_cfi = NULL;
> >>         long err;
> >>
> >> +       sc_cfi = (struct __sc_riscv_cfi_state *) (sc_ext_ptr + 1);
> >> +
> >
> >Is it intended that cfi sigcontext does not follow the sigcontext rule
> >setup by Vector? It seems like there is no extension header (struct
> >__riscv_ctx_hdr) defined for cfi sigcontext here. If the sigcontext is
> >directly appended to the signal stack, the user may not be able to
> >recognize the meaning without defining a new ABI.
>
> Hmm... I didn't realize that struct `struct __riscv_ctx_hdr` is strongly
> tied to vector state. I was under the impression that any new extended
> state addition would require this header to be present.

__riscv_ctx_hdr is not tied to vector state. Your impression is not
wrong. When sigcontext for Vector was designed, it is intended that
every new extension should define its header, please check
RISCV_V_MAGIC. The magic value and the size of the extension added to
the sigcontext are written into each hdr->magic and hdr->size.
However, I did not find the corresponding code in this patch. Or,
maybe I am missing something obvious. Could you help point me out it?

>
> cfi sigcontenxt doesn't need any ABI between user and kernel here. We need
> this space so that kernel can save a pointer to shadow stack token on signal
> delivery. Once sigreturn happens, kernel will use the same pointer, verify
> the token saved on shadow stack and restore shadow stack for user mode.
> At no point in this scheme, user mode is required to perform any action.
>
> All that is needed is that user mode doesn't accidenly trample at this offset.
>
> Since I was under the impression that `struct __riscv_ctx_hdr` is there for
> context extension and must be present for any state beyond `sc_regs`, I assumed
> that I must make space for this header (even if vector state is not present).
>
> >
> >BTW, I have sent a patch[1] that refactor setup_sigcontext so it'd be
> >easier for future extensions to expand on the signal stack.
>
> I can adopt to this, although its orthogonal to what we are discussing here.
>
> >
> >>         /* sc_regs is structured the same as the start of pt_regs */
> >>         err = __copy_to_user(&sc->sc_regs, regs, sizeof(sc->sc_regs));
> >>         /* Save the floating-point state. */
> >>         if (has_fpu())
> >>                 err |= save_fp_state(regs, &sc->sc_fpregs);
> >>         /* Save the vector state. */
> >> -       if (has_vector() && riscv_v_vstate_query(regs))
> >> +       if (has_vector() && riscv_v_vstate_query(regs)) {
> >>                 err |= save_v_state(regs, (void __user **)&sc_ext_ptr);
> >> +               sc_cfi = (struct __sc_riscv_cfi_state *) ((unsigned long) sc_cfi + riscv_v_sc_size);
> >> +       }
> >>         /* Write zero to fp-reserved space and check it on restore_sigcontext */
> >>         err |= __put_user(0, &sc->sc_extdesc.reserved);
> >> +       /*
> >> +        * Save a pointer to shadow stack itself on shadow stack as a form of token.
> >> +        * A token on shadow gives following properties
> >> +        *      - Safe save and restore for shadow stack switching. Any save of shadow stack
> >> +        *        must have had saved a token on shadow stack. Similarly any restore of shadow
> >> +        *        stack must check the token before restore. Since writing to shadow stack with
> >> +        *        address of shadow stack itself is not easily allowed. A restore without a save
> >> +        *        is quite difficult for an attacker to perform.
> >> +        *      - A natural break. A token in shadow stack provides a natural break in shadow stack
> >> +        *        So a single linear range can be bucketed into different shadow stack segments. Any
> >> +        *        sspopchk will detect the condition and fault to kernel as sw check exception.
> >> +        */
> >> +       if (is_shstk_enabled(current)) {
> >> +               err |= save_user_shstk(current, &ss_ptr);
> >> +               err |= __put_user(ss_ptr, &sc_cfi->ss_ptr);
> >> +       }
> >>         /* And put END __riscv_ctx_hdr at the end. */
> >>         err |= __put_user(END_MAGIC, &sc_ext_ptr->magic);
> >>         err |= __put_user(END_HDR_SIZE, &sc_ext_ptr->size);
> >> @@ -345,6 +400,11 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
> >>  #ifdef CONFIG_MMU
> >>         regs->ra = (unsigned long)VDSO_SYMBOL(
> >>                 current->mm->context.vdso, rt_sigreturn);
> >> +
> >> +       /* if bcfi is enabled x1 (ra) and x5 (t0) must match. not sure if we need this? */
> >> +       if (is_shstk_enabled(current))
> >> +               regs->t0 = regs->ra;
> >> +
> >>  #else
> >>         /*
> >>          * For the nommu case we don't have a VDSO.  Instead we push two
> >> diff --git a/arch/riscv/kernel/usercfi.c b/arch/riscv/kernel/usercfi.c
> >> index 8da509afdbe9..40c32258b6ec 100644
> >> --- a/arch/riscv/kernel/usercfi.c
> >> +++ b/arch/riscv/kernel/usercfi.c
> >> @@ -52,6 +52,11 @@ void set_active_shstk(struct task_struct *task, unsigned long shstk_addr)
> >>         task->thread_info.user_cfi_state.user_shdw_stk = shstk_addr;
> >>  }
> >>
> >> +unsigned long get_active_shstk(struct task_struct *task)
> >> +{
> >> +       return task->thread_info.user_cfi_state.user_shdw_stk;
> >> +}
> >> +
> >>  void set_shstk_status(struct task_struct *task, bool enable)
> >>  {
> >>         task->thread_info.user_cfi_state.ubcfi_en = enable ? 1 : 0;
> >> @@ -164,6 +169,58 @@ static int create_rstor_token(unsigned long ssp, unsigned long *token_addr)
> >>         return 0;
> >>  }
> >>
> >> +/*
> >> + * Save user shadow stack pointer on shadow stack itself and return pointer to saved location
> >> + * returns -EFAULT if operation was unsuccessful
> >> + */
> >> +int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr)
> >> +{
> >> +       unsigned long ss_ptr = 0;
> >> +       unsigned long token_loc = 0;
> >> +       int ret = 0;
> >> +
> >> +       if (saved_shstk_ptr == NULL)
> >> +               return -EINVAL;
> >> +
> >> +       ss_ptr = get_active_shstk(tsk);
> >> +       ret = create_rstor_token(ss_ptr, &token_loc);
> >> +
> >> +       if (!ret) {
> >> +               *saved_shstk_ptr = token_loc;
> >> +               set_active_shstk(tsk, token_loc);
> >> +       }
> >> +
> >> +       return ret;
> >> +}
> >> +
> >> +/*
> >> + * Restores user shadow stack pointer from token on shadow stack for task `tsk`
> >> + * returns -EFAULT if operation was unsuccessful
> >> + */
> >> +int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr)
> >> +{
> >> +       unsigned long token = 0;
> >> +
> >> +       token = amo_user_shstk((unsigned long __user *)shstk_ptr, 0);
> >> +
> >> +       if (token == -1)
> >> +               return -EFAULT;
> >> +
> >> +       /* invalid token, return EINVAL */
> >> +       if ((token - shstk_ptr) != SHSTK_ENTRY_SIZE) {
> >> +               pr_info_ratelimited(
> >> +                               "%s[%d]: bad restore token in %s: pc=%p sp=%p, token=%p, shstk_ptr=%p\n",
> >> +                               tsk->comm, task_pid_nr(tsk), __func__,
> >> +                               (void *)(task_pt_regs(tsk)->epc), (void *)(task_pt_regs(tsk)->sp),
> >> +                               (void *)token, (void *)shstk_ptr);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       /* all checks passed, set active shstk and return success */
> >> +       set_active_shstk(tsk, token);
> >> +       return 0;
> >> +}
> >> +
> >>  static unsigned long allocate_shadow_stack(unsigned long addr, unsigned long size,
> >>                                 unsigned long token_offset,
> >>                                 bool set_tok)
> >> --
> >> 2.45.0
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> >- [1]: https://lore.kernel.org/all/20240628-dev-signal-refactor-v1-1-0c391b260261@sifive.com/
> >
> >Thanks,
> >Andy

Regards,
Andy
Deepak Gupta Sept. 17, 2024, 10:52 p.m. UTC | #6
On Wed, Sep 18, 2024 at 12:03:45AM +0200, Andy Chiu wrote:
>Deepak Gupta <debug@rivosinc.com> 於 2024年9月17日 週二 上午12:03寫道:
>>
>> On Fri, Sep 13, 2024 at 09:25:57PM +0200, Andy Chiu wrote:
>> >Hi Deepak,
>> >
>> >Deepak Gupta <debug@rivosinc.com> 於 2024年9月13日 週五 上午1:20寫道:
>> >>
>> >> Save shadow stack pointer in sigcontext structure while delivering signal.
>> >> Restore shadow stack pointer from sigcontext on sigreturn.
>> >>
>> >> As part of save operation, kernel uses `ssamoswap` to save snapshot of
>> >> current shadow stack on shadow stack itself (can be called as a save
>> >> token). During restore on sigreturn, kernel retrieves token from top of
>> >> shadow stack and validates it. This allows that user mode can't arbitrary
>> >> pivot to any shadow stack address without having a token and thus provide
>> >> strong security assurance between signaly delivery and sigreturn window.
>> >>
>> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>> >> Suggested-by: Andy Chiu <andy.chiu@sifive.com>
>> >> ---
>> >>  arch/riscv/include/asm/usercfi.h | 19 ++++++++++
>> >>  arch/riscv/kernel/signal.c       | 62 +++++++++++++++++++++++++++++++-
>> >>  arch/riscv/kernel/usercfi.c      | 57 +++++++++++++++++++++++++++++
>> >>  3 files changed, 137 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/arch/riscv/include/asm/usercfi.h b/arch/riscv/include/asm/usercfi.h
>> >> index 20a9102cce51..d5050a5df26c 100644
>> >> --- a/arch/riscv/include/asm/usercfi.h
>> >> +++ b/arch/riscv/include/asm/usercfi.h
>> >> @@ -8,6 +8,7 @@
>> >>  #ifndef __ASSEMBLY__
>> >>  #include <linux/types.h>
>> >>  #include <linux/prctl.h>
>> >> +#include <linux/errno.h>
>> >>
>> >>  struct task_struct;
>> >>  struct kernel_clone_args;
>> >> @@ -35,6 +36,9 @@ bool is_shstk_locked(struct task_struct *task);
>> >>  bool is_shstk_allocated(struct task_struct *task);
>> >>  void set_shstk_lock(struct task_struct *task);
>> >>  void set_shstk_status(struct task_struct *task, bool enable);
>> >> +unsigned long get_active_shstk(struct task_struct *task);
>> >> +int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr);
>> >> +int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr);
>> >>  bool is_indir_lp_enabled(struct task_struct *task);
>> >>  bool is_indir_lp_locked(struct task_struct *task);
>> >>  void set_indir_lp_status(struct task_struct *task, bool enable);
>> >> @@ -96,6 +100,21 @@ static inline void set_shstk_status(struct task_struct *task, bool enable)
>> >>
>> >>  }
>> >>
>> >> +static inline int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr)
>> >> +{
>> >> +       return -EINVAL;
>> >> +}
>> >> +
>> >> +static inline int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr)
>> >> +{
>> >> +       return -EINVAL;
>> >> +}
>> >> +
>> >> +static inline unsigned long get_active_shstk(struct task_struct *task)
>> >> +{
>> >> +       return 0;
>> >> +}
>> >> +
>> >>  static inline bool is_indir_lp_enabled(struct task_struct *task)
>> >>  {
>> >>         return false;
>> >> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
>> >> index dcd282419456..7d5c1825650f 100644
>> >> --- a/arch/riscv/kernel/signal.c
>> >> +++ b/arch/riscv/kernel/signal.c
>> >> @@ -22,6 +22,7 @@
>> >>  #include <asm/vector.h>
>> >>  #include <asm/csr.h>
>> >>  #include <asm/cacheflush.h>
>> >> +#include <asm/usercfi.h>
>> >>
>> >>  unsigned long signal_minsigstksz __ro_after_init;
>> >>
>> >> @@ -153,6 +154,16 @@ static long restore_sigcontext(struct pt_regs *regs,
>> >>         void __user *sc_ext_ptr = &sc->sc_extdesc.hdr;
>> >>         __u32 rsvd;
>> >>         long err;
>> >> +       unsigned long ss_ptr = 0;
>> >> +       struct __sc_riscv_cfi_state __user *sc_cfi = NULL;
>> >> +
>> >> +       sc_cfi = (struct __sc_riscv_cfi_state *)
>> >> +                ((unsigned long) sc_ext_ptr + sizeof(struct __riscv_ctx_hdr));
>> >> +
>> >> +       if (has_vector() && riscv_v_vstate_query(regs))
>> >> +               sc_cfi = (struct __sc_riscv_cfi_state *)
>> >> +                        ((unsigned long) sc_cfi + riscv_v_sc_size);
>> >> +
>> >>         /* sc_regs is structured the same as the start of pt_regs */
>> >>         err = __copy_from_user(regs, &sc->sc_regs, sizeof(sc->sc_regs));
>> >>         if (unlikely(err))
>> >> @@ -172,6 +183,24 @@ static long restore_sigcontext(struct pt_regs *regs,
>> >>         if (unlikely(rsvd))
>> >>                 return -EINVAL;
>> >>
>> >> +       /*
>> >> +        * Restore shadow stack as a form of token stored on shadow stack itself as a safe
>> >> +        * way to restore.
>> >> +        * A token on shadow gives following properties
>> >> +        *      - Safe save and restore for shadow stack switching. Any save of shadow stack
>> >> +        *        must have had saved a token on shadow stack. Similarly any restore of shadow
>> >> +        *        stack must check the token before restore. Since writing to shadow stack with
>> >> +        *        address of shadow stack itself is not easily allowed. A restore without a save
>> >> +        *        is quite difficult for an attacker to perform.
>> >> +        *      - A natural break. A token in shadow stack provides a natural break in shadow stack
>> >> +        *        So a single linear range can be bucketed into different shadow stack segments.
>> >> +        *        sspopchk will detect the condition and fault to kernel as sw check exception.
>> >> +        */
>> >> +       if (is_shstk_enabled(current)) {
>> >> +               err |= __copy_from_user(&ss_ptr, &sc_cfi->ss_ptr, sizeof(unsigned long));
>> >> +               err |= restore_user_shstk(current, ss_ptr);
>> >> +       }
>> >> +
>> >>         while (!err) {
>> >>                 __u32 magic, size;
>> >>                 struct __riscv_ctx_hdr __user *head = sc_ext_ptr;
>> >> @@ -215,6 +244,10 @@ static size_t get_rt_frame_size(bool cal_all)
>> >>                 if (cal_all || riscv_v_vstate_query(task_pt_regs(current)))
>> >>                         total_context_size += riscv_v_sc_size;
>> >>         }
>> >> +
>> >> +       if (is_shstk_enabled(current))
>> >> +               total_context_size += sizeof(struct __sc_riscv_cfi_state);
>> >> +
>> >>         /*
>> >>          * Preserved a __riscv_ctx_hdr for END signal context header if an
>> >>          * extension uses __riscv_extra_ext_header
>> >> @@ -276,18 +309,40 @@ static long setup_sigcontext(struct rt_sigframe __user *frame,
>> >>  {
>> >>         struct sigcontext __user *sc = &frame->uc.uc_mcontext;
>> >>         struct __riscv_ctx_hdr __user *sc_ext_ptr = &sc->sc_extdesc.hdr;
>> >> +       unsigned long ss_ptr = 0;
>> >> +       struct __sc_riscv_cfi_state __user *sc_cfi = NULL;
>> >>         long err;
>> >>
>> >> +       sc_cfi = (struct __sc_riscv_cfi_state *) (sc_ext_ptr + 1);
>> >> +
>> >
>> >Is it intended that cfi sigcontext does not follow the sigcontext rule
>> >setup by Vector? It seems like there is no extension header (struct
>> >__riscv_ctx_hdr) defined for cfi sigcontext here. If the sigcontext is
>> >directly appended to the signal stack, the user may not be able to
>> >recognize the meaning without defining a new ABI.
>>
>> Hmm... I didn't realize that struct `struct __riscv_ctx_hdr` is strongly
>> tied to vector state. I was under the impression that any new extended
>> state addition would require this header to be present.
>
>__riscv_ctx_hdr is not tied to vector state. Your impression is not
>wrong. When sigcontext for Vector was designed, it is intended that
>every new extension should define its header, please check
>RISCV_V_MAGIC. The magic value and the size of the extension added to
>the sigcontext are written into each hdr->magic and hdr->size.
>However, I did not find the corresponding code in this patch. Or,
>maybe I am missing something obvious. Could you help point me out it?

Sorry I was under the impression that there is only one ctx header for
all extended state. It seems like from this conversation, any new state
must declare it's own header, magic word and size.

Now that I am having this conversation, it seems like that the idea for
having ctx header is to ensure that any software (user space or kernel)
must parse sigcontext beyong pt_regs iteratively and start poking only
when it sees relevant data structure (based on magic word?)

Hopefully, I got it right this time. I'll fix it, if that's the intention
here.

>
>>
>> cfi sigcontenxt doesn't need any ABI between user and kernel here. We need
>> this space so that kernel can save a pointer to shadow stack token on signal
>> delivery. Once sigreturn happens, kernel will use the same pointer, verify
>> the token saved on shadow stack and restore shadow stack for user mode.
>> At no point in this scheme, user mode is required to perform any action.
>>
>> All that is needed is that user mode doesn't accidenly trample at this offset.
>>
>> Since I was under the impression that `struct __riscv_ctx_hdr` is there for
>> context extension and must be present for any state beyond `sc_regs`, I assumed
>> that I must make space for this header (even if vector state is not present).
>>
>> >
>> >BTW, I have sent a patch[1] that refactor setup_sigcontext so it'd be
>> >easier for future extensions to expand on the signal stack.
>>
>> I can adopt to this, although its orthogonal to what we are discussing here.
>>
>> >
>> >>         /* sc_regs is structured the same as the start of pt_regs */
>> >>         err = __copy_to_user(&sc->sc_regs, regs, sizeof(sc->sc_regs));
>> >>         /* Save the floating-point state. */
>> >>         if (has_fpu())
>> >>                 err |= save_fp_state(regs, &sc->sc_fpregs);
>> >>         /* Save the vector state. */
>> >> -       if (has_vector() && riscv_v_vstate_query(regs))
>> >> +       if (has_vector() && riscv_v_vstate_query(regs)) {
>> >>                 err |= save_v_state(regs, (void __user **)&sc_ext_ptr);
>> >> +               sc_cfi = (struct __sc_riscv_cfi_state *) ((unsigned long) sc_cfi + riscv_v_sc_size);
>> >> +       }
>> >>         /* Write zero to fp-reserved space and check it on restore_sigcontext */
>> >>         err |= __put_user(0, &sc->sc_extdesc.reserved);
>> >> +       /*
>> >> +        * Save a pointer to shadow stack itself on shadow stack as a form of token.
>> >> +        * A token on shadow gives following properties
>> >> +        *      - Safe save and restore for shadow stack switching. Any save of shadow stack
>> >> +        *        must have had saved a token on shadow stack. Similarly any restore of shadow
>> >> +        *        stack must check the token before restore. Since writing to shadow stack with
>> >> +        *        address of shadow stack itself is not easily allowed. A restore without a save
>> >> +        *        is quite difficult for an attacker to perform.
>> >> +        *      - A natural break. A token in shadow stack provides a natural break in shadow stack
>> >> +        *        So a single linear range can be bucketed into different shadow stack segments. Any
>> >> +        *        sspopchk will detect the condition and fault to kernel as sw check exception.
>> >> +        */
>> >> +       if (is_shstk_enabled(current)) {
>> >> +               err |= save_user_shstk(current, &ss_ptr);
>> >> +               err |= __put_user(ss_ptr, &sc_cfi->ss_ptr);
>> >> +       }
>> >>         /* And put END __riscv_ctx_hdr at the end. */
>> >>         err |= __put_user(END_MAGIC, &sc_ext_ptr->magic);
>> >>         err |= __put_user(END_HDR_SIZE, &sc_ext_ptr->size);
>> >> @@ -345,6 +400,11 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
>> >>  #ifdef CONFIG_MMU
>> >>         regs->ra = (unsigned long)VDSO_SYMBOL(
>> >>                 current->mm->context.vdso, rt_sigreturn);
>> >> +
>> >> +       /* if bcfi is enabled x1 (ra) and x5 (t0) must match. not sure if we need this? */
>> >> +       if (is_shstk_enabled(current))
>> >> +               regs->t0 = regs->ra;
>> >> +
>> >>  #else
>> >>         /*
>> >>          * For the nommu case we don't have a VDSO.  Instead we push two
>> >> diff --git a/arch/riscv/kernel/usercfi.c b/arch/riscv/kernel/usercfi.c
>> >> index 8da509afdbe9..40c32258b6ec 100644
>> >> --- a/arch/riscv/kernel/usercfi.c
>> >> +++ b/arch/riscv/kernel/usercfi.c
>> >> @@ -52,6 +52,11 @@ void set_active_shstk(struct task_struct *task, unsigned long shstk_addr)
>> >>         task->thread_info.user_cfi_state.user_shdw_stk = shstk_addr;
>> >>  }
>> >>
>> >> +unsigned long get_active_shstk(struct task_struct *task)
>> >> +{
>> >> +       return task->thread_info.user_cfi_state.user_shdw_stk;
>> >> +}
>> >> +
>> >>  void set_shstk_status(struct task_struct *task, bool enable)
>> >>  {
>> >>         task->thread_info.user_cfi_state.ubcfi_en = enable ? 1 : 0;
>> >> @@ -164,6 +169,58 @@ static int create_rstor_token(unsigned long ssp, unsigned long *token_addr)
>> >>         return 0;
>> >>  }
>> >>
>> >> +/*
>> >> + * Save user shadow stack pointer on shadow stack itself and return pointer to saved location
>> >> + * returns -EFAULT if operation was unsuccessful
>> >> + */
>> >> +int save_user_shstk(struct task_struct *tsk, unsigned long *saved_shstk_ptr)
>> >> +{
>> >> +       unsigned long ss_ptr = 0;
>> >> +       unsigned long token_loc = 0;
>> >> +       int ret = 0;
>> >> +
>> >> +       if (saved_shstk_ptr == NULL)
>> >> +               return -EINVAL;
>> >> +
>> >> +       ss_ptr = get_active_shstk(tsk);
>> >> +       ret = create_rstor_token(ss_ptr, &token_loc);
>> >> +
>> >> +       if (!ret) {
>> >> +               *saved_shstk_ptr = token_loc;
>> >> +               set_active_shstk(tsk, token_loc);
>> >> +       }
>> >> +
>> >> +       return ret;
>> >> +}
>> >> +
>> >> +/*
>> >> + * Restores user shadow stack pointer from token on shadow stack for task `tsk`
>> >> + * returns -EFAULT if operation was unsuccessful
>> >> + */
>> >> +int restore_user_shstk(struct task_struct *tsk, unsigned long shstk_ptr)
>> >> +{
>> >> +       unsigned long token = 0;
>> >> +
>> >> +       token = amo_user_shstk((unsigned long __user *)shstk_ptr, 0);
>> >> +
>> >> +       if (token == -1)
>> >> +               return -EFAULT;
>> >> +
>> >> +       /* invalid token, return EINVAL */
>> >> +       if ((token - shstk_ptr) != SHSTK_ENTRY_SIZE) {
>> >> +               pr_info_ratelimited(
>> >> +                               "%s[%d]: bad restore token in %s: pc=%p sp=%p, token=%p, shstk_ptr=%p\n",
>> >> +                               tsk->comm, task_pid_nr(tsk), __func__,
>> >> +                               (void *)(task_pt_regs(tsk)->epc), (void *)(task_pt_regs(tsk)->sp),
>> >> +                               (void *)token, (void *)shstk_ptr);
>> >> +               return -EINVAL;
>> >> +       }
>> >> +
>> >> +       /* all checks passed, set active shstk and return success */
>> >> +       set_active_shstk(tsk, token);
>> >> +       return 0;
>> >> +}
>> >> +
>> >>  static unsigned long allocate_shadow_stack(unsigned long addr, unsigned long size,
>> >>                                 unsigned long token_offset,
>> >>                                 bool set_tok)
>> >> --
>> >> 2.45.0
>> >>
>> >>
>> >> _______________________________________________
>> >> linux-riscv mailing list
>> >> linux-riscv@lists.infradead.org
>> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
>> >
>> >- [1]: https://lore.kernel.org/all/20240628-dev-signal-refactor-v1-1-0c391b260261@sifive.com/
>> >
>> >Thanks,
>> >Andy
>
>Regards,
>Andy