[PATCH] RISC-V: Clobber V registers on syscalls

[Björn Töpel]

Andy Chiu <andy.chiu at sifive.com> writes:

> On Mon, Jun 26, 2023 at 11:36 PM Björn Töpel <bjorn at kernel.org> wrote:
>>
>> Andy Chiu <andy.chiu at sifive.com> writes:
>>
>> >> > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> >> > index 24d309c6ab8d..e36b69c9b07f 100644
>> >> > --- a/arch/riscv/kernel/traps.c
>> >> > +++ b/arch/riscv/kernel/traps.c
>> >> > @@ -291,10 +291,14 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
>> >> >  {
>> >> >       if (user_mode(regs)) {
>> >> >               ulong syscall = regs->a7;
>> >> > +             bool v_is_on;
>> >> >
>> >> >               regs->epc += 4;
>> >> >               regs->orig_a0 = regs->a0;
>> >> >
>> >> > +             v_is_on = riscv_v_vstate_query(regs);
>> >> > +             riscv_v_vstate_off(regs);
>> >> > +
>> >> >               syscall = syscall_enter_from_user_mode(regs, syscall);
>> >> >
>> >> >               if (syscall < NR_syscalls)
>> >> > @@ -303,6 +307,10 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
>> >> >                       regs->a0 = -ENOSYS;
>> >> >
>> >> >               syscall_exit_to_user_mode(regs);
>> >> > +             if (v_is_on) {
>> >> > +                     riscv_v_vstate_discard(regs);
>> >> > +                     riscv_v_vstate_dirty(regs);
>> >>
>> >> Ah! Neat! Why dirty, instead of just keeping the "set to Initial" from
>> >> my diff?
>> >
>> > Both work, I think. But here if we set it to "on" after discarding
>> > V-regs, then take a context switch before executing any V instructions
>> > in user space (does not change future vstate to dirty). Then we will
>> > leak V-regs previously set into its vstate.datap after switching back,
>> > because we only save V context if vstate is dirty. So, I think setting
>> > vstate to dirty is a safer option.
>>
>> Ah, yes, good point. An alternative variant is this:
>>
>> ---
>> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
>> index 04c0b07bf6cd..32b6115a54a5 100644
>> --- a/arch/riscv/include/asm/vector.h
>> +++ b/arch/riscv/include/asm/vector.h
>> @@ -139,15 +139,51 @@ 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, 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();
>> +}
>> +
>> +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) {
>> +       unsigned long status = regs->status & SR_VS;
>> +
>> +       WARN_ON(status == SR_VS_DIRTY);
>> +
>> +       if (status == SR_VS_CLEAN) {
>>                 struct __riscv_v_ext_state *vstate = &task->thread.vstate;
>>
>>                 __riscv_v_vstate_restore(vstate, vstate->datap);
>>                 __riscv_v_vstate_clean(regs);
>> +               return;
>>         }
>> +
>> +       if (status == SR_VS_INITIAL)
>> +               __riscv_v_vstate_discard();
>>  }
>>
>>  static inline void __switch_to_vector(struct task_struct *prev,
>> @@ -178,6 +214,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)
>>
>> ---
>>
>>
>> Here, we simply discard the regs if the state is Initial. Thoughts?
>>
>>
>> Björn
>
> Yes, it makes sense to me to handle the initial state in vstate_restore.

Ok! I sent out a proper v2, but without the WARN_ON to match the
behavior of the the original code.

PTAL, and let me know what you think.


Björn