The soc_dma code has a fastpath for when DMA transfers are from RAM to RAM. The current implementation of this has the caller of soc_dma_port_add_mem() pass the underlying host address of the RAM block that the DMA port is connected to (obtained via memory_region_get_ram_ptr()). Then the actual transfer function does a simple memcpy(). This has several problems.
Most importantly, no bounds checking is done on the address and size passed by the guest, so the memcpy source and destination might be outside the backing host RAM entirely. Secondly, because the DMA access is done via this back door, there is no updating of the dirty region when memory is written this way (there is a TODO comment in omap_dma.c noting this). Fix both of these by making the memory to memory transfer function use physical_memory_map() to get the host addresses for the memory copy. That function will automatically give us the bounds check that we want and return a short length if the transfer would run off the end of the RAM MemoryRegion it starts in. Since the OMAP DMA documentation states that it's a guest error to misprogram the addresses so that they fall outside the range that is valid for the particular DMA port being addressed and that this can result in guest memory corruption , we don't need to loop for short transfers, but can simply log them and continue. Note that we don't need to update addresses or bytecount here in the transfer function, because when soc_dma_ch_update() selects transfer_mem2mem it also sets ch->update to 1, which tells the omap_dma_transfer_setup() code that it is responsible for updating all the guest visible fields to match "transfer completed". (We use physical_memory_map() here to match the use of physical_memory_read() and physical_memory_write() in omap_dma.c; making the DMA controller use an explicit AddressSpace would be a separate cleanup task.) Together with the preceding commits that fixed some integer overflow problems, this fixes the "guest can provoke a bad memcpy() operation" reported in issue #3204. Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3204 Signed-off-by: Peter Maydell <[email protected]> --- hw/dma/omap_dma.c | 5 ----- hw/dma/soc_dma.c | 51 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/hw/dma/omap_dma.c b/hw/dma/omap_dma.c index 14b310bf40..1a3ae74726 100644 --- a/hw/dma/omap_dma.c +++ b/hw/dma/omap_dma.c @@ -559,11 +559,6 @@ static void omap_dma_transfer_setup(struct soc_dma_ch_s *dma) if (!ch->sync && frames) { ch->cpc = a->dest & 0xffff; } - - /* - * TODO: if the destination port is IMIF or EMIFF, set the dirty - * bits on it. - */ } omap_dma_interrupts_update(s); diff --git a/hw/dma/soc_dma.c b/hw/dma/soc_dma.c index 1b93aaf565..6e325122d9 100644 --- a/hw/dma/soc_dma.c +++ b/hw/dma/soc_dma.c @@ -20,13 +20,58 @@ #include "qemu/osdep.h" #include "qemu/error-report.h" #include "qemu/timer.h" +#include "qemu/log.h" +#include "system/physmem.h" #include "hw/arm/soc_dma.h" static void transfer_mem2mem(struct soc_dma_ch_s *ch) { - memcpy(ch->paddr[0], ch->paddr[1], ch->bytes); - ch->paddr[0] += ch->bytes; - ch->paddr[1] += ch->bytes; + /* + * Memory-to-memory transfer: do the whole thing in one go. The + * hardware spec says that it is invalid to program the OMAP DMA + * controller with addresses that don't match the port (i.e. to + * ask for a transfer to/from a memory port with a physaddr that + * isn't within that port range) and that if you do then the + * transfer continues and memory can be corrupted. So we can map + * both source and destination, and treat short mappings and + * failed mappings as a guest error. + */ + hwaddr srclen = ch->bytes; + hwaddr dstlen = ch->bytes; + hwaddr srcaddr = ch->vaddr[0]; + hwaddr dstaddr = ch->vaddr[1]; + void *srcmem, *dstmem; + hwaddr xferlen = 0; + + srcmem = physical_memory_map(srcaddr, &srclen, false); + if (!srcmem) { + qemu_log_mask(LOG_GUEST_ERROR, + "soc_dma mem2mem transfer: could not map source; " + "guest error programming source port/address\n"); + return; + } + + dstmem = physical_memory_map(dstaddr, &dstlen, true); + if (!dstmem) { + qemu_log_mask(LOG_GUEST_ERROR, + "soc_dma mem2mem transfer: could not map destination; " + "guest error programming destination port/address\n"); + goto unmap_src; + } + + xferlen = MIN(srclen, dstlen); + if (xferlen < ch->bytes) { + qemu_log_mask(LOG_GUEST_ERROR, + "soc_dma mem2mem transfer: could not transfer all data; " + "guest error programming src or destination addresses\n"); + /* Continue to transfer whatever did fit in the port window */ + } + + memmove(dstmem, srcmem, xferlen); + + physical_memory_unmap(dstmem, dstlen, true, xferlen); +unmap_src: + physical_memory_unmap(srcmem, srclen, false, xferlen); } struct dma_s { -- 2.43.0
