This patch series suggests fixes for several corner cases in the RISC-V vector ptrace implementation:
- follow gdbserver expectations and return ENODATA instead of EINVAL if vector extension is supported but not yet activated for a traced process
- force vector context save on the next context switch after ptrace call that modified vector CSRs, to avoid reading stale values by the next ptrace calls
- force vector context save on the first context switch after vector context initialization, to avoid reading zero vlenb by an early attached debugger
For detailed description see the appropriate commit messages. A new test is added into the tools/testing/selftests/riscv/vector to verify the fixes. Each fix is accompanied by its own test case.
Initial version [1] of this series included only the last fix for zero vlenb.
[1] https://lore.kernel.org/linux-riscv/20250821173957.563472-1-geomatsi@gmail.c...
Ilya Mamay (1): riscv: ptrace: return ENODATA for inactive vector extension
Sergey Matyukevich (5): selftests: riscv: test ptrace vector interface selftests: riscv: set invalid vtype using ptrace riscv: vector: allow to force vector context save selftests: riscv: verify initial vector state with ptrace riscv: vector: initialize vlenb on the first context switch
arch/riscv/include/asm/thread_info.h | 2 + arch/riscv/include/asm/vector.h | 3 + arch/riscv/kernel/process.c | 2 + arch/riscv/kernel/ptrace.c | 15 +- arch/riscv/kernel/vector.c | 4 + .../testing/selftests/riscv/vector/.gitignore | 1 + tools/testing/selftests/riscv/vector/Makefile | 5 +- .../testing/selftests/riscv/vector/v_ptrace.c | 302 ++++++++++++++++++ 8 files changed, 331 insertions(+), 3 deletions(-) create mode 100644 tools/testing/selftests/riscv/vector/v_ptrace.c
base-commit: c746c3b5169831d7fb032a1051d8b45592ae8d78
Add a test case to check ptrace behavior in the case when vector extension is supported by the system, but vector context is not yet enabled for the traced process.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com --- .../testing/selftests/riscv/vector/.gitignore | 1 + tools/testing/selftests/riscv/vector/Makefile | 5 +- .../testing/selftests/riscv/vector/v_ptrace.c | 87 +++++++++++++++++++ 3 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/riscv/vector/v_ptrace.c
diff --git a/tools/testing/selftests/riscv/vector/.gitignore b/tools/testing/selftests/riscv/vector/.gitignore index 7d9c87cd0649..d21c03c3ee0e 100644 --- a/tools/testing/selftests/riscv/vector/.gitignore +++ b/tools/testing/selftests/riscv/vector/.gitignore @@ -2,3 +2,4 @@ vstate_exec_nolibc vstate_prctl v_initval v_exec_initval_nolibc +v_ptrace diff --git a/tools/testing/selftests/riscv/vector/Makefile b/tools/testing/selftests/riscv/vector/Makefile index 6f7497f4e7b3..c14ad127e7fb 100644 --- a/tools/testing/selftests/riscv/vector/Makefile +++ b/tools/testing/selftests/riscv/vector/Makefile @@ -2,7 +2,7 @@ # Copyright (C) 2021 ARM Limited # Originally tools/testing/arm64/abi/Makefile
-TEST_GEN_PROGS := v_initval vstate_prctl +TEST_GEN_PROGS := v_initval vstate_prctl v_ptrace TEST_GEN_PROGS_EXTENDED := vstate_exec_nolibc v_exec_initval_nolibc
include ../../lib.mk @@ -26,3 +26,6 @@ $(OUTPUT)/v_initval: v_initval.c $(OUTPUT)/sys_hwprobe.o $(OUTPUT)/v_helpers.o $(OUTPUT)/v_exec_initval_nolibc: v_exec_initval_nolibc.c $(CC) -nostdlib -static -include ../../../../include/nolibc/nolibc.h \ -Wall $(CFLAGS) $(LDFLAGS) $^ -o $@ -lgcc + +$(OUTPUT)/v_ptrace: v_ptrace.c $(OUTPUT)/sys_hwprobe.o $(OUTPUT)/v_helpers.o + $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ diff --git a/tools/testing/selftests/riscv/vector/v_ptrace.c b/tools/testing/selftests/riscv/vector/v_ptrace.c new file mode 100644 index 000000000000..6a8d56a5c4f4 --- /dev/null +++ b/tools/testing/selftests/riscv/vector/v_ptrace.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <sys/ptrace.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <sys/wait.h> +#include <sys/uio.h> +#include <unistd.h> +#include <errno.h> + +#include <linux/ptrace.h> +#include <linux/elf.h> + +#include "../../kselftest_harness.h" +#include "v_helpers.h" + +volatile unsigned long chld_lock; + +TEST(ptrace_rvv_not_enabled) +{ + pid_t pid; + + if (!is_vector_supported()) + SKIP(return, "Vector not supported"); + + chld_lock = 1; + + pid = fork(); + + ASSERT_LE(0, pid) + TH_LOG("fork: %m"); + + if (pid == 0) { + while (chld_lock == 1) + asm volatile("" : : "g"(chld_lock) : "memory"); + + asm volatile ("ebreak" : : : ); + } else { + struct __riscv_v_regset_state *regset_data; + unsigned long vlenb; + size_t regset_size; + struct iovec iov; + int status; + int ret; + + asm volatile("csrr %[vlenb], vlenb" : [vlenb] "=r"(vlenb)); + + ASSERT_GT(vlenb, 0) + TH_LOG("vlenb is not valid: %lu\n", vlenb); + + /* attach */ + + ASSERT_EQ(0, ptrace(PTRACE_ATTACH, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* unlock */ + + ASSERT_EQ(0, ptrace(PTRACE_POKEDATA, pid, &chld_lock, 0)); + + /* resume and wait for ebreak */ + + ASSERT_EQ(0, ptrace(PTRACE_CONT, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* try to read vector registers from the tracee */ + + regset_size = sizeof(*regset_data) + vlenb * 32; + regset_data = calloc(1, regset_size); + + iov.iov_base = regset_data; + iov.iov_len = regset_size; + + /* V extension is available, but not yet enabled for the tracee */ + + errno = 0; + ret = ptrace(PTRACE_GETREGSET, pid, NT_RISCV_VECTOR, &iov); + ASSERT_EQ(ENODATA, errno); + ASSERT_EQ(-1, ret); + + /* cleanup */ + + ASSERT_EQ(0, kill(pid, SIGKILL)); + } +} + +TEST_HARNESS_MAIN
From: Ilya Mamay mmamayka01@gmail.com
Currently, ptrace returns EINVAL when the vector extension is supported but not yet activated for the traced process. This error code is inappropriate since all the ptrace arguments are valid.
Debug tools like gdbserver expect ENODATA when the requested register set is not active, e.g. see [1]. This expectation seems to be more appropriate, so modify the vector ptrace implementation to return: - EINVAL when V extension is not supported - ENODATA when V extension is supported but not active
[1] https://github.com/bminor/binutils-gdb/blob/637f25e88675fa47e47f9cc5e2cf3738...
Signed-off-by: Ilya Mamay mmamayka01@gmail.com --- arch/riscv/kernel/ptrace.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c index 8e86305831ea..906cf1197edc 100644 --- a/arch/riscv/kernel/ptrace.c +++ b/arch/riscv/kernel/ptrace.c @@ -95,9 +95,12 @@ static int riscv_vr_get(struct task_struct *target, struct __riscv_v_ext_state *vstate = &target->thread.vstate; struct __riscv_v_regset_state ptrace_vstate;
- if (!riscv_v_vstate_query(task_pt_regs(target))) + if (!has_vector()) return -EINVAL;
+ if (!riscv_v_vstate_query(task_pt_regs(target))) + return -ENODATA; + /* * Ensure the vector registers have been saved to the memory before * copying them to membuf. @@ -130,9 +133,12 @@ static int riscv_vr_set(struct task_struct *target, struct __riscv_v_ext_state *vstate = &target->thread.vstate; struct __riscv_v_regset_state ptrace_vstate;
- if (!riscv_v_vstate_query(task_pt_regs(target))) + if (!has_vector()) return -EINVAL;
+ if (!riscv_v_vstate_query(task_pt_regs(target))) + return -ENODATA; + /* Copy rest of the vstate except datap */ ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &ptrace_vstate, 0, sizeof(struct __riscv_v_regset_state));
Add a test case that attempts to set invalid vtype value using ptrace and verifies that the 'vill' bit is set as required by the RISC-V Vector specification v1.0, Section 3.4.1.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com --- .../testing/selftests/riscv/vector/v_ptrace.c | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+)
diff --git a/tools/testing/selftests/riscv/vector/v_ptrace.c b/tools/testing/selftests/riscv/vector/v_ptrace.c index 6a8d56a5c4f4..ccda8a4dc49b 100644 --- a/tools/testing/selftests/riscv/vector/v_ptrace.c +++ b/tools/testing/selftests/riscv/vector/v_ptrace.c @@ -84,4 +84,116 @@ TEST(ptrace_rvv_not_enabled) } }
+TEST(ptrace_rvv_invalid_vtype) +{ + static volatile unsigned long vtype; + unsigned long vlenb; + unsigned long reg; + pid_t pid; + + if (!is_vector_supported()) + SKIP(return, "Vector not supported"); + + asm volatile("csrr %[vlenb], vlenb" : [vlenb] "=r"(vlenb)); + + if (vlenb > 16) + SKIP(return, "This test does not support VLEN > 128"); + + chld_lock = 1; + + pid = fork(); + + ASSERT_LE(0, pid) + TH_LOG("fork: %m"); + + if (pid == 0) { + while (chld_lock == 1) + asm volatile("" : : "g"(chld_lock) : "memory"); + + asm(".option arch, +v\n"); + asm(".option arch, +c\n"); + asm volatile("vsetvli x0, x0, e8, m8, tu, mu\n"); + + while (1) { + asm volatile ("c.ebreak"); + asm volatile("csrr %[vtype], vtype" : [vtype] "=r"(vtype) : :); + asm volatile ("c.ebreak"); + } + } else { + struct __riscv_v_regset_state *regset_data; + struct user_regs_struct regs; + size_t regset_size; + struct iovec iov; + int status; + + /* attach */ + + ASSERT_EQ(0, ptrace(PTRACE_ATTACH, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* unlock */ + + ASSERT_EQ(0, ptrace(PTRACE_POKEDATA, pid, &chld_lock, 0)); + + /* resume and wait for the 1st c.ebreak */ + + ASSERT_EQ(0, ptrace(PTRACE_CONT, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* read tracee vector csr regs using ptrace GETREGSET */ + + regset_size = sizeof(*regset_data) + vlenb * 32; + regset_data = calloc(1, regset_size); + + iov.iov_base = regset_data; + iov.iov_len = regset_size; + + ASSERT_EQ(0, ptrace(PTRACE_GETREGSET, pid, NT_RISCV_VECTOR, &iov)); + + /* set invalid vtype 0x1d = (5 | 3 << 3): + * - LMUL: 1/8 + * - SEW: 64 + * - invalid configuration for VLENB <= 128 + */ + regset_data->vtype = 0x1d; + ASSERT_EQ(0, ptrace(PTRACE_SETREGSET, pid, NT_RISCV_VECTOR, &iov)); + + /* skip 1st c.ebreak, then resume and wait for the 2nd c.ebreak */ + + iov.iov_base = ®s; + iov.iov_len = sizeof(regs); + + ASSERT_EQ(0, ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov)); + regs.pc += 2; + ASSERT_EQ(0, ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov)); + + ASSERT_EQ(0, ptrace(PTRACE_CONT, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* read tracee vtype using ptrace GETREGSET */ + + iov.iov_base = regset_data; + iov.iov_len = regset_size; + ASSERT_EQ(0, ptrace(PTRACE_GETREGSET, pid, NT_RISCV_VECTOR, &iov)); + + /* read tracee vtype ptrace PEEKDATA */ + + errno = 0; + reg = ptrace(PTRACE_PEEKDATA, pid, &vtype, NULL); + ASSERT_FALSE((errno != 0) && (reg == -1)); + + /* verify that V state is illegal */ + + EXPECT_EQ(reg, regset_data->vtype); + EXPECT_EQ(1UL, (regset_data->vtype >> (__riscv_xlen - 1))); + + /* cleanup */ + + ASSERT_EQ(0, kill(pid, SIGKILL)); + } +} + TEST_HARNESS_MAIN
When ptrace updates vector CSR registers for a traced process, the changes may not be immediately visible to the next ptrace operations due to vector context switch optimizations.
The function 'riscv_v_vstate_save' saves context only if mstatus.VS is 'dirty'. However mstatus.VS of the traced process context may remain 'clean' between two breakpoints, if no vector instructions were executed between those two breakpoints. In this case the vector context will not be saved at the second breakpoint. As a result, the second ptrace may read stale vector CSR values.
Fix this by introducing a TIF flag that forces vector context save on the next context switch, regardless of mstatus.VS state. Set this flag on ptrace oprations that modify vector CSR registers.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com --- arch/riscv/include/asm/thread_info.h | 2 ++ arch/riscv/include/asm/vector.h | 3 +++ arch/riscv/kernel/process.c | 2 ++ arch/riscv/kernel/ptrace.c | 5 +++++ 4 files changed, 12 insertions(+)
diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h index 836d80dd2921..e05e9aa89c43 100644 --- a/arch/riscv/include/asm/thread_info.h +++ b/arch/riscv/include/asm/thread_info.h @@ -118,7 +118,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
#define TIF_32BIT 16 /* compat-mode 32bit process */ #define TIF_RISCV_V_DEFER_RESTORE 17 /* restore Vector before returing to user */ +#define TIF_RISCV_V_FORCE_SAVE 13 /* force Vector context save */
#define _TIF_RISCV_V_DEFER_RESTORE BIT(TIF_RISCV_V_DEFER_RESTORE) +#define _TIF_RISCV_V_FORCE_SAVE BIT(TIF_RISCV_V_FORCE_SAVE)
#endif /* _ASM_RISCV_THREAD_INFO_H */ diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h index b61786d43c20..d3770e13da93 100644 --- a/arch/riscv/include/asm/vector.h +++ b/arch/riscv/include/asm/vector.h @@ -370,6 +370,9 @@ static inline void __switch_to_vector(struct task_struct *prev, { struct pt_regs *regs;
+ if (test_and_clear_tsk_thread_flag(prev, TIF_RISCV_V_FORCE_SAVE)) + __riscv_v_vstate_dirty(task_pt_regs(prev)); + if (riscv_preempt_v_started(prev)) { if (riscv_v_is_on()) { WARN_ON(prev->thread.riscv_v_flags & RISCV_V_CTX_DEPTH_MASK); diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index 31a392993cb4..47959c55cefb 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -183,6 +183,7 @@ void flush_thread(void) kfree(current->thread.vstate.datap); memset(¤t->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); clear_tsk_thread_flag(current, TIF_RISCV_V_DEFER_RESTORE); + clear_tsk_thread_flag(current, TIF_RISCV_V_FORCE_SAVE); #endif #ifdef CONFIG_RISCV_ISA_SUPM if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM)) @@ -205,6 +206,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) memset(&dst->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); memset(&dst->thread.kernel_vstate, 0, sizeof(struct __riscv_v_ext_state)); clear_tsk_thread_flag(dst, TIF_RISCV_V_DEFER_RESTORE); + clear_tsk_thread_flag(dst, TIF_RISCV_V_FORCE_SAVE);
return 0; } diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c index 906cf1197edc..569f756bef23 100644 --- a/arch/riscv/kernel/ptrace.c +++ b/arch/riscv/kernel/ptrace.c @@ -148,6 +148,11 @@ static int riscv_vr_set(struct task_struct *target, if (vstate->vlenb != ptrace_vstate.vlenb) return -EINVAL;
+ if (vstate->vtype != ptrace_vstate.vtype || + vstate->vcsr != ptrace_vstate.vcsr || + vstate->vl != ptrace_vstate.vl) + set_tsk_thread_flag(target, TIF_RISCV_V_FORCE_SAVE); + vstate->vstart = ptrace_vstate.vstart; vstate->vl = ptrace_vstate.vl; vstate->vtype = ptrace_vstate.vtype;
On Tue, Oct 7, 2025 at 6:58 AM Sergey Matyukevich geomatsi@gmail.com wrote:
When ptrace updates vector CSR registers for a traced process, the changes may not be immediately visible to the next ptrace operations due to vector context switch optimizations.
The function 'riscv_v_vstate_save' saves context only if mstatus.VS is 'dirty'. However mstatus.VS of the traced process context may remain 'clean' between two breakpoints, if no vector instructions were executed between those two breakpoints. In this case the vector context will not be saved at the second breakpoint. As a result, the second ptrace may read stale vector CSR values.
IIUC, the second ptrace should not get the stale vector CSR values. The second riscv_vr_get() should be reading from the context memory (vstate), which is updated from the last riscv_vr_set(). The user's vstate should remain the same since last riscv_vr_set(). Could you explain more on how this bug is observed and why only CSRs are affected but not v-regs as well?
Thanks, Andy
Fix this by introducing a TIF flag that forces vector context save on the next context switch, regardless of mstatus.VS state. Set this flag on ptrace oprations that modify vector CSR registers.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com
arch/riscv/include/asm/thread_info.h | 2 ++ arch/riscv/include/asm/vector.h | 3 +++ arch/riscv/kernel/process.c | 2 ++ arch/riscv/kernel/ptrace.c | 5 +++++ 4 files changed, 12 insertions(+)
diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h index 836d80dd2921..e05e9aa89c43 100644 --- a/arch/riscv/include/asm/thread_info.h +++ b/arch/riscv/include/asm/thread_info.h @@ -118,7 +118,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
#define TIF_32BIT 16 /* compat-mode 32bit process */ #define TIF_RISCV_V_DEFER_RESTORE 17 /* restore Vector before returing to user */ +#define TIF_RISCV_V_FORCE_SAVE 13 /* force Vector context save */
#define _TIF_RISCV_V_DEFER_RESTORE BIT(TIF_RISCV_V_DEFER_RESTORE) +#define _TIF_RISCV_V_FORCE_SAVE BIT(TIF_RISCV_V_FORCE_SAVE)
#endif /* _ASM_RISCV_THREAD_INFO_H */ diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h index b61786d43c20..d3770e13da93 100644 --- a/arch/riscv/include/asm/vector.h +++ b/arch/riscv/include/asm/vector.h @@ -370,6 +370,9 @@ static inline void __switch_to_vector(struct task_struct *prev, { struct pt_regs *regs;
if (test_and_clear_tsk_thread_flag(prev, TIF_RISCV_V_FORCE_SAVE))__riscv_v_vstate_dirty(task_pt_regs(prev));if (riscv_preempt_v_started(prev)) { if (riscv_v_is_on()) { WARN_ON(prev->thread.riscv_v_flags & RISCV_V_CTX_DEPTH_MASK);diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index 31a392993cb4..47959c55cefb 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -183,6 +183,7 @@ void flush_thread(void) kfree(current->thread.vstate.datap); memset(¤t->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); clear_tsk_thread_flag(current, TIF_RISCV_V_DEFER_RESTORE);
clear_tsk_thread_flag(current, TIF_RISCV_V_FORCE_SAVE);#endif #ifdef CONFIG_RISCV_ISA_SUPM if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM)) @@ -205,6 +206,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) memset(&dst->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); memset(&dst->thread.kernel_vstate, 0, sizeof(struct __riscv_v_ext_state)); clear_tsk_thread_flag(dst, TIF_RISCV_V_DEFER_RESTORE);
clear_tsk_thread_flag(dst, TIF_RISCV_V_FORCE_SAVE); return 0;} diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c index 906cf1197edc..569f756bef23 100644 --- a/arch/riscv/kernel/ptrace.c +++ b/arch/riscv/kernel/ptrace.c @@ -148,6 +148,11 @@ static int riscv_vr_set(struct task_struct *target, if (vstate->vlenb != ptrace_vstate.vlenb) return -EINVAL;
if (vstate->vtype != ptrace_vstate.vtype ||vstate->vcsr != ptrace_vstate.vcsr ||vstate->vl != ptrace_vstate.vl)set_tsk_thread_flag(target, TIF_RISCV_V_FORCE_SAVE);vstate->vstart = ptrace_vstate.vstart; vstate->vl = ptrace_vstate.vl; vstate->vtype = ptrace_vstate.vtype;-- 2.51.0
On Wed, Oct 15, 2025 at 3:18 PM Andy Chiu andybnac@gmail.com wrote:
On Tue, Oct 7, 2025 at 6:58 AM Sergey Matyukevich geomatsi@gmail.com wrote:
When ptrace updates vector CSR registers for a traced process, the changes may not be immediately visible to the next ptrace operations due to vector context switch optimizations.
The function 'riscv_v_vstate_save' saves context only if mstatus.VS is 'dirty'. However mstatus.VS of the traced process context may remain 'clean' between two breakpoints, if no vector instructions were executed between those two breakpoints. In this case the vector context will not be saved at the second breakpoint. As a result, the second ptrace may read stale vector CSR values.
IIUC, the second ptrace should not get the stale vector CSR values. The second riscv_vr_get() should be reading from the context memory (vstate), which is updated from the last riscv_vr_set(). The user's vstate should remain the same since last riscv_vr_set(). Could you explain more on how this bug is observed and why only CSRs are affected but not v-regs as well?
From looking into your test, I can see that you were trying to set an invalid configuration to Vetor CSRs and expect vill to be reflected upon next read. Yes, this is not happening on the current implementation as it was not expecting invalid input from the user, which should be taken into consideration. Thanks for spotting the case!
According to the spec, "The use of vtype encodings with LMUL < SEWMIN/ELEN is reserved, implementations can set vill if they do not support these configurations." This mean the implementation may actually support this configuration. If that is the case, I think we should not allow this to be configured through the vector ptrace interface, which is designed to support 1.0 (and 0.7) specs. That means, we should not allow this problematic configuration to pass through riscv_vr_set(), reach user space, then the forced save.
I would opt for validating all CSR configurations in the first place. Could you also help enforce checks on other reserved bits as well?
Thanks, Andy
Thanks, Andy
Fix this by introducing a TIF flag that forces vector context save on the next context switch, regardless of mstatus.VS state. Set this flag on ptrace oprations that modify vector CSR registers.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com
arch/riscv/include/asm/thread_info.h | 2 ++ arch/riscv/include/asm/vector.h | 3 +++ arch/riscv/kernel/process.c | 2 ++ arch/riscv/kernel/ptrace.c | 5 +++++ 4 files changed, 12 insertions(+)
diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h index 836d80dd2921..e05e9aa89c43 100644 --- a/arch/riscv/include/asm/thread_info.h +++ b/arch/riscv/include/asm/thread_info.h @@ -118,7 +118,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
#define TIF_32BIT 16 /* compat-mode 32bit process */ #define TIF_RISCV_V_DEFER_RESTORE 17 /* restore Vector before returing to user */ +#define TIF_RISCV_V_FORCE_SAVE 13 /* force Vector context save */
#define _TIF_RISCV_V_DEFER_RESTORE BIT(TIF_RISCV_V_DEFER_RESTORE) +#define _TIF_RISCV_V_FORCE_SAVE BIT(TIF_RISCV_V_FORCE_SAVE)
#endif /* _ASM_RISCV_THREAD_INFO_H */ diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h index b61786d43c20..d3770e13da93 100644 --- a/arch/riscv/include/asm/vector.h +++ b/arch/riscv/include/asm/vector.h @@ -370,6 +370,9 @@ static inline void __switch_to_vector(struct task_struct *prev, { struct pt_regs *regs;
if (test_and_clear_tsk_thread_flag(prev, TIF_RISCV_V_FORCE_SAVE))__riscv_v_vstate_dirty(task_pt_regs(prev));if (riscv_preempt_v_started(prev)) { if (riscv_v_is_on()) { WARN_ON(prev->thread.riscv_v_flags & RISCV_V_CTX_DEPTH_MASK);diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index 31a392993cb4..47959c55cefb 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -183,6 +183,7 @@ void flush_thread(void) kfree(current->thread.vstate.datap); memset(¤t->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); clear_tsk_thread_flag(current, TIF_RISCV_V_DEFER_RESTORE);
clear_tsk_thread_flag(current, TIF_RISCV_V_FORCE_SAVE);#endif #ifdef CONFIG_RISCV_ISA_SUPM if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM)) @@ -205,6 +206,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) memset(&dst->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); memset(&dst->thread.kernel_vstate, 0, sizeof(struct __riscv_v_ext_state)); clear_tsk_thread_flag(dst, TIF_RISCV_V_DEFER_RESTORE);
clear_tsk_thread_flag(dst, TIF_RISCV_V_FORCE_SAVE); return 0;} diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c index 906cf1197edc..569f756bef23 100644 --- a/arch/riscv/kernel/ptrace.c +++ b/arch/riscv/kernel/ptrace.c @@ -148,6 +148,11 @@ static int riscv_vr_set(struct task_struct *target, if (vstate->vlenb != ptrace_vstate.vlenb) return -EINVAL;
if (vstate->vtype != ptrace_vstate.vtype ||vstate->vcsr != ptrace_vstate.vcsr ||vstate->vl != ptrace_vstate.vl)set_tsk_thread_flag(target, TIF_RISCV_V_FORCE_SAVE);vstate->vstart = ptrace_vstate.vstart; vstate->vl = ptrace_vstate.vl; vstate->vtype = ptrace_vstate.vtype;-- 2.51.0
On Wed, Oct 15, 2025 at 04:32:05PM -0500, Andy Chiu wrote:
On Wed, Oct 15, 2025 at 3:18 PM Andy Chiu andybnac@gmail.com wrote:
On Tue, Oct 7, 2025 at 6:58 AM Sergey Matyukevich geomatsi@gmail.com wrote:
When ptrace updates vector CSR registers for a traced process, the changes may not be immediately visible to the next ptrace operations due to vector context switch optimizations.
The function 'riscv_v_vstate_save' saves context only if mstatus.VS is 'dirty'. However mstatus.VS of the traced process context may remain 'clean' between two breakpoints, if no vector instructions were executed between those two breakpoints. In this case the vector context will not be saved at the second breakpoint. As a result, the second ptrace may read stale vector CSR values.
IIUC, the second ptrace should not get the stale vector CSR values. The second riscv_vr_get() should be reading from the context memory (vstate), which is updated from the last riscv_vr_set(). The user's vstate should remain the same since last riscv_vr_set(). Could you explain more on how this bug is observed and why only CSRs are affected but not v-regs as well?
From looking into your test, I can see that you were trying to set an invalid configuration to Vetor CSRs and expect vill to be reflected upon next read. Yes, this is not happening on the current implementation as it was not expecting invalid input from the user, which should be taken into consideration. Thanks for spotting the case!
According to the spec, "The use of vtype encodings with LMUL < SEWMIN/ELEN is reserved, implementations can set vill if they do not support these configurations." This mean the implementation may actually support this configuration. If that is the case, I think we should not allow this to be configured through the vector ptrace interface, which is designed to support 1.0 (and 0.7) specs. That means, we should not allow this problematic configuration to pass through riscv_vr_set(), reach user space, then the forced save.
I would opt for validating all CSR configurations in the first place. Could you also help enforce checks on other reserved bits as well?
Just to clarify, the suggestion is to drop the TIF_RISCV_V_FORCE_SAVE entirely and use only careful validation of input parameter in riscv_vr_set, rather than using both checks. Is that correct?
If that is correct, then I assume we can rely on the simple rule ELEN == XLEN to validate vsew/vlmul supported combinations. Additionally, reserved vsew values (see 3.4.1 in spec) should also be rejected.
Thanks, Sergey
On Sun, Oct 19, 2025 at 4:29 PM Sergey Matyukevich geomatsi@gmail.com wrote:
On Wed, Oct 15, 2025 at 04:32:05PM -0500, Andy Chiu wrote:
On Wed, Oct 15, 2025 at 3:18 PM Andy Chiu andybnac@gmail.com wrote:
On Tue, Oct 7, 2025 at 6:58 AM Sergey Matyukevich geomatsi@gmail.com wrote:
When ptrace updates vector CSR registers for a traced process, the changes may not be immediately visible to the next ptrace operations due to vector context switch optimizations.
The function 'riscv_v_vstate_save' saves context only if mstatus.VS is 'dirty'. However mstatus.VS of the traced process context may remain 'clean' between two breakpoints, if no vector instructions were executed between those two breakpoints. In this case the vector context will not be saved at the second breakpoint. As a result, the second ptrace may read stale vector CSR values.
IIUC, the second ptrace should not get the stale vector CSR values. The second riscv_vr_get() should be reading from the context memory (vstate), which is updated from the last riscv_vr_set(). The user's vstate should remain the same since last riscv_vr_set(). Could you explain more on how this bug is observed and why only CSRs are affected but not v-regs as well?
From looking into your test, I can see that you were trying to set an invalid configuration to Vetor CSRs and expect vill to be reflected upon next read. Yes, this is not happening on the current implementation as it was not expecting invalid input from the user, which should be taken into consideration. Thanks for spotting the case!
According to the spec, "The use of vtype encodings with LMUL < SEWMIN/ELEN is reserved, implementations can set vill if they do not support these configurations." This mean the implementation may actually support this configuration. If that is the case, I think we should not allow this to be configured through the vector ptrace interface, which is designed to support 1.0 (and 0.7) specs. That means, we should not allow this problematic configuration to pass through riscv_vr_set(), reach user space, then the forced save.
I would opt for validating all CSR configurations in the first place. Could you also help enforce checks on other reserved bits as well?
Just to clarify, the suggestion is to drop the TIF_RISCV_V_FORCE_SAVE entirely and use only careful validation of input parameter in riscv_vr_set, rather than using both checks. Is that correct?
Yes, exactly
If that is correct, then I assume we can rely on the simple rule ELEN == XLEN to validate vsew/vlmul supported combinations. Additionally, reserved vsew values (see 3.4.1 in spec) should also be rejected.
I am sorry but this assumption may not be correct. The spec does not restrict a 32b machine from supporting ELEN=64, according to my search. There is a way to infer ELEN though, by inspecting if zve64x is present on isa.
Thanks, Andy
Add a test case that attaches to a traced process immediately after its first vector instructions to verify the initial vector context state.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com --- .../testing/selftests/riscv/vector/v_ptrace.c | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+)
diff --git a/tools/testing/selftests/riscv/vector/v_ptrace.c b/tools/testing/selftests/riscv/vector/v_ptrace.c index ccda8a4dc49b..f452e04629ea 100644 --- a/tools/testing/selftests/riscv/vector/v_ptrace.c +++ b/tools/testing/selftests/riscv/vector/v_ptrace.c @@ -196,4 +196,107 @@ TEST(ptrace_rvv_invalid_vtype) } }
+TEST(ptrace_rvv_early_access) +{ + static volatile unsigned long vstart; + static volatile unsigned long vtype; + static volatile unsigned long vlenb; + static volatile unsigned long vcsr; + static volatile unsigned long vl; + pid_t pid; + + if (!is_vector_supported()) + SKIP(return, "Vector not supported"); + + chld_lock = 1; + + pid = fork(); + + ASSERT_LE(0, pid) + TH_LOG("fork: %m"); + + if (pid == 0) { + while (chld_lock == 1) + asm volatile("" : : "g"(chld_lock) : "memory"); + + asm volatile("csrr %[vstart], vstart" : [vstart] "=r"(vstart)); + asm volatile("csrr %[vl], vl" : [vl] "=r"(vl)); + asm volatile("csrr %[vtype], vtype" : [vtype] "=r"(vtype)); + asm volatile("csrr %[vcsr], vcsr" : [vcsr] "=r"(vcsr)); + asm volatile("csrr %[vlenb], vlenb" : [vlenb] "=r"(vlenb)); + + asm volatile ("ebreak" : : : ); + } else { + struct __riscv_v_regset_state *regset_data; + unsigned long vstart_csr; + unsigned long vl_csr; + unsigned long vtype_csr; + unsigned long vcsr_csr; + unsigned long vlenb_csr; + size_t regset_size; + struct iovec iov; + int status; + + /* attach */ + + ASSERT_EQ(0, ptrace(PTRACE_ATTACH, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* unlock */ + + ASSERT_EQ(0, ptrace(PTRACE_POKEDATA, pid, &chld_lock, 0)); + + /* resume and wait for ebreak */ + + ASSERT_EQ(0, ptrace(PTRACE_CONT, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* read tracee vector csr regs using ptrace PEEKDATA */ + + errno = 0; + vstart_csr = ptrace(PTRACE_PEEKDATA, pid, &vstart, NULL); + ASSERT_FALSE((errno != 0) && (vstart_csr == -1)); + + errno = 0; + vl_csr = ptrace(PTRACE_PEEKDATA, pid, &vl, NULL); + ASSERT_FALSE((errno != 0) && (vl_csr == -1)); + + errno = 0; + vtype_csr = ptrace(PTRACE_PEEKDATA, pid, &vtype, NULL); + ASSERT_FALSE((errno != 0) && (vtype_csr == -1)); + + errno = 0; + vcsr_csr = ptrace(PTRACE_PEEKDATA, pid, &vcsr, NULL); + ASSERT_FALSE((errno != 0) && (vcsr_csr == -1)); + + errno = 0; + vlenb_csr = ptrace(PTRACE_PEEKDATA, pid, &vlenb, NULL); + ASSERT_FALSE((errno != 0) && (vlenb_csr == -1)); + + /* read tracee csr regs using ptrace GETREGSET */ + + regset_size = sizeof(*regset_data) + vlenb_csr * 32; + regset_data = calloc(1, regset_size); + + iov.iov_base = regset_data; + iov.iov_len = regset_size; + + ASSERT_EQ(0, ptrace(PTRACE_GETREGSET, pid, NT_RISCV_VECTOR, &iov)); + + /* compare */ + + EXPECT_EQ(vstart_csr, regset_data->vstart); + EXPECT_EQ(vtype_csr, regset_data->vtype); + EXPECT_EQ(vlenb_csr, regset_data->vlenb); + EXPECT_EQ(vcsr_csr, regset_data->vcsr); + EXPECT_EQ(vl_csr, regset_data->vl); + + /* cleanup */ + + ASSERT_EQ(0, kill(pid, SIGKILL)); + } +} + TEST_HARNESS_MAIN
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; }
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
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
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
linux-kselftest-mirror@lists.linaro.org