Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Jun 2023 10:46:31 -0700 (PDT)
Subject:     Re: [PATCH] riscv: Discard vector state on syscalls
In-Reply-To: <20230622173613.30722-1-bjorn@kernel.org>
CC:     Paul Walmsley <paul.walmsley@sifive.com>, aou@eecs.berkeley.edu,
        linux-riscv@lists.infradead.org, Bjorn Topel <bjorn@rivosinc.com>,
        linux-kernel@vger.kernel.org, linux@rivosinc.com, remi@remlab.net,
        Darius Rad <darius@bluespec.com>, andy.chiu@sifive.com
From:   Palmer Dabbelt <palmer@dabbelt.com>
To:     bjorn@kernel.org
Message-ID: <mhng-5f8ae912-3b56-4eda-bdfa-1de31a7c93e0@palmer-ri-x1c9a>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 22 Jun 2023 10:36:13 PDT (-0700), bjorn@kernel.org wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
>
> 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 cleared, 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 clearing the registers might be superfluous,
> and setting vill is sufficient.
>
> Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@basile.remlab.net/ # [1]
> Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
> Suggested-by: Rémi Denis-Courmont <remi@remlab.net>
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>
> I figured I'd sent out a proper patch. I like Andy's optimization
> patch, but TBH I think we should do that as a follow up.
>
> As Rémi pointed out, the clearing might be opted out, but I left it in
> here.

I think we're going to end up with a bunch of uarch-specific stuff here, 
but for now having the heavy hammer seems safest.  There's no V hardware 
yet so we can't really tell how anything performs, at least this way 
we're definately not leaking anything.

So I'm OK with this.  I'd also be fine with clearing to all-1s, I think 
it's kind of splitting hairs at this point: the 1s are nice because 
they're what the rest of V does, but setting vill should make everything 
trap anyway so maybe it doesn't matter -- and it's not clear if 1 or 0 
will allow initial, so who knows.

Darius: I'm cool swapping over to the 1s if you feel strongly about it.  
Bjorn says Sweeden is on vacation, so just LMK and I'll re-spin it with 
the 1s.

Regardless I'd like to pick up something that blows away V state for 
this merge window, as it'll make sure the uABI is quite strictly 
enforced.

> Björn
>
> ---
>  arch/riscv/include/asm/vector.h | 25 +++++++++++++++++++++++++
>  arch/riscv/kernel/traps.c       |  2 ++
>  2 files changed, 27 insertions(+)
>
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index 04c0b07bf6cd..692ce55e4a69 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -163,6 +163,30 @@ static inline void __switch_to_vector(struct task_struct *prev,
>  void riscv_v_vstate_ctrl_init(struct task_struct *tsk);
>  bool riscv_v_vstate_ctrl_user_allowed(void);
>
> +static inline void riscv_v_vstate_discard(struct pt_regs *regs)
> +{
> +	unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1);
> +
> +	if (!riscv_v_vstate_query(regs))
> +		return;
> +
> +	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, 0\n\t"
> +		"vmv.v.i	v8, 0\n\t"
> +		"vmv.v.i	v16, 0\n\t"
> +		"vmv.v.i	v24, 0\n\t"
> +		"vsetvl		%0, x0, %1\n\t"
> +		".option pop\n\t"
> +		: "=&r" (vl) : "r" (vtype_inval) : "memory");
> +	riscv_v_disable();
> +
> +	riscv_v_vstate_on(regs);
> +}
> +
>  #else /* ! CONFIG_RISCV_ISA_V  */
>
>  struct pt_regs;
> @@ -178,6 +202,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 05ffdcd1424e..00c68b57ff88 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -295,6 +295,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: 4681dacadeefa5ca6017e00736adc1d7dc963c6a