[PATCH] hw/dma: prevent overflow in soc_dma_set_request

[PATCH] hw/dma: prevent overflow in soc_dma_set_request

[Anastasia Belova]

ch->num can reach values up to 31. Add casting to
a larger type before performing left shift to
prevent integer overflow.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: afbb5194d4 ("Handle on-chip DMA controllers in one place, convert OMAP DMA to use it.")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 hw/dma/soc_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/dma/soc_dma.c b/hw/dma/soc_dma.c
index 3a430057f5..d5c52b804f 100644
--- a/hw/dma/soc_dma.c
+++ b/hw/dma/soc_dma.c
@@ -209,9 +209,9 @@ void soc_dma_set_request(struct soc_dma_ch_s *ch, int level)
     dma->enabled_count += level - ch->enable;
 
     if (level)
-        dma->ch_enable_mask |= 1 << ch->num;
+        dma->ch_enable_mask |= (uint64_t)1 << ch->num;
     else
-        dma->ch_enable_mask &= ~(1 << ch->num);
+        dma->ch_enable_mask &= ~((uint64_t)1 << ch->num);
 
     if (level != ch->enable) {
         soc_dma_ch_freq_update(dma);
-- 
2.30.2

Re: [PATCH] hw/dma: prevent overflow in soc_dma_set_request

[Peter Maydell]

On Tue, 9 Apr 2024 at 12:54, Anastasia Belova <abelova@astralinux.ru> wrote:
>
> ch->num can reach values up to 31. Add casting to
> a larger type before performing left shift to
> prevent integer overflow.

If ch->num can only reach up to 31, then 1 << ch->num
is fine, because QEMU can assume that integers are 32 bits,
and we compile with -fwrapv so there isn't a problem with
shifting into the sign bit.

And I agree that we shouldn't ever have a ch->num greater
than 31, because the worst case here is when we call
soc_dma_init() with an argument of 32, which sets up
soc_dma_ch_s structs with values of num from 0 to 31.

So this doesn't seem to me to be fixing an active bug.
Am I missing something?

thanks
-- PMM

Re: [PATCH] hw/dma: prevent overflow in soc_dma_set_request

[Philippe Mathieu-Daudé]

On 9/4/24 14:02, Peter Maydell wrote:
> On Tue, 9 Apr 2024 at 12:54, Anastasia Belova <abelova@astralinux.ru> wrote:
>>
>> ch->num can reach values up to 31. Add casting to
>> a larger type before performing left shift to
>> prevent integer overflow.
> 
> If ch->num can only reach up to 31, then 1 << ch->num
> is fine, because QEMU can assume that integers are 32 bits,
> and we compile with -fwrapv so there isn't a problem with
> shifting into the sign bit.
> 
> And I agree that we shouldn't ever have a ch->num greater
> than 31, because the worst case here is when we call
> soc_dma_init() with an argument of 32, which sets up
> soc_dma_ch_s structs with values of num from 0 to 31.
> 
> So this doesn't seem to me to be fixing an active bug.
> Am I missing something?

Maybe this path?

omap2420_mpu_init():
  -> omap_dma4_init(chans=32);
      -> soc_dma_init(n=32);
          -> s->chnum = 32;

Re: [PATCH] hw/dma: prevent overflow in soc_dma_set_request

[Anastasia Belova]

09/04/24 15:02, Peter Maydell пишет:
> On Tue, 9 Apr 2024 at 12:54, Anastasia Belova <abelova@astralinux.ru> wrote:
>> ch->num can reach values up to 31. Add casting to
>> a larger type before performing left shift to
>> prevent integer overflow.
> If ch->num can only reach up to 31, then 1 << ch->num
> is fine, because QEMU can assume that integers are 32 bits,
> and we compile with -fwrapv so there isn't a problem with
> shifting into the sign bit.

Right, thanks for your comments.
I didn't know about this flag before. It became more clear for me now.

Thanks,
Anastasia Belova

Re: [PATCH] hw/dma: prevent overflow in soc_dma_set_request

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:32, Anastasia Belova <abelova@astralinux.ru> wrote:
>
>
>
> 09/04/24 15:02, Peter Maydell пишет:
> > On Tue, 9 Apr 2024 at 12:54, Anastasia Belova <abelova@astralinux.ru> wrote:
> >> ch->num can reach values up to 31. Add casting to
> >> a larger type before performing left shift to
> >> prevent integer overflow.
> > If ch->num can only reach up to 31, then 1 << ch->num
> > is fine, because QEMU can assume that integers are 32 bits,
> > and we compile with -fwrapv so there isn't a problem with
> > shifting into the sign bit.
>
> Right, thanks for your comments.
> I didn't know about this flag before. It became more clear for me now.

Yep; if you're using a static analyser you probably want to
configure it to accept the behaviours that are
undefined-in-standard-C and which get defined behaviour
with -fwrapv.

This code is definitely a bit dubious, though, because
ch_enable_mask is a uint64_t, so the intention was clearly
to allow up to 64 channels. So I think we should take this
patch anyway, with a slightly adjusted commit message.

All the soc_dma.c code will probably be removed in the
9.2 release, because it's only used by the OMAP board models
which we've just deprecated, so it doesn't seem worth spending
too much time on cleaning up the code, but in this case you've
already written the patch.

I'll put this patch on my list to apply after we've made the
9.0 release and restarted development for 9.1.

thanks
-- PMM

Re: [PATCH] hw/dma: prevent overflow in soc_dma_set_request

[Peter Maydell]

On Tue, 9 Apr 2024 at 14:38, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 9 Apr 2024 at 14:32, Anastasia Belova <abelova@astralinux.ru> wrote:
> >
> >
> >
> > 09/04/24 15:02, Peter Maydell пишет:
> > > On Tue, 9 Apr 2024 at 12:54, Anastasia Belova <abelova@astralinux.ru> wrote:
> > >> ch->num can reach values up to 31. Add casting to
> > >> a larger type before performing left shift to
> > >> prevent integer overflow.
> > > If ch->num can only reach up to 31, then 1 << ch->num
> > > is fine, because QEMU can assume that integers are 32 bits,
> > > and we compile with -fwrapv so there isn't a problem with
> > > shifting into the sign bit.
> >
> > Right, thanks for your comments.
> > I didn't know about this flag before. It became more clear for me now.
>
> Yep; if you're using a static analyser you probably want to
> configure it to accept the behaviours that are
> undefined-in-standard-C and which get defined behaviour
> with -fwrapv.
>
> This code is definitely a bit dubious, though, because
> ch_enable_mask is a uint64_t, so the intention was clearly
> to allow up to 64 channels. So I think we should take this
> patch anyway, with a slightly adjusted commit message.
>
> All the soc_dma.c code will probably be removed in the
> 9.2 release, because it's only used by the OMAP board models
> which we've just deprecated, so it doesn't seem worth spending
> too much time on cleaning up the code, but in this case you've
> already written the patch.
>
> I'll put this patch on my list to apply after we've made the
> 9.0 release and restarted development for 9.1.

Now applied to target-arm.next for 9.1 (with adjustments
to the commit message); thanks.

-- PMM