| Message ID | 20241128103831.3452572-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| 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~
On Thu, 28 Nov 2024 at 12:59, Richard Henderson <richard.henderson@linaro.org> wrote: > > 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. Perhaps so, but looking at https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf it says that mvien, mie, etc are 64-bit registers and the cause value can be validly greater than 31. So we need to use the ULL suffix here. And if we're doing that, then it's harmless to also calculate these booleans even in the semihosting case, because we don't look at them then. So I think we definitely need this patch, and whether to refactor the code to move the bool initializers around is a separate question. thanks -- PMM
On 12/2/24 07:46, Peter Maydell wrote: > On Thu, 28 Nov 2024 at 12:59, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> 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. > > Perhaps so, but looking at > https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf > it says that mvien, mie, etc are 64-bit registers and the > cause value can be validly greater than 31. So we need to > use the ULL suffix here. And if we're doing that, then it's > harmless to also calculate these booleans even in the > semihosting case, because we don't look at them then. > > So I think we definitely need this patch, and whether > to refactor the code to move the bool initializers around > is a separate question. I missed that the registers are 64-bit, so fair enough. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Thu, Nov 28, 2024 at 7:39 PM Peter Maydell <peter.maydell@linaro.org> 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. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > 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; > -- > 2.34.1 > >
On Thu, Nov 28, 2024 at 7:39 PM Peter Maydell <peter.maydell@linaro.org> 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. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Thanks! Applied to riscv-to-apply.next Alistair > --- > 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; > -- > 2.34.1 > >
Hi Alistair, On 3/12/24 07:31, Alistair Francis wrote: > On Thu, Nov 28, 2024 at 7:39 PM Peter Maydell <peter.maydell@linaro.org> 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. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > Thanks! > > Applied to riscv-to-apply.next Since next release PRs are due in less than 4h, I'll take this patch via my hw-misc tree (I already ran various tests with it). Regards, Phil.
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(-)