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

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

[Jim MacArthur]

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

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

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

[Alex Bennée]

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

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Peter Maydell]

On Thu, 4 Dec 2025 at 19:34, Jim MacArthur <jim.macarthur@linaro.org> wrote:
>
> 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).
>
> 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;

The real hardware is surely not doing 64-bit arithmetic though.
It's presumably working with elements * frames as unsigned 32 bit
multiplication.

-- PMM

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

[Jim MacArthur]

On 12/5/25 10:33, Peter Maydell wrote:
> On Thu, 4 Dec 2025 at 19:34, Jim MacArthur <jim.macarthur@linaro.org> wrote:
>> 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).
> The real hardware is surely not doing 64-bit arithmetic though.
> It's presumably working with elements * frames as unsigned 32 bit
> multiplication.
>
> -- PMM

I agree it's unlikely that the hardware would use 64 bit maths, but 
without any spec (that I've found so far) or real hardware to test on, I 
can't be sure what the expected behaviour is.

If we think that uint32_t is a more likely situation than the current 
`int` then I'm happy to change it to that.

Jim

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

[Philippe Mathieu-Daudé]

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

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

[Jim MacArthur]

On 12/5/25 15:57, Philippe Mathieu-Daudé wrote:
> 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")
>
I'm unclear on whether this actually resolves or fixes the issue, so I 
just said 'Raised by'. The bug only includes a test case, not a text 
description of the problem. The test case will give a different error if 
this patch is applied, but still doesn't pass. I've mentioned this on 
the bug page.

Jim

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

[Peter Maydell]

On Fri, 5 Dec 2025 at 16:11, Jim MacArthur <jim.macarthur@linaro.org> wrote:
>
>
> On 12/5/25 15:57, Philippe Mathieu-Daudé wrote:
> > 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")
> >
> I'm unclear on whether this actually resolves or fixes the issue, so I
> just said 'Raised by'. The bug only includes a test case, not a text
> description of the problem. The test case will give a different error if
> this patch is applied, but still doesn't pass. I've mentioned this on
> the bug page.

Generally for this kind of fuzzer-generated bug report, the
bug is "it is possible to make QEMU assert/crash/etc". They
don't come with textual analysis of why exactly we ended up
crashing, because the crash was auto-generated. So the
"what actually happened here" is one of the things you have
to figure out as part of fixing the bug.

If you're satisfied that your fix deals with the problem,
then you can mark it as "Resolves:" for that bug. If it
turns out that the fix accidentally failed to catch the whole
class of problems, then the people running fuzzers will let
us know by filing a fresh bug report at some future point.

thanks
-- PMM

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

[Jim MacArthur]

On 12/5/25 16:20, Peter Maydell wrote:
> On Fri, 5 Dec 2025 at 16:11, Jim MacArthur <jim.macarthur@linaro.org> wrote:
>>
>> On 12/5/25 15:57, Philippe Mathieu-Daudé wrote:
>>> 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")
>>>
>> I'm unclear on whether this actually resolves or fixes the issue, so I
>> just said 'Raised by'. The bug only includes a test case, not a text
>> description of the problem. The test case will give a different error if
>> this patch is applied, but still doesn't pass. I've mentioned this on
>> the bug page.
> Generally for this kind of fuzzer-generated bug report, the
> bug is "it is possible to make QEMU assert/crash/etc". They
> don't come with textual analysis of why exactly we ended up
> crashing, because the crash was auto-generated. So the
> "what actually happened here" is one of the things you have
> to figure out as part of fixing the bug.

By that criterion, this patch doesn't fix the bug as it will still throw 
a different address sanitizer error. The test case identified at least 
two problems, one of which is fixed by this patch (when I address your 
comments about u64/u32 math) and the other which I haven't figured out 
how to address yet. I can leave this without the Resolves: tag, or add 
an extra issue to Gitlab with the specific problem, or we can leave it 
until we have a patch for the other problems.

Jim