[5/5] target/riscv/cpu_helper.c: fix bad_shift in riscv_cpu_interrupt()

Commit Message

Daniel Henrique Barboza Jan. 21, 2025, 6:48 p.m. UTC

Coverity reported a BAD_SHIFT issue in the following code:

 > 2097
 >>>>      CID 1590355:  Integer handling issues  (BAD_SHIFT)
 >>>>      In expression "hdeleg >> cause", right shifting by more than 63
       bits has undefined behavior.  The shift amount, "cause", is at least 64.
 > 2098         vsmode_exc = env->virt_enabled && (((hdeleg >> cause) & 1) || vs_injected);
 > 2099         /*

It is not clear to me how the tool guarantees that '"cause" is at least
64', but indeed there's no guarantees that it would be < 64 in the
'async = true' code path.

A simple fix to avoid a potential UB is to add a 'cause < 64' guard like
'mode' is already doing right before 'vsmode_exc'.

Resolves: Coverity CID 1590355
Fixes: 967760f62c ("target/riscv: Implement Ssdbltrp exception handling")
Signed-off-by: Daniel Henrique Barboza <[e-mail]>
---
 target/riscv/cpu_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Alistair Francis Jan. 29, 2025, 12:59 a.m. UTC | #1

On Wed, Jan 22, 2025 at 4:49 AM Daniel Henrique Barboza
<[e-mail]> wrote:
>
> Coverity reported a BAD_SHIFT issue in the following code:
>
>  > 2097
>  >>>>      CID 1590355:  Integer handling issues  (BAD_SHIFT)
>  >>>>      In expression "hdeleg >> cause", right shifting by more than 63
>        bits has undefined behavior.  The shift amount, "cause", is at least 64.
>  > 2098         vsmode_exc = env->virt_enabled && (((hdeleg >> cause) & 1) || vs_injected);
>  > 2099         /*
>
> It is not clear to me how the tool guarantees that '"cause" is at least
> 64', but indeed there's no guarantees that it would be < 64 in the
> 'async = true' code path.
>
> A simple fix to avoid a potential UB is to add a 'cause < 64' guard like
> 'mode' is already doing right before 'vsmode_exc'.
>
> Resolves: Coverity CID 1590355
> Fixes: 967760f62c ("target/riscv: Implement Ssdbltrp exception handling")
> Signed-off-by: Daniel Henrique Barboza <[e-mail]>

Reviewed-by: Alistair Francis <[e-mail]>

Alistair

> ---
>  target/riscv/cpu_helper.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e1dfc4ecbf..64d1d68550 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -2095,7 +2095,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>      mode = env->priv <= PRV_S && cause < 64 &&
>          (((deleg >> cause) & 1) || s_injected || vs_injected) ? PRV_S : PRV_M;
>
> -    vsmode_exc = env->virt_enabled && (((hdeleg >> cause) & 1) || vs_injected);
> +    vsmode_exc = env->virt_enabled && cause < 64 &&
> +        (((hdeleg >> cause) & 1) || vs_injected);
> +
>      /*
>       * Check double trap condition only if already in S-mode and targeting
>       * S-mode
> --
> 2.47.1
>
>

Patch

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e1dfc4ecbf..64d1d68550 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -2095,7 +2095,9 @@  void riscv_cpu_do_interrupt(CPUState *cs)
     mode = env->priv <= PRV_S && cause < 64 &&
         (((deleg >> cause) & 1) || s_injected || vs_injected) ? PRV_S : PRV_M;
 
-    vsmode_exc = env->virt_enabled && (((hdeleg >> cause) & 1) || vs_injected);
+    vsmode_exc = env->virt_enabled && cause < 64 &&
+        (((hdeleg >> cause) & 1) || vs_injected);
+
     /*
      * Check double trap condition only if already in S-mode and targeting
      * S-mode