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

["Alex Bennée" <alex.bennee@linaro.org>]

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