From b51ae104c2a381c1634f6a0d4a18192cb2297831 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josef=20S=C3=B6ntgen?= Date: Fri, 12 Feb 2021 12:55:44 +0100 Subject: [PATCH] qemu-usb: use bounce buffer to access DMA memory The former implemention assumed that the guest physical memory is mapped continously. This, however, is not true. Writing larger files to an USB stick with a Windows 10 guest would therefore lead to data corruption. The current implementation uses a bounce buffer to copy the data to and from the guest physical memory and leaves dealing with the memory mappings entirely up to the VMM. Fixes #4017. --- repos/libports/include/qemu/usb.h | 6 +- repos/libports/src/lib/qemu-usb/qemu_emul.cc | 13 +++- repos/ports/src/virtualbox5/devxhci.cc | 68 +++++++++++++++++--- 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/repos/libports/include/qemu/usb.h b/repos/libports/include/qemu/usb.h index 5a001595ae..86bc781d7e 100644 --- a/repos/libports/include/qemu/usb.h +++ b/repos/libports/include/qemu/usb.h @@ -57,6 +57,8 @@ namespace Qemu { */ struct Pci_device { + enum class Dma_direction { IN = 0, OUT = 1, }; + /** * Raise interrupt * @@ -65,8 +67,8 @@ namespace Qemu { virtual void raise_interrupt(int assert) = 0; virtual int read_dma(addr_t addr, void *buf, size_t size) = 0; virtual int write_dma(addr_t addr, void const *buf, size_t size) = 0; - virtual void *map_dma(addr_t base, size_t size) = 0; - virtual void unmap_dma(void *addr, size_t size) = 0; + virtual void *map_dma(addr_t base, size_t size, Dma_direction dir) = 0; + virtual void unmap_dma(void *addr, size_t size, Dma_direction dir) = 0; }; diff --git a/repos/libports/src/lib/qemu-usb/qemu_emul.cc b/repos/libports/src/lib/qemu-usb/qemu_emul.cc index cd0e13d28a..926627bc8e 100644 --- a/repos/libports/src/lib/qemu-usb/qemu_emul.cc +++ b/repos/libports/src/lib/qemu-usb/qemu_emul.cc @@ -849,6 +849,10 @@ void qemu_sglist_destroy(QEMUSGList *sgl) { int usb_packet_map(USBPacket *p, QEMUSGList *sgl) { + Qemu::Pci_device::Dma_direction dir = + (p->pid == USB_TOKEN_IN) ? Qemu::Pci_device::Dma_direction::IN + : Qemu::Pci_device::Dma_direction::OUT; + void *mem; for (int i = 0; i < sgl->niov; i++) { @@ -857,7 +861,7 @@ int usb_packet_map(USBPacket *p, QEMUSGList *sgl) while (len) { dma_addr_t xlen = len; - mem = _pci_device->map_dma(base, xlen); + mem = _pci_device->map_dma(base, xlen, dir); if (verbose_iov) Genode::log("mem: ", mem, " base: ", (void *)base, " len: ", Genode::Hex(len)); @@ -884,9 +888,14 @@ err: void usb_packet_unmap(USBPacket *p, QEMUSGList *sgl) { + Qemu::Pci_device::Dma_direction dir = + (p->pid == USB_TOKEN_IN) ? Qemu::Pci_device::Dma_direction::IN + : Qemu::Pci_device::Dma_direction::OUT; + for (int i = 0; i < p->iov.niov; i++) { _pci_device->unmap_dma(p->iov.iov[i].iov_base, - p->iov.iov[i].iov_len); + p->iov.iov[i].iov_len, + dir); } } diff --git a/repos/ports/src/virtualbox5/devxhci.cc b/repos/ports/src/virtualbox5/devxhci.cc index ca685a56a2..cd68cab1d2 100644 --- a/repos/ports/src/virtualbox5/devxhci.cc +++ b/repos/ports/src/virtualbox5/devxhci.cc @@ -15,6 +15,8 @@ /* Genode includes */ #include #include +#include +#include #include /* qemu-usb includes */ @@ -285,8 +287,33 @@ struct Timer_queue : public Qemu::Timer_queue struct Pci_device : public Qemu::Pci_device { + Libc::Allocator _alloc { }; + PPDMDEVINS pci_dev; + struct Dma_bounce_buffer + { + Genode::Allocator &_alloc; + + Qemu::addr_t const base; + Qemu::size_t const size; + void * const addr { _alloc.alloc(size) }; + + Dma_bounce_buffer(Genode::Allocator &alloc, + Qemu::addr_t base, + Qemu::size_t size) + : _alloc { alloc }, base { base }, size { size } + { } + + virtual ~Dma_bounce_buffer() + { + _alloc.free(addr, size); + } + }; + + using Reg_dma_buffer = Genode::Registered; + Genode::Registry _dma_buffers { }; + Pci_device(PPDMDEVINS pDevIns) : pci_dev(pDevIns) { } void raise_interrupt(int level) override { @@ -298,20 +325,43 @@ struct Pci_device : public Qemu::Pci_device int write_dma(Qemu::addr_t addr, void const *buf, Qemu::size_t size) override { return PDMDevHlpPhysWrite(pci_dev, addr, buf, size); } - void *map_dma(Qemu::addr_t base, Qemu::size_t size) override + void *map_dma(Qemu::addr_t base, Qemu::size_t size, + Qemu::Pci_device::Dma_direction dir) override { - PGMPAGEMAPLOCK lock; - void * vmm_addr = nullptr; + Reg_dma_buffer *dma = nullptr; - int rc = PDMDevHlpPhysGCPhys2CCPtr(pci_dev, base, 0, &vmm_addr, &lock); - Assert(rc == VINF_SUCCESS); + try { + dma = new (_alloc) Reg_dma_buffer(_dma_buffers, + _alloc, base, size); + } catch (...) { + return nullptr; + } - /* the mapping doesn't go away, so release internal lock immediately */ - PDMDevHlpPhysReleasePageMappingLock(pci_dev, &lock); - return vmm_addr; + /* copy data for write request to bounce buffer */ + if (dir == Qemu::Pci_device::Dma_direction::OUT) { + (void)PDMDevHlpPhysRead(pci_dev, base, dma->addr, size); + } + + return dma->addr; } - void unmap_dma(void *addr, Qemu::size_t size) override { } + void unmap_dma(void *addr, Qemu::size_t size, + Qemu::Pci_device::Dma_direction dir) override + { + _dma_buffers.for_each([&] (Reg_dma_buffer &dma) { + if (dma.addr != addr) { + return; + } + + /* copy data for read request from bounce buffer */ + if (dir == Qemu::Pci_device::Dma_direction::IN) { + (void)PDMDevHlpPhysWrite(pci_dev, + dma.base, dma.addr, dma.size); + } + + Genode::destroy(_alloc, &dma); + }); + } };