On Wed, Mar 19, 2025 at 5:08 AM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
>
>
>
> On 3/18/25 1:42 PM, Peter Maydell wrote:
> > On Fri, 7 Mar 2025 at 12:46, Daniel Henrique Barboza
> > <dbarb...@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 <dbarb...@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
>
>
