Hi Nick,
^ typo in the patch subject: s/risvc/riscv
On 9/3/25 12:01 AM, Nicholas Piggin wrote:
The whole vector ldst instructions do not include a vstart check,
so an overflowed vstart can result in an underflowed memory address
offset and crash:
accel/tcg/cputlb.c:1465:probe_access_flags:
assertion failed: (-(addr | TARGET_PAGE_MASK) >= size)
Add the VSTART_CHECK_EARLY_EXIT() check for these helpers.
This was found with a verification test generator based on RiESCUE.
Reported-by: Nicholas Joaquin <njoaq...@tenstorrent.com>
Reported-by: Ganesh Valliappan <gvalliap...@tenstorrent.com>
Signed-off-by: Nicholas Piggin <npig...@gmail.com>
---
target/riscv/vector_helper.c | 2 +
tests/tcg/riscv64/Makefile.target | 5 ++
tests/tcg/riscv64/test-vstart-overflow.c | 75 ++++++++++++++++++++++++
3 files changed, 82 insertions(+)
create mode 100644 tests/tcg/riscv64/test-vstart-overflow.c
diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index fc85a34a84..e0e8735000 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -825,6 +825,8 @@ vext_ldst_whole(void *vd, target_ulong base, CPURISCVState
*env, uint32_t desc,
uint32_t esz = 1 << log2_esz;
int mmu_index = riscv_env_mmu_index(env, false);
+ VSTART_CHECK_EARLY_EXIT(env, evl);
+
/* Calculate the page range of first page */
addr = base + (env->vstart << log2_esz);
page_split = -(addr | TARGET_PAGE_MASK);
diff --git a/tests/tcg/riscv64/Makefile.target
b/tests/tcg/riscv64/Makefile.target
index 4da5b9a3b3..19a49b6467 100644
--- a/tests/tcg/riscv64/Makefile.target
+++ b/tests/tcg/riscv64/Makefile.target
@@ -18,3 +18,8 @@ TESTS += test-fcvtmod
test-fcvtmod: CFLAGS += -march=rv64imafdc
test-fcvtmod: LDFLAGS += -static
run-test-fcvtmod: QEMU_OPTS += -cpu rv64,d=true,zfa=true
+
+# Test for vstart >= vl
+TESTS += test-vstart-overflow
+test-vstart-overflow: CFLAGS += -march=rv64gcv
+run-test-vstart-overflow: QEMU_OPTS += -cpu rv64,v=on
diff --git a/tests/tcg/riscv64/test-vstart-overflow.c
b/tests/tcg/riscv64/test-vstart-overflow.c
new file mode 100644
index 0000000000..72999f2c8a
--- /dev/null
+++ b/tests/tcg/riscv64/test-vstart-overflow.c
@@ -0,0 +1,75 @@
+/*
+ * Test for VSTART set to overflow VL
+ *
+ * TCG vector instructions should call VSTART_CHECK_EARLY_EXIT() to check
+ * this case, otherwise memory addresses can underflow and misbehave or
+ * crash QEMU.
+ *
+ * TODO: Add stores and other instructions.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <stdint.h>
+#include <riscv_vector.h>
The fix in vector_helper.c is fine but this patch (and patch 3) won't execute
'make check-tcg'. It complains about this header being missing in the docker
env.
To eliminate the possibility of my env being the problem I ran this series in
Gitlab. Same error:
https://gitlab.com/danielhb/qemu/-/jobs/11236091281
/builds/danielhb/qemu/tests/tcg/riscv64/test-vstart-overflow.c:13:10: fatal
error: riscv_vector.h: No such file or directory
3899
13 | #include <riscv_vector.h>
3900
| ^~~~~~~~~~~~~~~~
3901
compilation terminated.
3902
make[1]: *** [Makefile:122: test-vstart-overflow] Error 1
I believe you need to add the Docker changes you made in this patch. Same
thing for patch 3. And same thing for patch 4 of:
[PATCH 0/4] linux-user/riscv: add vector state to signal
Given that you're also using riscv_vector.h in there too. Thanks,
Daniel
+
+#define VSTART_OVERFLOW_TEST(insn) \
+({ \
+ uint8_t vmem[64] = { 0 }; \
+ uint64_t vstart; \
+ asm volatile(" \r\n \
+ # Set VL=52 and VSTART=56 \r\n \
+ li t0, 52 \r\n \
+ vsetvli x0, t0, e8, m4, ta, ma \r\n \
+ li t0, 56 \r\n \
+ csrrw x0, vstart, t0 \r\n \
+ li t1, 64 \r\n \
+ " insn " \r\n \
+ csrr %0, vstart \r\n \
+ " : "=r"(vstart), "+A"(vmem) :: "t0", "t1", "v24", "memory"); \
+ vstart; \
+})
+
+int run_vstart_overflow_tests()
+{
+ /*
+ * An implementation is permitted to raise an illegal instruction
+ * exception when executing a vector instruction if vstart is set to a
+ * value that could not be produced by the execution of that instruction
+ * with the same vtype. If TCG is changed to do this, then this test
+ * could be updated to handle the SIGILL.
+ */
+ if (VSTART_OVERFLOW_TEST("vl1re16.v v24, %1")) {
+ return 1;
+ }
+
+ if (VSTART_OVERFLOW_TEST("vs1r.v v24, %1")) {
+ return 1;
+ }
+
+ if (VSTART_OVERFLOW_TEST("vle16.v v24, %1")) {
+ return 1;
+ }
+
+ if (VSTART_OVERFLOW_TEST("vse16.v v24, %1")) {
+ return 1;
+ }
+
+ if (VSTART_OVERFLOW_TEST("vluxei8.v v24, %1, v20")) {
+ return 1;
+ }
+
+ if (VSTART_OVERFLOW_TEST("vlse16.v v24, %1, t1")) {
+ return 1;
+ }
+
+ if (VSTART_OVERFLOW_TEST("vlseg2e8.v v24, %1")) {
+ return 1;
+ }
+
+ return 0;
+}
+
+int main()
+{
+ return run_vstart_overflow_tests();
+}
