hw/dma/omap_dma.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
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.0On 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
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
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
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)?
>
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
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
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
© 2016 - 2025 Red Hat, Inc.