| Message ID | 20241128103831.3452572-1-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [for-9.2] target/riscv: Avoid bad shift in riscv_cpu_do_interrupt() | expand |
On 11/28/24 7:38 AM, Peter Maydell wrote: > In riscv_cpu_do_interrupt() we use the 'cause' value we got out of > cs->exception as a shift value. However this value can be larger > than 31, which means that "1 << cause" is undefined behaviour, > because we do the shift on an 'int' type. > > This causes the undefined behaviour sanitizer to complain > on one of the check-tcg tests: > > $ UBSAN_OPTIONS=print_stacktrace=1:abort_on_error=1:halt_on_error=1 ./build/clang/qemu-system-riscv64 -M virt -semihosting -display none -device loader,file=build/clang/tests/tcg/riscv64-softmmu/issue1060 > ../../target/riscv/cpu_helper.c:1805:38: runtime error: shift exponent 63 is too large for 32-bit type 'int' > #0 0x55f2dc026703 in riscv_cpu_do_interrupt /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../target/riscv/cpu_helper.c:1805:38 > #1 0x55f2dc3d170e in cpu_handle_exception /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../accel/tcg/cpu-exec.c:752:9 > > In this case cause is RISCV_EXCP_SEMIHOST, which is 0x3f. > > Use 1ULL instead to ensure that the shift is in range. I believe we can add: Fixes: 1697837ed9 ("target/riscv: Add M-mode virtual interrupt and IRQ filtering support.") Fixes: 40336d5b1d ("target/riscv: Add HS-mode virtual interrupt and IRQ filtering support.") > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > target/riscv/cpu_helper.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 0a3ead69eab..45806f5ab0f 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -1802,10 +1802,10 @@ void riscv_cpu_do_interrupt(CPUState *cs) > bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG); > target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK; > uint64_t deleg = async ? env->mideleg : env->medeleg; > - bool s_injected = env->mvip & (1 << cause) & env->mvien && > - !(env->mip & (1 << cause)); > - bool vs_injected = env->hvip & (1 << cause) & env->hvien && > - !(env->mip & (1 << cause)); > + bool s_injected = env->mvip & (1ULL << cause) & env->mvien && > + !(env->mip & (1ULL << cause)); > + bool vs_injected = env->hvip & (1ULL << cause) & env->hvien && > + !(env->mip & (1ULL << cause)); > target_ulong tval = 0; > target_ulong tinst = 0; > target_ulong htval = 0;
On Thu, 28 Nov 2024 at 11:20, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > > > On 11/28/24 7:38 AM, Peter Maydell wrote: > > In riscv_cpu_do_interrupt() we use the 'cause' value we got out of > > cs->exception as a shift value. However this value can be larger > > than 31, which means that "1 << cause" is undefined behaviour, > > because we do the shift on an 'int' type. > > > > This causes the undefined behaviour sanitizer to complain > > on one of the check-tcg tests: > > > > $ UBSAN_OPTIONS=print_stacktrace=1:abort_on_error=1:halt_on_error=1 ./build/clang/qemu-system-riscv64 -M virt -semihosting -display none -device loader,file=build/clang/tests/tcg/riscv64-softmmu/issue1060 > > ../../target/riscv/cpu_helper.c:1805:38: runtime error: shift exponent 63 is too large for 32-bit type 'int' > > #0 0x55f2dc026703 in riscv_cpu_do_interrupt /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../target/riscv/cpu_helper.c:1805:38 > > #1 0x55f2dc3d170e in cpu_handle_exception /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../accel/tcg/cpu-exec.c:752:9 > > > > In this case cause is RISCV_EXCP_SEMIHOST, which is 0x3f. > > > > Use 1ULL instead to ensure that the shift is in range. > > > > I believe we can add: > > Fixes: 1697837ed9 ("target/riscv: Add M-mode virtual interrupt and IRQ filtering support.") > Fixes: 40336d5b1d ("target/riscv: Add HS-mode virtual interrupt and IRQ filtering support.") > > > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > > Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Thanks. Probably also reasonable to Cc: qemu-stable@nongnu.org, which I forgot. -- PMM
On 11/28/24 04:38, Peter Maydell wrote: > In riscv_cpu_do_interrupt() we use the 'cause' value we got out of > cs->exception as a shift value. However this value can be larger > than 31, which means that "1 << cause" is undefined behaviour, > because we do the shift on an 'int' type. > > This causes the undefined behaviour sanitizer to complain > on one of the check-tcg tests: > > $ UBSAN_OPTIONS=print_stacktrace=1:abort_on_error=1:halt_on_error=1 ./build/clang/qemu-system-riscv64 -M virt -semihosting -display none -device loader,file=build/clang/tests/tcg/riscv64-softmmu/issue1060 > ../../target/riscv/cpu_helper.c:1805:38: runtime error: shift exponent 63 is too large for 32-bit type 'int' > #0 0x55f2dc026703 in riscv_cpu_do_interrupt /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../target/riscv/cpu_helper.c:1805:38 > #1 0x55f2dc3d170e in cpu_handle_exception /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../accel/tcg/cpu-exec.c:752:9 > > In this case cause is RISCV_EXCP_SEMIHOST, which is 0x3f. Semihosting is completely artificial and should never be injected. The maximum "real" cause is RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT = 0x17, We ought to hoist the handling of RISCV_EXCP_SEMIHOST higher in the function, before these calculations. r~
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 0a3ead69eab..45806f5ab0f 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -1802,10 +1802,10 @@ void riscv_cpu_do_interrupt(CPUState *cs) bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG); target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK; uint64_t deleg = async ? env->mideleg : env->medeleg; - bool s_injected = env->mvip & (1 << cause) & env->mvien && - !(env->mip & (1 << cause)); - bool vs_injected = env->hvip & (1 << cause) & env->hvien && - !(env->mip & (1 << cause)); + bool s_injected = env->mvip & (1ULL << cause) & env->mvien && + !(env->mip & (1ULL << cause)); + bool vs_injected = env->hvip & (1ULL << cause) & env->hvien && + !(env->mip & (1ULL << cause)); target_ulong tval = 0; target_ulong tinst = 0; target_ulong htval = 0;
In riscv_cpu_do_interrupt() we use the 'cause' value we got out of cs->exception as a shift value. However this value can be larger than 31, which means that "1 << cause" is undefined behaviour, because we do the shift on an 'int' type. This causes the undefined behaviour sanitizer to complain on one of the check-tcg tests: $ UBSAN_OPTIONS=print_stacktrace=1:abort_on_error=1:halt_on_error=1 ./build/clang/qemu-system-riscv64 -M virt -semihosting -display none -device loader,file=build/clang/tests/tcg/riscv64-softmmu/issue1060 ../../target/riscv/cpu_helper.c:1805:38: runtime error: shift exponent 63 is too large for 32-bit type 'int' #0 0x55f2dc026703 in riscv_cpu_do_interrupt /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../target/riscv/cpu_helper.c:1805:38 #1 0x55f2dc3d170e in cpu_handle_exception /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../accel/tcg/cpu-exec.c:752:9 In this case cause is RISCV_EXCP_SEMIHOST, which is 0x3f. Use 1ULL instead to ensure that the shift is in range. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/riscv/cpu_helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)