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

[Palmer Dabbelt]

On Thu, 22 Jun 2023 10:55:54 PDT (-0700), Darius Rad wrote:
> 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.  

OK, I'm just going to take this then as it's the less work option ;)

> > 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
>
> --
> You received this message because you are subscribed to the Google Groups "linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux+unsubscribe@xxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/a/rivosinc.com/d/msgid/linux/ZJSLKrB/4xaYB75d%40bruce.bluespec.com.
> For more options, visit https://groups.google.com/a/rivosinc.com/d/optout.