[PATCH v2 6/6] riscv: vector: initialize vlenb on the first context switch

Sergey Matyukevich posted 6 patches 4 months ago
There is a newer version of this series
[PATCH v2 6/6] riscv: vector: initialize vlenb on the first context switch
Posted by Sergey Matyukevich 4 months ago
The vstate in thread_struct is zeroed when the vector context is
initialized. That includes read-only register vlenb, which holds
the vector register length in bytes. This zeroed state persists
until mstatus.VS becomes 'dirty' and a context switch saves the
actual hardware values.

This can expose the zero vlenb value to the user-space in early
debug scenarios, e.g. when ptrace attaches to a traced process
early, before any vector instruction except the first one was
executed.

Fix this by forcing the vector context save on the first context switch.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
---
 arch/riscv/kernel/vector.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
index 901e67adf576..3dd22a71aa18 100644
--- a/arch/riscv/kernel/vector.c
+++ b/arch/riscv/kernel/vector.c
@@ -120,6 +120,7 @@ static int riscv_v_thread_zalloc(struct kmem_cache *cache,
 
 	ctx->datap = datap;
 	memset(ctx, 0, offsetof(struct __riscv_v_ext_state, datap));
+
 	return 0;
 }
 
@@ -216,8 +217,11 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
 		force_sig(SIGBUS);
 		return true;
 	}
+
 	riscv_v_vstate_on(regs);
 	riscv_v_vstate_set_restore(current, regs);
+	set_tsk_thread_flag(current, TIF_RISCV_V_FORCE_SAVE);
+
 	return true;
 }
 
-- 
2.51.0
Re: [PATCH v2 6/6] riscv: vector: initialize vlenb on the first context switch
Posted by Andy Chiu 3 months, 3 weeks ago
Hi Sergey,

On Tue, Oct 7, 2025 at 6:58 AM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> The vstate in thread_struct is zeroed when the vector context is
> initialized. That includes read-only register vlenb, which holds
> the vector register length in bytes. This zeroed state persists
> until mstatus.VS becomes 'dirty' and a context switch saves the
> actual hardware values.
>
> This can expose the zero vlenb value to the user-space in early
> debug scenarios, e.g. when ptrace attaches to a traced process
> early, before any vector instruction except the first one was
> executed.
>
> Fix this by forcing the vector context save on the first context switch.
>
> Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> ---
>  arch/riscv/kernel/vector.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> index 901e67adf576..3dd22a71aa18 100644
> --- a/arch/riscv/kernel/vector.c
> +++ b/arch/riscv/kernel/vector.c
> @@ -120,6 +120,7 @@ static int riscv_v_thread_zalloc(struct kmem_cache *cache,
>
>         ctx->datap = datap;
>         memset(ctx, 0, offsetof(struct __riscv_v_ext_state, datap));
> +
>         return 0;
>  }
>
> @@ -216,8 +217,11 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
>                 force_sig(SIGBUS);
>                 return true;
>         }
> +
>         riscv_v_vstate_on(regs);
>         riscv_v_vstate_set_restore(current, regs);
> +       set_tsk_thread_flag(current, TIF_RISCV_V_FORCE_SAVE);
> +

I am afraid that this approach can result in a security issue where a
context switch happens before the v-restore part of the current
process, cheating the kernel to store stale v-regs onto the current
context memory. Please note that this handler is run with irq enabled
so preemption is allowed.

I would expect simply initializing the vleb in riscv_v_thread_zalloc,
perhaps dropping the "z" in the name to prevent confusion.

>         return true;
>  }
>
> --
> 2.51.0
>

Thanks,
Andy
Re: [PATCH v2 6/6] riscv: vector: initialize vlenb on the first context switch
Posted by Sergey Matyukevich 3 months, 3 weeks ago
On Wed, Oct 15, 2025 at 02:54:39PM -0500, Andy Chiu wrote:
> Hi Sergey,
> 
> On Tue, Oct 7, 2025 at 6:58 AM Sergey Matyukevich <geomatsi@gmail.com> wrote:
> >
> > The vstate in thread_struct is zeroed when the vector context is
> > initialized. That includes read-only register vlenb, which holds
> > the vector register length in bytes. This zeroed state persists
> > until mstatus.VS becomes 'dirty' and a context switch saves the
> > actual hardware values.
> >
> > This can expose the zero vlenb value to the user-space in early
> > debug scenarios, e.g. when ptrace attaches to a traced process
> > early, before any vector instruction except the first one was
> > executed.
> >
> > Fix this by forcing the vector context save on the first context switch.
> >
> > Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> > ---
> >  arch/riscv/kernel/vector.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> > index 901e67adf576..3dd22a71aa18 100644
> > --- a/arch/riscv/kernel/vector.c
> > +++ b/arch/riscv/kernel/vector.c
> > @@ -120,6 +120,7 @@ static int riscv_v_thread_zalloc(struct kmem_cache *cache,
> >
> >         ctx->datap = datap;
> >         memset(ctx, 0, offsetof(struct __riscv_v_ext_state, datap));
> > +
> >         return 0;
> >  }
> >
> > @@ -216,8 +217,11 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
> >                 force_sig(SIGBUS);
> >                 return true;
> >         }
> > +
> >         riscv_v_vstate_on(regs);
> >         riscv_v_vstate_set_restore(current, regs);
> > +       set_tsk_thread_flag(current, TIF_RISCV_V_FORCE_SAVE);
> > +
> 
> I am afraid that this approach can result in a security issue where a
> context switch happens before the v-restore part of the current
> process, cheating the kernel to store stale v-regs onto the current
> context memory. Please note that this handler is run with irq enabled
> so preemption is allowed.
> 
> I would expect simply initializing the vleb in riscv_v_thread_zalloc,
> perhaps dropping the "z" in the name to prevent confusion.

Ok, so we can just set 'ctx->vlenb = riscv_v_vsize / 32' in the renamed
riscv_v_thread_alloc function. But note, that w/o forced context save
we implicitly reset the vector configuration to 'all zeros', overwriting
the hardware defaults.

By the way, could you please elaborate a little bit more about your security
concerns with the TIF_RISCV_V_FORCE_SAVE approach ? The atomic and per-process
flag modification looks safe to me, so I'd like to understand what I am
missing.

Thanks,
Sergey
Re: [PATCH v2 6/6] riscv: vector: initialize vlenb on the first context switch
Posted by Andy Chiu 3 months, 2 weeks ago
On Sun, Oct 19, 2025 at 4:43 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> On Wed, Oct 15, 2025 at 02:54:39PM -0500, Andy Chiu wrote:
> > Hi Sergey,
> >
> > On Tue, Oct 7, 2025 at 6:58 AM Sergey Matyukevich <geomatsi@gmail.com> wrote:
> > >
> > > The vstate in thread_struct is zeroed when the vector context is
> > > initialized. That includes read-only register vlenb, which holds
> > > the vector register length in bytes. This zeroed state persists
> > > until mstatus.VS becomes 'dirty' and a context switch saves the
> > > actual hardware values.
> > >
> > > This can expose the zero vlenb value to the user-space in early
> > > debug scenarios, e.g. when ptrace attaches to a traced process
> > > early, before any vector instruction except the first one was
> > > executed.
> > >
> > > Fix this by forcing the vector context save on the first context switch.
> > >
> > > Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> > > ---
> > >  arch/riscv/kernel/vector.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> > > index 901e67adf576..3dd22a71aa18 100644
> > > --- a/arch/riscv/kernel/vector.c
> > > +++ b/arch/riscv/kernel/vector.c
> > > @@ -120,6 +120,7 @@ static int riscv_v_thread_zalloc(struct kmem_cache *cache,
> > >
> > >         ctx->datap = datap;
> > >         memset(ctx, 0, offsetof(struct __riscv_v_ext_state, datap));
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -216,8 +217,11 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
> > >                 force_sig(SIGBUS);
> > >                 return true;
> > >         }
> > > +
> > >         riscv_v_vstate_on(regs);
> > >         riscv_v_vstate_set_restore(current, regs);
> > > +       set_tsk_thread_flag(current, TIF_RISCV_V_FORCE_SAVE);
> > > +
> >
> > I am afraid that this approach can result in a security issue where a
> > context switch happens before the v-restore part of the current
> > process, cheating the kernel to store stale v-regs onto the current
> > context memory. Please note that this handler is run with irq enabled
> > so preemption is allowed.
> >
> > I would expect simply initializing the vleb in riscv_v_thread_zalloc,
> > perhaps dropping the "z" in the name to prevent confusion.
>
> Ok, so we can just set 'ctx->vlenb = riscv_v_vsize / 32' in the renamed
> riscv_v_thread_alloc function. But note, that w/o forced context save
> we implicitly reset the vector configuration to 'all zeros', overwriting
> the hardware defaults.

Resetting all vregs to zero is desired as otherwise we may
unintentionally leak stale states from other users or the kernel to
the user process.

>
> By the way, could you please elaborate a little bit more about your security
> concerns with the TIF_RISCV_V_FORCE_SAVE approach ? The atomic and per-process
> flag modification looks safe to me, so I'd like to understand what I am
> missing.
>

The concern is information leak. A context switch can happen right
after the FORCE_SAVE bit is set. At this point the kernel saves live
vregs on the machine to the context memory (vstate) of that process.
The content of live registers may come from another process, or stale
value of in-kernel Vector uses, since we don't flush registers at
every ownership change. When we switch back to the original process
and return to the user space, the saved stale content is restored back
to registers. As a result, the user space can read Vector registers
from other contexts.

Thanks,
Andy