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

[Alistair Francis <alistair23@gmail.com>]

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