| Message ID | 20220726182341.1888115-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix Coverity and other errors in ppc440_uc DMA | expand |
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
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~
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
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 --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 */
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(-)