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

[Darius Rad]

On Thu, Jun 22, 2023 at 10:46:31AM -0700, Palmer Dabbelt wrote:
> On Thu, 22 Jun 2023 10:36:13 PDT (-0700), 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 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@xxxxxxxxxxxxxxxxx/ # [1]
> > Suggested-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
> > Suggested-by: Rémi Denis-Courmont <remi@xxxxxxxxxx>
> > Signed-off-by: Björn Töpel <bjorn@xxxxxxxxxxxx>
> > ---
> > 
> > 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.

I think all 1s would be preferred, but I don't think it's particularly
critical (splitting hairs, like you said), so I'll let you make the call.  

> 
> 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