| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <mhng-4c2014dc-5e23-4fb8-b5cc-d70f29d905d5@palmer-ri-x1c9a>
Date: Thu, 22 Jun 2023 11:02:08 -0700 (PDT)
From: Palmer Dabbelt <palmer@...belt.com>
To: Darius Rad <darius@...espec.com>
CC: bjorn@...nel.org, Paul Walmsley <paul.walmsley@...ive.com>,
aou@...s.berkeley.edu, linux-riscv@...ts.infradead.org,
Bjorn Topel <bjorn@...osinc.com>, linux-kernel@...r.kernel.org,
linux@...osinc.com, remi@...lab.net, andy.chiu@...ive.com
Subject: Re: [PATCH] riscv: Discard vector state on syscalls
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@...nel.org wrote:
>> > From: Björn Töpel <bjorn@...osinc.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@...osinc.com>
>> > Suggested-by: Rémi Denis-Courmont <remi@...lab.net>
>> > Signed-off-by: Björn Töpel <bjorn@...osinc.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.
>
> 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@...osinc.com.
> 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.
Powered by blists - more mailing lists