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

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

[Daniel Henrique Barboza]

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));
     }
-- 
2.48.1

Re: [PATCH] target/riscv/csr.c: fix OVERFLOW_BEFORE_WIDEN in rmw_sctrdepth()

[Alistair Francis]

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
>
>

Re: [PATCH] target/riscv/csr.c: fix OVERFLOW_BEFORE_WIDEN in rmw_sctrdepth()

[Alistair Francis]

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
>
>

Re: [PATCH] target/riscv/csr.c: fix OVERFLOW_BEFORE_WIDEN in rmw_sctrdepth()

[Peter Maydell]

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

Re: [PATCH] target/riscv/csr.c: fix OVERFLOW_BEFORE_WIDEN in rmw_sctrdepth()

[Daniel Henrique Barboza]

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

Re: [PATCH] target/riscv/csr.c: fix OVERFLOW_BEFORE_WIDEN in rmw_sctrdepth()

[Alistair Francis]

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
>
>