Re: [PATCH 2/3] target/risvc: Fix vector whole ldst vstart check

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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 <njoaquin@tenstorrent.com>
> Reported-by: Ganesh Valliappan <gvalliappan@tenstorrent.com>
> Signed-off-by: Nicholas Piggin <npiggin@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();
> +}