Re: [PATCH] hw/dma/omap_dma.c: Use 64 bit maths for omap_dma_transfer_setup

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 4/12/25 22:33, Alex Bennée wrote:
> Jim MacArthur <jim.macarthur@linaro.org> writes:
> 
>> If both frame and element count are 65535, which appears valid from my
>> reading of the OMAP5912 documentation, then some of the calculations
>> will overflow the 32-bit signed integer range and produce a negative
>> min_elems value.
>>
>> Raised by #3204 (https://gitlab.com/qemu-project/qemu/-/issues/3204).
>>
> 
> nit:
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3204

Format is:

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3204
Fixes: afbb5194d43 ("Handle on-chip DMA controllers in one place")

> 
>> Signed-off-by: Jim MacArthur <jim.macarthur@linaro.org>
>> ---
>>   hw/dma/omap_dma.c | 32 +++++++++++++++++++++-----------
>>   1 file changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/dma/omap_dma.c b/hw/dma/omap_dma.c
>> index 101f91f4a3..93e6503ff9 100644
>> --- a/hw/dma/omap_dma.c
>> +++ b/hw/dma/omap_dma.c
>> @@ -504,9 +504,19 @@ static void omap_dma_transfer_setup(struct soc_dma_ch_s *dma)
>>       struct omap_dma_channel_s *ch = dma->opaque;
>>       struct omap_dma_s *s = dma->dma->opaque;
>>       int frames, min_elems, elements[__omap_dma_intr_last];
>> +    uint64_t frames64, frame64, elements64, element64;
>>   
>>       a = &ch->active_set;
>>   
>> +    /*
>> +     * We do maths with the frame and element fields which exceeds
>> +     * a signed 32-bit integer, so convert all these to 64 bit for future use.
>> +     */
>> +    frames64 = a->frames;
>> +    frame64 = a->frame;
>> +    elements64 = a->elements;
>> +    element64 = a->element;
>> +
>>       src_p = &s->mpu->port[ch->port[0]];
>>       dest_p = &s->mpu->port[ch->port[1]];
>>       if ((!ch->constant_fill && !src_p->addr_valid(s->mpu, a->src)) ||
>> @@ -527,7 +537,7 @@ static void omap_dma_transfer_setup(struct soc_dma_ch_s *dma)
>>       /* Check all the conditions that terminate the transfer starting
>>        * with those that can occur the soonest.  */
>>   #define INTR_CHECK(cond, id, nelements) \
>> -    if (cond) {         \
>> +    if (cond && nelements <= INT_MAX) {         \
>>           elements[id] = nelements;   \
>>           if (elements[id] < min_elems)   \
>>               min_elems = elements[id];   \
>> @@ -547,24 +557,24 @@ static void omap_dma_transfer_setup(struct soc_dma_ch_s *dma)
>>        * See also the TODO in omap_dma_channel_load.  */
>>       INTR_CHECK(
>>                       (ch->interrupts & LAST_FRAME_INTR) &&
>> -                    ((a->frame < a->frames - 1) || !a->element),
>> +                    ((frame64 < frames64 - 1) || !element64),
>>                       omap_dma_intr_last_frame,
>> -                    (a->frames - a->frame - 2) * a->elements +
>> -                    (a->elements - a->element + 1))
>> +                    (frames64 - frame64 - 2) * elements64 +
>> +                    (elements64 - element64 + 1))
>>       INTR_CHECK(
>>                       ch->interrupts & HALF_FRAME_INTR,
>>                       omap_dma_intr_half_frame,
>> -                    (a->elements >> 1) +
>> -                    (a->element >= (a->elements >> 1) ? a->elements : 0) -
>> -                    a->element)
>> +                    (elements64 >> 1) +
>> +                    (element64 >= (elements64 >> 1) ? elements64 : 0) -
>> +                    element64)
>>       INTR_CHECK(
>>                       ch->sync && ch->fs && (ch->interrupts & END_FRAME_INTR),
>>                       omap_dma_intr_frame,
>> -                    a->elements - a->element)
>> +                    elements64 - element64)
>>       INTR_CHECK(
>>                       ch->sync && ch->fs && !ch->bs,
>>                       omap_dma_intr_frame_sync,
>> -                    a->elements - a->element)
>> +                    elements64 - element64)
>>   
>>       /* Packets */
>>       INTR_CHECK(
>> @@ -581,8 +591,8 @@ static void omap_dma_transfer_setup(struct soc_dma_ch_s *dma)
>>       INTR_CHECK(
>>                       1,
>>                       omap_dma_intr_block,
>> -                    (a->frames - a->frame - 1) * a->elements +
>> -                    (a->elements - a->element))
>> +                    (frames64 - frame64 - 1) * elements64 +
>> +                    (elements64 - element64))
>>   
>>       dma->bytes = min_elems * ch->data_type;
> 
> Can we also add a qtest for the device that checks for this (and can be
> expanded for other unit tests later)?
>