[RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA

[RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA

[Peter Maydell]

This patchset is mainly trying to fix a problem that Coverity spotted
in the dcr_write_dma() function in hw/ppc/ppc440_uc.c, where the code
is not correctly using the cpu_physical_memory_map() function.
While I was fixing that I noticed a second problem in this code,
where it doesn't have a fallback for when cpu_physical_memory_map()
says "I couldn't map that for you".

I've marked these patches as RFC, partly because I don't have any
guest that would exercise the code changes[*], and partly because
I don't have any documentation of the hardware to tell me how it
should behave, so patch 2 in particular has some FIXMEs. I also
notice that the code doesn't update any of the registers like the
count or source/base addresses when the DMA transfer happens, which
seems odd, but perhaps the real hardware does work like that.

I think we should probably take patch 1 (which is a fairly minimal
fix of the use-of-uninitialized-data problem), but patch 2 is a bit
more unfinished.

[*] The commit 3c409c1927efde2fc that added this code says it's used
by AmigaOS.)

thanks
-- PMM

Peter Maydell (2):
  hw/ppc/ppc440_uc: Initialize length passed to
    cpu_physical_memory_map()
  hw/ppc/ppc440_uc: Handle mapping failure in DMA engine

 hw/ppc/ppc440_uc.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

-- 
2.25.1

[RFC 1/2] hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

[Peter Maydell]

In dcr_write_dma(), there is code that uses cpu_physical_memory_map()
to implement a DMA transfer.  That function takes a 'plen' argument,
which points to a hwaddr which is used for both input and output: the
caller must set it to the size of the range it wants to map, and on
return it is updated to the actual length mapped. The dcr_write_dma()
code fails to initialize rlen and wlen, so will end up mapping an
unpredictable amount of memory.

Initialize the length values correctly, and check that we managed to
map the entire range before using the fast-path memmove().

This was spotted by Coverity, which points out that we never
initialized the variables before using them.

Fixes: Coverity CID 1487137
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This seems totally broken, so I presume we just don't have any
guest code that actually exercises this...
---
 hw/ppc/ppc440_uc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index a1ecf6dd1c2..11fdb88c220 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -904,14 +904,17 @@ static void dcr_write_dma(void *opaque, int dcrn, uint32_t val)
                     int width, i, sidx, didx;
                     uint8_t *rptr, *wptr;
                     hwaddr rlen, wlen;
+                    hwaddr xferlen;
 
                     sidx = didx = 0;
                     width = 1 << ((val & DMA0_CR_PW) >> 25);
+                    xferlen = count * width;
+                    wlen = rlen = xferlen;
                     rptr = cpu_physical_memory_map(dma->ch[chnl].sa, &rlen,
                                                    false);
                     wptr = cpu_physical_memory_map(dma->ch[chnl].da, &wlen,
                                                    true);
-                    if (rptr && wptr) {
+                    if (rptr && rlen == xferlen && wptr && wlen == xferlen) {
                         if (!(val & DMA0_CR_DEC) &&
                             val & DMA0_CR_SAI && val & DMA0_CR_DAI) {
                             /* optimise common case */
-- 
2.25.1

[RFC 2/2] hw/ppc/ppc440_uc: Handle mapping failure in DMA engine

[Peter Maydell]

Currently the code for doing DMA in dcr_write_dma() has no fallback
code for if its calls to cpu_physical_memory_map() fail. Add
handling for this situation, by using address_space_read() and
address_space_write() to do the data transfers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I believe this to be equivalent to the fastpath code.  However, as
the comments note, I don't know what the intended behaviour on a DMA
memory access error is, because I couldn't find a datasheet for this
hardware.  I am also a bit suspicious that the current code does not
seem to update any of the count, source or destination addresses
after the memory transfer: is that really how the hardware behaves?
---
 hw/ppc/ppc440_uc.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index 11fdb88c220..0879f180a14 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -905,6 +905,7 @@ static void dcr_write_dma(void *opaque, int dcrn, uint32_t val)
                     uint8_t *rptr, *wptr;
                     hwaddr rlen, wlen;
                     hwaddr xferlen;
+                    bool fastpathed = false;
 
                     sidx = didx = 0;
                     width = 1 << ((val & DMA0_CR_PW) >> 25);
@@ -915,6 +916,7 @@ static void dcr_write_dma(void *opaque, int dcrn, uint32_t val)
                     wptr = cpu_physical_memory_map(dma->ch[chnl].da, &wlen,
                                                    true);
                     if (rptr && rlen == xferlen && wptr && wlen == xferlen) {
+                        fastpathed = true;
                         if (!(val & DMA0_CR_DEC) &&
                             val & DMA0_CR_SAI && val & DMA0_CR_DAI) {
                             /* optimise common case */
@@ -940,6 +942,33 @@ static void dcr_write_dma(void *opaque, int dcrn, uint32_t val)
                     if (rptr) {
                         cpu_physical_memory_unmap(rptr, rlen, 0, sidx);
                     }
+                    if (!fastpathed) {
+                        /* Fast-path failed, do each access one at a time */
+                        for (sidx = didx = i = 0; i < count; i++) {
+                            uint8_t buf[8];
+                            assert(width <= sizeof(buf));
+                            if (address_space_read(&address_space_memory,
+                                                   dma->ch[chnl].sa + sidx,
+                                                   MEMTXATTRS_UNSPECIFIED,
+                                                   buf, width) != MEMTX_OK) {
+                                /* FIXME: model correct behaviour on errors */
+                                break;
+                            }
+                            if (address_space_write(&address_space_memory,
+                                                    dma->ch[chnl].da + didx,
+                                                    MEMTXATTRS_UNSPECIFIED,
+                                                    buf, width) != MEMTX_OK) {
+                                /* FIXME: model correct behaviour on errors */
+                                break;
+                            }
+                            if (val & DMA0_CR_SAI) {
+                                sidx += width;
+                            }
+                            if (val & DMA0_CR_DAI) {
+                                didx += width;
+                            }
+                        }
+                    }
                 }
             }
             break;
-- 
2.25.1

Re: [RFC 1/2] hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

[Peter Maydell]

On Tue, 26 Jul 2022 at 19:23, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In dcr_write_dma(), there is code that uses cpu_physical_memory_map()
> to implement a DMA transfer.  That function takes a 'plen' argument,
> which points to a hwaddr which is used for both input and output: the
> caller must set it to the size of the range it wants to map, and on
> return it is updated to the actual length mapped. The dcr_write_dma()
> code fails to initialize rlen and wlen, so will end up mapping an
> unpredictable amount of memory.
>
> Initialize the length values correctly, and check that we managed to
> map the entire range before using the fast-path memmove().
>
> This was spotted by Coverity, which points out that we never
> initialized the variables before using them.
>
> Fixes: Coverity CID 1487137

Also CID 1487150 (it reports the wlen and rlen issues separately).

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [RFC 1/2] hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

[Richard Henderson]

On 7/26/22 11:23, Peter Maydell wrote:
> In dcr_write_dma(), there is code that uses cpu_physical_memory_map()
> to implement a DMA transfer.  That function takes a 'plen' argument,
> which points to a hwaddr which is used for both input and output: the
> caller must set it to the size of the range it wants to map, and on
> return it is updated to the actual length mapped. The dcr_write_dma()
> code fails to initialize rlen and wlen, so will end up mapping an
> unpredictable amount of memory.
> 
> Initialize the length values correctly, and check that we managed to
> map the entire range before using the fast-path memmove().
> 
> This was spotted by Coverity, which points out that we never
> initialized the variables before using them.
> 
> Fixes: Coverity CID 1487137
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> This seems totally broken, so I presume we just don't have any
> guest code that actually exercises this...
> ---
>   hw/ppc/ppc440_uc.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA

[Cédric Le Goater]

On 7/26/22 20:23, Peter Maydell wrote:
> This patchset is mainly trying to fix a problem that Coverity spotted
> in the dcr_write_dma() function in hw/ppc/ppc440_uc.c, where the code
> is not correctly using the cpu_physical_memory_map() function.
> While I was fixing that I noticed a second problem in this code,
> where it doesn't have a fallback for when cpu_physical_memory_map()
> says "I couldn't map that for you".
> 
> I've marked these patches as RFC, partly because I don't have any
> guest that would exercise the code changes[*], 

I build these :

   https://github.com/legoater/qemu-ppc-boot/tree/main/buildroot/qemu_ppc_sam460ex-2022.02-4-geae5011c83-20220309

but none of the DCR DMA registers are used.

There are images for the sam460ex images here :

   http://www.aros.org/nightly1.php

But AFAICT, it does not go beyond the bootloader.

> and partly because
> I don't have any documentation of the hardware to tell me how it
> should behave, so patch 2 in particular has some FIXMEs. I also
> notice that the code doesn't update any of the registers like the
> count or source/base addresses when the DMA transfer happens, which
> seems odd, but perhaps the real hardware does work like that.
> 
> I think we should probably take patch 1 (which is a fairly minimal
> fix of the use-of-uninitialized-data problem),

LGTM,

Thanks,

C.




> but patch 2 is a bit more unfinished.
> 
> [*] The commit 3c409c1927efde2fc that added this code says it's used
> by AmigaOS.)
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>    hw/ppc/ppc440_uc: Initialize length passed to
>      cpu_physical_memory_map()
>    hw/ppc/ppc440_uc: Handle mapping failure in DMA engine
> 
>   hw/ppc/ppc440_uc.c | 34 +++++++++++++++++++++++++++++++++-
>   1 file changed, 33 insertions(+), 1 deletion(-)
> 

Re: [RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4021 bytes --]

On Wed, 27 Jul 2022, Cédric Le Goater wrote:
> On 7/26/22 20:23, Peter Maydell wrote:
>> This patchset is mainly trying to fix a problem that Coverity spotted
>> in the dcr_write_dma() function in hw/ppc/ppc440_uc.c, where the code
>> is not correctly using the cpu_physical_memory_map() function.

Likely I did not know how this function works when implementing it and may 
have used it wrong but none of the reviews spotted it either. (I may have 
used some other DMA device model as an inspiration but don't remember 
which.)

>> While I was fixing that I noticed a second problem in this code,
>> where it doesn't have a fallback for when cpu_physical_memory_map()
>> says "I couldn't map that for you".

When can that happen? If only in cases when guest gives invalid parameters 
then maybe we don't have to bother with that and can let it fail but 
having a fallback does not hurt.

>> I've marked these patches as RFC, partly because I don't have any
>> guest that would exercise the code changes[*], 
>
> I build these :
>
>  https://github.com/legoater/qemu-ppc-boot/tree/main/buildroot/qemu_ppc_sam460ex-2022.02-4-geae5011c83-20220309
>
> but none of the DCR DMA registers are used.

To my knowledge only AmigaOS needs it but that does use it and it boots 
and runs so this is not completely broken but maybe mapping random length 
memory areas could cause some problems if the length mapped is less than 
needed. I've seen random crashes with AmigaOS before that I could not 
explain but that seems to be related to processor speed (happens more on 
slower host) so it seems more like a race condition not wrong DMA and 
haven't seen it recently.

> There are images for the sam460ex images here :
>
>  http://www.aros.org/nightly1.php
>
> But AFAICT, it does not go beyond the bootloader.

Those did boot before (may take a while due to bitbanging i2c driver in 
guest taking time to read RTC) but haven't checked for a while. It could 
be some changes in AROS broke it or some recent change in QEMU. I'll test 
these later when I'll have some time.

>> and partly because
>> I don't have any documentation of the hardware to tell me how it
>> should behave, so patch 2 in particular has some FIXMEs. I also

I remember I've found some info on this in some similar SoC that had docs 
on-line but don't remember which. Maybe 440EPx/GPx or something like that. 
It may not be the same but seems similar enough for AmigaOS to work. I 
think the two main sources were PPC440EPx/GRx Embedded Processor User's 
Manual and NXP/Freescale Application Note AN2661 Software Migration from 
the IBM (AMCC) 440GP to the MPC8540 which seem to be similar to 460EX and 
have some info on the DMA controller registers.

>> notice that the code doesn't update any of the registers like the
>> count or source/base addresses when the DMA transfer happens, which
>> seems odd, but perhaps the real hardware does work like that.

The emulation is only partial, I were only concerned about the parts 
AmigaOS uses so it may be some things are off. The docs above talk about 
some increment bits so this may be settable but I did not read the whole 
docs again and don't remember anything about it now. Probably AmigaOS does 
not use it so I did not implement that.

I was going to test the first rc with all OSes I have on sam460ex and 
pegasos2 but maybe I'll try to do it this week then.

Regards,
BALATON Zoltan

>> I think we should probably take patch 1 (which is a fairly minimal
>> fix of the use-of-uninitialized-data problem),
>
> LGTM,
>
> Thanks,
>
> C.
>
>
>
>
>> but patch 2 is a bit more unfinished.
>> 
>> [*] The commit 3c409c1927efde2fc that added this code says it's used
>> by AmigaOS.)
>> 
>> thanks
>> -- PMM
>> 
>> Peter Maydell (2):
>>    hw/ppc/ppc440_uc: Initialize length passed to
>>      cpu_physical_memory_map()
>>    hw/ppc/ppc440_uc: Handle mapping failure in DMA engine
>>
>>   hw/ppc/ppc440_uc.c | 34 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 33 insertions(+), 1 deletion(-)
>> 
>
>

Re: [RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA

[Peter Maydell]

On Wed, 27 Jul 2022 at 12:55, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Wed, 27 Jul 2022, Cédric Le Goater wrote:
> > On 7/26/22 20:23, Peter Maydell wrote:
> >> This patchset is mainly trying to fix a problem that Coverity spotted
> >> in the dcr_write_dma() function in hw/ppc/ppc440_uc.c, where the code
> >> is not correctly using the cpu_physical_memory_map() function.
>
> Likely I did not know how this function works when implementing it and may
> have used it wrong but none of the reviews spotted it either. (I may have
> used some other DMA device model as an inspiration but don't remember
> which.)
>
> >> While I was fixing that I noticed a second problem in this code,
> >> where it doesn't have a fallback for when cpu_physical_memory_map()
> >> says "I couldn't map that for you".
>
> When can that happen? If only in cases when guest gives invalid parameters
> then maybe we don't have to bother with that and can let it fail but
> having a fallback does not hurt.

Mostly it happens when the thing being DMA'd to or from is not RAM.
Ordinarily I wouldn't expect that to be likely, but the DMA device
here has a "don't advance the src/destination" option which I assume
would be used for things like DMA'ing to or from a device FIFO.
Perhaps AmigaOS doesn't in practice do that.

> >> and partly because
> >> I don't have any documentation of the hardware to tell me how it
> >> should behave, so patch 2 in particular has some FIXMEs. I also
>
> I remember I've found some info on this in some similar SoC that had docs
> on-line but don't remember which. Maybe 440EPx/GPx or something like that.
> It may not be the same but seems similar enough for AmigaOS to work. I
> think the two main sources were PPC440EPx/GRx Embedded Processor User's
> Manual and NXP/Freescale Application Note AN2661 Software Migration from
> the IBM (AMCC) 440GP to the MPC8540 which seem to be similar to 460EX and
> have some info on the DMA controller registers.

Thanks, I'll see if I can track those down.

-- PMM

Re: [RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA

[Peter Maydell]

On Wed, 27 Jul 2022 at 14:01, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 27 Jul 2022 at 12:55, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >
> > On Wed, 27 Jul 2022, Cédric Le Goater wrote:
> > > On 7/26/22 20:23, Peter Maydell wrote:
> > >> This patchset is mainly trying to fix a problem that Coverity spotted
> > >> in the dcr_write_dma() function in hw/ppc/ppc440_uc.c, where the code
> > >> is not correctly using the cpu_physical_memory_map() function.
> >
> > Likely I did not know how this function works when implementing it and may
> > have used it wrong but none of the reviews spotted it either. (I may have
> > used some other DMA device model as an inspiration but don't remember
> > which.)
> >
> > >> While I was fixing that I noticed a second problem in this code,
> > >> where it doesn't have a fallback for when cpu_physical_memory_map()
> > >> says "I couldn't map that for you".
> >
> > When can that happen? If only in cases when guest gives invalid parameters
> > then maybe we don't have to bother with that and can let it fail but
> > having a fallback does not hurt.
>
> Mostly it happens when the thing being DMA'd to or from is not RAM.
> Ordinarily I wouldn't expect that to be likely, but the DMA device
> here has a "don't advance the src/destination" option which I assume
> would be used for things like DMA'ing to or from a device FIFO.
> Perhaps AmigaOS doesn't in practice do that.

Oh, and we should probably use the 'call address_space_read/write'
fallback for all cases of "don't advance the pointer", because
address_space_map() will not handle that correctly -- it will
typically return a 'bounce buffer' and then copy the bounce buffer
to the device, so the effect will be like reading/writing the
FIFO device only once instead of reading/writing all the data.

thanks
-- PMM

Re: [RFC 1/2] hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

[Daniel Henrique Barboza]

On 7/26/22 15:24, Peter Maydell wrote:
> On Tue, 26 Jul 2022 at 19:23, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> In dcr_write_dma(), there is code that uses cpu_physical_memory_map()
>> to implement a DMA transfer.  That function takes a 'plen' argument,
>> which points to a hwaddr which is used for both input and output: the
>> caller must set it to the size of the range it wants to map, and on
>> return it is updated to the actual length mapped. The dcr_write_dma()
>> code fails to initialize rlen and wlen, so will end up mapping an
>> unpredictable amount of memory.
>>
>> Initialize the length values correctly, and check that we managed to
>> map the entire range before using the fast-path memmove().
>>
>> This was spotted by Coverity, which points out that we never
>> initialized the variables before using them.
>>
>> Fixes: Coverity CID 1487137
> 
> Also CID 1487150 (it reports the wlen and rlen issues separately).

I amended in tree:


     Fixes: Coverity CID 1487137, 1487150


I also believe that this Coverity fix isn't dependent on patch 2, which
apparently will take longer to get reviewed, so it's fine to take it
now.



Thanks,


Daniel




> 
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> -- PMM

Re: [RFC 1/2] hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

[Peter Maydell]

On Wed, 27 Jul 2022 at 15:11, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
>
>
> On 7/26/22 15:24, Peter Maydell wrote:
> > On Tue, 26 Jul 2022 at 19:23, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> In dcr_write_dma(), there is code that uses cpu_physical_memory_map()
> >> to implement a DMA transfer.  That function takes a 'plen' argument,
> >> which points to a hwaddr which is used for both input and output: the
> >> caller must set it to the size of the range it wants to map, and on
> >> return it is updated to the actual length mapped. The dcr_write_dma()
> >> code fails to initialize rlen and wlen, so will end up mapping an
> >> unpredictable amount of memory.
> >>
> >> Initialize the length values correctly, and check that we managed to
> >> map the entire range before using the fast-path memmove().
> >>
> >> This was spotted by Coverity, which points out that we never
> >> initialized the variables before using them.
> >>
> >> Fixes: Coverity CID 1487137
> >
> > Also CID 1487150 (it reports the wlen and rlen issues separately).
>
> I amended in tree:
>
>
>      Fixes: Coverity CID 1487137, 1487150
>
>
> I also believe that this Coverity fix isn't dependent on patch 2, which
> apparently will take longer to get reviewed, so it's fine to take it
> now.

Correct, and thank you.

-- PMM

Re: [RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA

[BALATON Zoltan]

On Tue, 26 Jul 2022, Peter Maydell wrote:
> This patchset is mainly trying to fix a problem that Coverity spotted
> in the dcr_write_dma() function in hw/ppc/ppc440_uc.c, where the code
> is not correctly using the cpu_physical_memory_map() function.
> While I was fixing that I noticed a second problem in this code,
> where it doesn't have a fallback for when cpu_physical_memory_map()
> says "I couldn't map that for you".
>
> I've marked these patches as RFC, partly because I don't have any
> guest that would exercise the code changes[*], and partly because
> I don't have any documentation of the hardware to tell me how it
> should behave, so patch 2 in particular has some FIXMEs. I also
> notice that the code doesn't update any of the registers like the
> count or source/base addresses when the DMA transfer happens, which
> seems odd, but perhaps the real hardware does work like that.
>
> I think we should probably take patch 1 (which is a fairly minimal
> fix of the use-of-uninitialized-data problem), but patch 2 is a bit
> more unfinished.
>
> [*] The commit 3c409c1927efde2fc that added this code says it's used
> by AmigaOS.)

AmigaOS still boots with these patches and I see no difference so

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

(I did not check what parameters AmigaOS uses (could not find a simple 
trace option for that, may need to add some debug printfs to test that) so 
not sure if the added code was actually run or it still only uses the code 
path as before. Fixing the map length should show some effect but I don't 
see any.)

Regards,
BALATON Zoltan

> thanks
> -- PMM
>
> Peter Maydell (2):
>  hw/ppc/ppc440_uc: Initialize length passed to
>    cpu_physical_memory_map()
>  hw/ppc/ppc440_uc: Handle mapping failure in DMA engine
>
> hw/ppc/ppc440_uc.c | 34 +++++++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
>