diff mbox series

target/riscv/csr.c: fix OVERFLOW_BEFORE_WIDEN in rmw_sctrdepth()

Message ID 20250307124602.1905754-1-dbarboza@ventanamicro.com (mailing list archive)
State New
Headers show
Series target/riscv/csr.c: fix OVERFLOW_BEFORE_WIDEN in rmw_sctrdepth() | expand

Commit Message

Daniel Henrique Barboza March 7, 2025, 12:46 p.m. UTC 
Coverity found the following issue:

  >>>     CID 1593156:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
  >>>     Potentially overflowing expression "0x10 << depth" with type
  "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then
  used in a context that expects an expression of type "uint64_t" (64
  bits, unsigned).
  4299             depth = 16 << depth;

Fix it by forcing the expression to be 64 bits wide by using '16ULL'.

Resolves: Coverity CID 1593156
Fixes: c48bd18eae ("target/riscv: Add support for Control Transfer Records extension CSRs.")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/csr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alistair Francis March 9, 2025, 10:37 p.m. UTC | #1 
On Fri, Mar 7, 2025 at 10:47 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Coverity found the following issue:
>
>   >>>     CID 1593156:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>   >>>     Potentially overflowing expression "0x10 << depth" with type
>   "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then
>   used in a context that expects an expression of type "uint64_t" (64
>   bits, unsigned).
>   4299             depth = 16 << depth;
>
> Fix it by forcing the expression to be 64 bits wide by using '16ULL'.
>
> Resolves: Coverity CID 1593156
> Fixes: c48bd18eae ("target/riscv: Add support for Control Transfer Records extension CSRs.")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/csr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0ebcca4597..e832ff3ca9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -4296,7 +4296,7 @@ static RISCVException rmw_sctrdepth(CPURISCVState *env, int csrno,
>          }
>
>          /* Update sctrstatus.WRPTR with a legal value */
> -        depth = 16 << depth;
> +        depth = 16ULL << depth;
>          env->sctrstatus =
>              env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth - 1));
>      }
> --
> 2.48.1
>
>
Alistair Francis March 9, 2025, 10:42 p.m. UTC | #2 
On Fri, Mar 7, 2025 at 10:47 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Coverity found the following issue:
>
>   >>>     CID 1593156:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>   >>>     Potentially overflowing expression "0x10 << depth" with type
>   "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then
>   used in a context that expects an expression of type "uint64_t" (64
>   bits, unsigned).
>   4299             depth = 16 << depth;
>
> Fix it by forcing the expression to be 64 bits wide by using '16ULL'.
>
> Resolves: Coverity CID 1593156
> Fixes: c48bd18eae ("target/riscv: Add support for Control Transfer Records extension CSRs.")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/csr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0ebcca4597..e832ff3ca9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -4296,7 +4296,7 @@ static RISCVException rmw_sctrdepth(CPURISCVState *env, int csrno,
>          }
>
>          /* Update sctrstatus.WRPTR with a legal value */
> -        depth = 16 << depth;
> +        depth = 16ULL << depth;
>          env->sctrstatus =
>              env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth - 1));
>      }
> --
> 2.48.1
>
>
Peter Maydell March 18, 2025, 4:42 p.m. UTC | #3 
On Fri, 7 Mar 2025 at 12:46, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Coverity found the following issue:
>
>   >>>     CID 1593156:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>   >>>     Potentially overflowing expression "0x10 << depth" with type
>   "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then
>   used in a context that expects an expression of type "uint64_t" (64
>   bits, unsigned).
>   4299             depth = 16 << depth;
>
> Fix it by forcing the expression to be 64 bits wide by using '16ULL'.
>
> Resolves: Coverity CID 1593156
> Fixes: c48bd18eae ("target/riscv: Add support for Control Transfer Records extension CSRs.")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/csr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0ebcca4597..e832ff3ca9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -4296,7 +4296,7 @@ static RISCVException rmw_sctrdepth(CPURISCVState *env, int csrno,
>          }
>
>          /* Update sctrstatus.WRPTR with a legal value */
> -        depth = 16 << depth;
> +        depth = 16ULL << depth;
>          env->sctrstatus =
>              env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth - 1));
>      }

This is a clear false-positive from Coverity, by the way: we just
checked and enforced that depth is at most SCTRDEPTH_MAX, i.e. 4,
and 16 << 4 cannot possibly overflow anything.

-- PMM
Daniel Henrique Barboza March 18, 2025, 7:07 p.m. UTC | #4 
On 3/18/25 1:42 PM, Peter Maydell wrote:
> On Fri, 7 Mar 2025 at 12:46, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> Coverity found the following issue:
>>
>>    >>>     CID 1593156:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>>    >>>     Potentially overflowing expression "0x10 << depth" with type
>>    "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then
>>    used in a context that expects an expression of type "uint64_t" (64
>>    bits, unsigned).
>>    4299             depth = 16 << depth;
>>
>> Fix it by forcing the expression to be 64 bits wide by using '16ULL'.
>>
>> Resolves: Coverity CID 1593156
>> Fixes: c48bd18eae ("target/riscv: Add support for Control Transfer Records extension CSRs.")
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/csr.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index 0ebcca4597..e832ff3ca9 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -4296,7 +4296,7 @@ static RISCVException rmw_sctrdepth(CPURISCVState *env, int csrno,
>>           }
>>
>>           /* Update sctrstatus.WRPTR with a legal value */
>> -        depth = 16 << depth;
>> +        depth = 16ULL << depth;
>>           env->sctrstatus =
>>               env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth - 1));
>>       }
> 
> This is a clear false-positive from Coverity, by the way: we just
> checked and enforced that depth is at most SCTRDEPTH_MAX, i.e. 4,
> and 16 << 4 cannot possibly overflow anything.

True. I wonder if we should keep this patch anyway due to the better code
pattern in using ULL when left shifting into a 64 bit var, regardless of
not fixing any overflows. There's a chance that we might copy/paste the
existing pattern into another situation where an overflow might actually
happen.

I'll leave to Alistair to decide whether to keep to drop this patch. Either
way works for me. Thanks,



Daniel

> 
> -- PMM
Alistair Francis March 19, 2025, 12:08 a.m. UTC | #5 
On Wed, Mar 19, 2025 at 5:08 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 3/18/25 1:42 PM, Peter Maydell wrote:
> > On Fri, 7 Mar 2025 at 12:46, Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> Coverity found the following issue:
> >>
> >>    >>>     CID 1593156:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> >>    >>>     Potentially overflowing expression "0x10 << depth" with type
> >>    "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then
> >>    used in a context that expects an expression of type "uint64_t" (64
> >>    bits, unsigned).
> >>    4299             depth = 16 << depth;
> >>
> >> Fix it by forcing the expression to be 64 bits wide by using '16ULL'.
> >>
> >> Resolves: Coverity CID 1593156
> >> Fixes: c48bd18eae ("target/riscv: Add support for Control Transfer Records extension CSRs.")
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> ---
> >>   target/riscv/csr.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index 0ebcca4597..e832ff3ca9 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -4296,7 +4296,7 @@ static RISCVException rmw_sctrdepth(CPURISCVState *env, int csrno,
> >>           }
> >>
> >>           /* Update sctrstatus.WRPTR with a legal value */
> >> -        depth = 16 << depth;
> >> +        depth = 16ULL << depth;
> >>           env->sctrstatus =
> >>               env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth - 1));
> >>       }
> >
> > This is a clear false-positive from Coverity, by the way: we just
> > checked and enforced that depth is at most SCTRDEPTH_MAX, i.e. 4,
> > and 16 << 4 cannot possibly overflow anything.
>
> True. I wonder if we should keep this patch anyway due to the better code
> pattern in using ULL when left shifting into a 64 bit var, regardless of
> not fixing any overflows. There's a chance that we might copy/paste the
> existing pattern into another situation where an overflow might actually
> happen.
>
> I'll leave to Alistair to decide whether to keep to drop this patch. Either
> way works for me. Thanks,

Yeah, I figured it was a false positive with SCTRDEPTH_MAX being 4. It
seemed easiest to just "fix" it to keep Coverity happy though. It
doesn't cost us anything to fix it here.

Alistair

>
>
>
> Daniel
>
> >
> > -- PMM
>
>
diff mbox series

Patch

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0ebcca4597..e832ff3ca9 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -4296,7 +4296,7 @@  static RISCVException rmw_sctrdepth(CPURISCVState *env, int csrno,
         }
 
         /* Update sctrstatus.WRPTR with a legal value */
-        depth = 16 << depth;
+        depth = 16ULL << depth;
         env->sctrstatus =
             env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth - 1));
     }