Re: [PATCH] hw/ppc: ppc440_uc: avoid multiply overflow in dcr_write_dma()

[Daniel Henrique Barboza]

On 8/5/22 21:30, BALATON Zoltan wrote:
> On Fri, 5 Aug 2022, Daniel Henrique Barboza wrote:
> > Coverity reports a OVERFLOW_BEFORE_WIDEN issue in dcr_write_dma(). When
> > handling the DMA0_CR switch we're doing a multiplication between two
> > integers (count and width), and the product is assigned to an uint64_t
> > (xferlen). The int32 product can be overflow before widened.
>
> It can't in practice though as count is max 0xffff and width is max 8 so this sounds like a Coverity false positive. 

This time I believe it's worth fixing it. This code can be used as a base
to do something similar somewhere else, where the variables don't have
hard caps like we have here, and then we'll have a real overflow to
deal with.

> Maybe you could avoid it by changing the type of count or width to uint64_t 
> instead of casting?

That was my first idea but it would require more changes. I'll send a
v2.


> > Fix it by casting the first operand to uint64_t to force the product to
> > be 64 bit.
> >
> > Fixes: Coverity CID 1490852
> > Fixes: 3c409c1927ef ("ppc440_uc: Basic emulation of PPC440 DMA controller")
>
> This line does not appear in 3c409c1927ef so this second Fixes line is bogus. 
> This was added to fix other Coverity issues in eda3f17bcd7b96 but still did not 
> make Coverity happy :-)


My bad. I'll drop that "Fixes" line.


Daniel

> Regards,
> BALATON Zoltan
>
> > Cc: BALATON Zoltan <balaton@eik.bme.hu>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> > hw/ppc/ppc440_uc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> > index 11fdb88c22..31eeffa946 100644
> > --- a/hw/ppc/ppc440_uc.c
> > +++ b/hw/ppc/ppc440_uc.c
> > @@ -908,7 +908,7 @@ static void dcr_write_dma(void *opaque, int dcrn, uint32_t 
> > val)
> >
> >                     sidx = didx = 0;
> >                     width = 1 << ((val & DMA0_CR_PW) >> 25);
> > -                    xferlen = count * width;
> > +                    xferlen = (uint64_t)count * width;
> >                     wlen = rlen = xferlen;
> >                     rptr = cpu_physical_memory_map(dma->ch[chnl].sa, &rlen,
> >                                                    false);