| Message ID | 20200903110831.353476-12-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Forbid DMA write accesses to MMIO regions | expand |
On 03/09/20 13:08, Philippe Mathieu-Daudé wrote: > Do not allow PCI slaves to write to indirect memory > regions such MMIO. > > This fixes LP#1886362 and LP#1888606. What is a "PCI slave"? Which devices would still be allowed to write? I'm worried that there are cases of MMIO reads that would be broken. They are certainly niche these days, but they should still work; the most "famous" one is perhaps the old BASIC DEF SEG=&HB800 BLOAD "picture.pic", 0 Paolo
On 9/3/20 2:26 PM, Paolo Bonzini wrote: > On 03/09/20 13:08, Philippe Mathieu-Daudé wrote: >> Do not allow PCI slaves to write to indirect memory >> regions such MMIO. >> >> This fixes LP#1886362 and LP#1888606. > > What is a "PCI slave"? TBH I at a quick look at the PCIe SPEC, but not PCI. PCIe might be able to check the requester_id to check if the access is valid, but I'm not sure where to add this check. > Which devices would still be allowed to write? As of this patch, all the non-PCI, but I plan to add a similar check for USB on top of this series. > I'm worried that there are cases of MMIO reads that would be broken. > They are certainly niche these days, but they should still work; the > most "famous" one is perhaps the old BASIC > > DEF SEG=&HB800 > BLOAD "picture.pic", 0 This looks like ISA stuff. I don't think ISA does such checks (and didn't plan to add them there) but I'd need to verify. Do you have an acceptance test? > > Paolo >
On 03/09/20 15:18, Philippe Mathieu-Daudé wrote: > As of this patch, all the non-PCI, but I plan to add a similar > check for USB on top of this series. Do you mean for memory-mapped USB host controllers? >> I'm worried that there are cases of MMIO reads that would be broken. >> They are certainly niche these days, but they should still work; the >> most "famous" one is perhaps the old BASIC >> >> DEF SEG=&HB800 >> BLOAD "picture.pic", 0 > > This looks like ISA stuff. I don't think ISA does such checks > (and didn't plan to add them there) but I'd need to verify. It works on bare metal even with a modern video card. > Do you have an acceptance test? Nope. :( Paolo
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 8f901e6c289..cd97268b3a8 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -788,8 +788,12 @@ static inline AddressSpace *pci_get_address_space(PCIDevice *dev) static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr, void *buf, dma_addr_t len, DMADirection dir) { + MemTxAttrs attrs = { + .direct_access = (dir == DMA_DIRECTION_FROM_DEVICE), + .requester_id = pci_requester_id(dev), + }; return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, - dir, MEMTXATTRS_UNSPECIFIED); + dir, attrs); } static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr, @@ -808,14 +812,18 @@ static inline int pci_dma_write(PCIDevice *dev, dma_addr_t addr, static inline uint##_bits##_t ld##_l##_pci_dma(PCIDevice *dev, \ dma_addr_t addr) \ { \ - return ld##_l##_dma(pci_get_address_space(dev), addr, \ - MEMTXATTRS_UNSPECIFIED); \ + MemTxAttrs attrs = { \ + .requester_id = pci_requester_id(dev), \ + }; \ + return ld##_l##_dma(pci_get_address_space(dev), addr, attrs); \ } \ static inline void st##_s##_pci_dma(PCIDevice *dev, \ dma_addr_t addr, uint##_bits##_t val) \ { \ - st##_s##_dma(pci_get_address_space(dev), addr, val, \ - MEMTXATTRS_UNSPECIFIED); \ + MemTxAttrs attrs = { \ + .requester_id = pci_requester_id(dev), \ + }; \ + st##_s##_dma(pci_get_address_space(dev), addr, val, attrs); \ } PCI_DMA_DEFINE_LDST(ub, b, 8);
Do not allow PCI slaves to write to indirect memory regions such MMIO. This fixes LP#1886362 and LP#1888606. Example with the former reproducer: $ cat << EOF | \ qemu-system-i386 -M q35,accel=qtest \ -qtest stdio \ -trace memory_access\* \ outl 0xcf8 0x80001010 outl 0xcfc 0xe1020000 outl 0xcf8 0x80001014 outl 0xcf8 0x80001004 outw 0xcfc 0x7 outl 0xcf8 0x800010a2 write 0xe102003b 0x1 0xff write 0xe1020103 0x1e 0xffffff055c5e5c30be4511d084ffffffffffffffffffffffffffffffffff write 0xe1020420 0x4 0xffffffff write 0xe1020424 0x4 0xffffffff write 0xe102042b 0x1 0xff write 0xe1020430 0x4 0x055c5e5c write 0x5c041 0x1 0x04 write 0x5c042 0x1 0x02 write 0x5c043 0x1 0xe1 write 0x5c048 0x1 0x8a write 0x5c04a 0x1 0x31 write 0x5c04b 0x1 0xff write 0xe1020403 0x1 0xff EOF 562564:memory_access_illegal is_write:1 addr:0xe1020400 size:0x000e region:'e1000e-mmio' 562592:memory_access_illegal is_write:1 addr:0xe102040e size:0x007c region:'e1000e-mmio' 562601:memory_access_illegal is_write:1 addr:0xe102048a size:0x0004 region:'e1000e-mmio' Reported-by: Alexander Bulekov <alxndr@bu.edu> Buglink: https://bugs.launchpad.net/qemu/+bug/1886362 Buglink: https://bugs.launchpad.net/qemu/+bug/1888606 Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/hw/pci/pci.h | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)