Re: [PATCH for-9.2] target/riscv: Avoid bad shift in riscv_cpu_do_interrupt()

[Peter Maydell]

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