diff mbox series

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

Message ID 20220726182341.1888115-2-peter.maydell@linaro.org
State Superseded
Headers show
Series Fix Coverity and other errors in ppc440_uc DMA | expand

Commit Message

Peter Maydell July 26, 2022, 6:23 p.m. UTC
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(-)

Comments

Peter Maydell July 26, 2022, 6:24 p.m. UTC | #1
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
Richard Henderson July 26, 2022, 7:35 p.m. UTC | #2
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~
Daniel Henrique Barboza July 27, 2022, 2:11 p.m. UTC | #3
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
Peter Maydell July 28, 2022, 9:43 a.m. UTC | #4
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
diff mbox series

Patch

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 */