Re: [PATCH v2] riscv: Discard vector state on syscalls

[Andy Chiu]

On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <bjorn@xxxxxxxxxx> wrote:
>
> From: Björn Töpel <bjorn@xxxxxxxxxxxx>
>
> The RISC-V vector specification states:
>   Executing a system call causes all caller-saved vector registers
>   (v0-v31, vl, vtype) and vstart to become unspecified.
>
> The vector registers are set to all 1s, vill is set (invalid), and the
> vector status is set to Initial.
>
> That way we can prevent userspace from accidentally relying on the
> stated save.
>
> Rémi pointed out [1] that writing to the registers might be
> superfluous, and setting vill is sufficient.
>
> Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@xxxxxxxxxxxxxxxxx/ # [1]
> Suggested-by: Darius Rad <darius@xxxxxxxxxxxx>
> Suggested-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
> Suggested-by: Rémi Denis-Courmont <remi@xxxxxxxxxx>
> Signed-off-by: Björn Töpel <bjorn@xxxxxxxxxxxx>
> ---
> v1->v2:
>   Proper register restore for initial state (Andy)
>   Set registers to 1s, and not 0s (Darius)
> ---
>  arch/riscv/include/asm/vector.h | 42 ++++++++++++++++++++++++++++++---
>  arch/riscv/kernel/traps.c       |  2 ++
>  2 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index 04c0b07bf6cd..93d702d9988c 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -139,14 +139,49 @@ static inline void riscv_v_vstate_save(struct task_struct *task,
>         }
>  }
>
> +static inline void __riscv_v_vstate_discard(void)
> +{
> +       unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1);
> +
> +       riscv_v_enable();
> +       asm volatile (
> +               ".option push\n\t"
> +               ".option arch, +v\n\t"
> +               "vsetvli        %0, x0, e8, m8, ta, ma\n\t"
> +               "vmv.v.i        v0, -1\n\t"
> +               "vmv.v.i        v8, -1\n\t"
> +               "vmv.v.i        v16, -1\n\t"
> +               "vmv.v.i        v24, -1\n\t"
> +               "vsetvl         %0, x0, %1\n\t"
> +               ".option pop\n\t"
> +               : "=&r" (vl) : "r" (vtype_inval) : "memory");
> +       riscv_v_disable();
> +}
> +
> +static inline void riscv_v_vstate_discard(struct pt_regs *regs)
> +{
> +       if (!riscv_v_vstate_query(regs))
> +               return;
> +
> +       __riscv_v_vstate_discard();
> +       riscv_v_vstate_on(regs);
> +}
> +
>  static inline void riscv_v_vstate_restore(struct task_struct *task,
>                                           struct pt_regs *regs)
>  {
> -       if ((regs->status & SR_VS) != SR_VS_OFF) {
> -               struct __riscv_v_ext_state *vstate = &task->thread.vstate;
> -
> +       struct __riscv_v_ext_state *vstate = &task->thread.vstate;
> +       unsigned long status = regs->status & SR_VS;
> +
> +       switch (status) {
> +       case SR_VS_INITIAL:
> +               __riscv_v_vstate_discard();
> +               break;
> +       case SR_VS_CLEAN:
> +       case SR_VS_DIRTY:
>                 __riscv_v_vstate_restore(vstate, vstate->datap);
>                 __riscv_v_vstate_clean(regs);
> +               break;
>         }
>  }
>
> @@ -178,6 +213,7 @@ static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; }
>  #define __switch_to_vector(__prev, __next)     do {} while (0)
>  #define riscv_v_vstate_off(regs)               do {} while (0)
>  #define riscv_v_vstate_on(regs)                        do {} while (0)
> +#define riscv_v_vstate_discard(regs)           do {} while (0)
>
>  #endif /* CONFIG_RISCV_ISA_V */
>
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 5158961ea977..5ff63a784a6d 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -296,6 +296,8 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
>                 regs->epc += 4;
>                 regs->orig_a0 = regs->a0;
>
> +               riscv_v_vstate_discard(regs);
> +
>                 syscall = syscall_enter_from_user_mode(regs, syscall);
>
>                 if (syscall < NR_syscalls)
>
> base-commit: 488833ccdcac118da16701f4ee0673b20ba47fe3
> --
> 2.39.2
>

Hi, the above part looks good to me. In the context of kernel-mode
vector, it would also be good to just discard V-context at the syscall
entry. So the kernel can freely use Vector if needed. I will rebase my
work on top of yours.

Another part that just came into my mind is the one for ptrace. Do we
need to disallow, or immediately return all -1 if the tracee process
is in the syscall path? It seems that we are likely to get stale
values on datap if a tracee is being traced during a syscall.

Thanks,
Andy