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.macart...@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.macart...@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)?