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.

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")
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);
-- 
2.36.1

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. Maybe you could avoid it by 
changing the type of count or width to uint64_t instead of casting?

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

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

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

Balaton,

I had a change of heart. The code is too clear that it won't overflow.
It felt overkill changing var types just for that.

Peter already marked it as Ignored - False Positive in Coverity as well.
So this would be a code change to "look better". It didn't look particularly
better after the changes I was about to send, so let's forget about it.


Daniel

On 8/6/22 07:39, Daniel Henrique Barboza wrote:
> 
> 
> 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);
>>>

On Sat, 6 Aug 2022 at 12:17, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> Balaton,
>
> I had a change of heart. The code is too clear that it won't overflow.
> It felt overkill changing var types just for that.
>
> Peter already marked it as Ignored - False Positive in Coverity as well.
> So this would be a code change to "look better". It didn't look particularly
> better after the changes I was about to send, so let's forget about it.

Yeah, I marked it false positive because the limit on the two
inputs to the multiply is very clear in the code. (Usually I
put a note about why when I mark something an FP, but you have
to expand the 'Triage' section in the right hand panel of the
Coverity Scan web UI to show that text.)

-- PMM

On Sat, 6 Aug 2022, Daniel Henrique Barboza wrote:
> Balaton,
>
> I had a change of heart. The code is too clear that it won't overflow.
> It felt overkill changing var types just for that.
>
> Peter already marked it as Ignored - False Positive in Coverity as well.
> So this would be a code change to "look better". It didn't look particularly
> better after the changes I was about to send, so let's forget about it.

Fine with me. I may look at it later when Peter's second patch changing 
this code lands if there are any cleanups possible, I've noticed that now 
that we have xferlen = count * width it could be used elsewehere where 
that product appears for example. At that point I'll see if the type can 
be changed too. Until then it's fine as it is now.

Regards,
BALATON Zoltan

On Sat, 6 Aug 2022 at 12:24, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> I may look at it later when Peter's second patch changing
> this code lands if there are any cleanups possible

You mean the 2nd patch I sent in that RFC series? I'm not
currently totally sure what I want to do with that. Looking
at the code and at the docs that describe how the device
works, there's definitely quite a bit of missing
functionality. So I'm sort of undecided between "do a fair
bit of overhaul to the device to fix up the more obviously
missing pieces (but without much ability to test the changes)"
and "just leave the code as it is in git at the moment, since
it's sufficient for the only guest we know touches it to be
able to boot". What would you prefer?

thanks
-- PMM

On Sat, 6 Aug 2022, Peter Maydell wrote:
> On Sat, 6 Aug 2022 at 12:24, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> I may look at it later when Peter's second patch changing
>> this code lands if there are any cleanups possible
>
> You mean the 2nd patch I sent in that RFC series? I'm not

Yes I mean that patch.

> currently totally sure what I want to do with that. Looking
> at the code and at the docs that describe how the device
> works, there's definitely quite a bit of missing
> functionality. So I'm sort of undecided between "do a fair

As the original commit message says I only aimed for the functionality 
AmigaOS seems to use and did not try to implement everything. What we have 
now seems to be enough for AmigaOS but I don't know if there are any other 
functionality it may want to use in rare cases that I don't know about or 
there are other guests that would do so.

> bit of overhaul to the device to fix up the more obviously
> missing pieces (but without much ability to test the changes)"
> and "just leave the code as it is in git at the moment, since
> it's sufficient for the only guest we know touches it to be
> able to boot". What would you prefer?

I won't stop you if you want to improve this device but I also don't know 
how to test it apart from verifying AmigaOS still boots and works. It did 
with your second patch (I've sent a Tested-by) and I'll test any further 
patches. So just fixing the obviously wrong parts without adding any new 
functionality is probably enough if you don't want to spend too much time 
with it. I'll try to gather some traces on how AmigaOS uses it to help you 
to judge what's needed.

Thank you,
BALATON Zoltan