Hello everybody, I will try help clarify some things. > On a DMA unmap SWIOTLB (when force is used) it trusts the driver from providing > the correct DMA address and length which SWIOTLB uses to match to its associated > original PA address. > The length is not checked so the attacker can modify that to say a huge number > and cause SWIOTLB bounce code to write or read data from non SWIOTLB PA into the > SWIOTLB PA pool. This is true. As an example, I attached to the QEMU process, set a BP to `virtqueue_split_fill` and modified the length field from 0x40 to 0x10000, and filled the corresponding buffer in the swiotlb region with As (0x41). Immediately after resuming execution, the kernel would crash: [ 122.154142] general protection fault, probably for non-canonical address 0x4141414141414141: 0000 [#1] PREEMPT SMP NOPTI [ 122.156088] CPU: 0 PID: 917 Comm: kworker/0:6 Kdump: loaded Tainted: G W E 5.6.12-sevault+ #28 [ 122.157855] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015 [ 122.159079] Workqueue: events_freezable_power_ disk_events_workfn [ 122.160040] RIP: 0010:scsi_queue_rq+0x5af/0xa70 [scsi_mod] [ 122.160916] Code: 01 89 83 9c 02 00 00 41 80 7f 08 00 74 07 83 8b 9c 02 00 00 08 48 8b 83 40 02 00 00 c7 83 3c 01 00 00 00 00 00 00 48 8d 78 08 <48> c7 00 00 00 00 00 48 c7 40 58 00 00 00 00 48 83 e7 f8 48 29 f8 [ 122.163821] RSP: 0018:ffffc900002efb08 EFLAGS: 00010202 [ 122.164637] RAX: 4141414141414141 RBX: ffff888035b89c00 RCX: ffff888035b89ed0 [ 122.165775] RDX: 0000000000000008 RSI: 0000000000000000 RDI: 4141414141414149 [ 122.166891] RBP: ffff888035946000 R08: ffff888035a79860 R09: 0000000000000000 [ 122.168016] R10: ffffea0001287280 R11: 0000000000000008 R12: ffff888035b89d18 [ 122.169159] R13: ffff888035945000 R14: ffff888035946000 R15: ffffc900002efba0 [ 122.170287] FS: 0000000000000000(0000) GS:ffff88807f800000(0000) knlGS:0000000000000000 [ 122.171564] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 122.172470] CR2: 0000560e654b77b8 CR3: 000000004dd38000 CR4: 00000000003406f0 What and where gets overwritten entirely depends on what virtio driver is being targeted. All manage their memory for the descriptor buffers differently so the overwrite may require to be large. In the context of VirtIO and SWIOTLB, there are also these three fields other than the length: dma_addr, flags, next I had a look around a little bit, so my take is the following: 1) There's already validation for dma_addr before doing the unmap with a call to is_swiotlb_buffer (1). I think this check is sufficient. 2) flags Before doing the unmap, the virtio implementation would check the flag and based on it would select a DMA direction (TO/FROM DEVICE). Still, it seems that this would not trick the driver to copy data to the device since only a `sync for CPU` may be performed in the unmap path. However, I can see that this would eliminate a memcpy to any passed buffer which means that an uninitialized buffer may be passed to some client like a user space application. I tested that with the device `virtio-rng` which would leak some stored kernel address. 3) next I think this was reported by Felicitas that this can lead to an OOB access, but it is limited. So, I think that the problem of overwriting the addr and flags fields are not a big issue on their own. But there should be some validation for the size and that should be probably done the SWIOTLB implementation level. There's already an array to keep the original address (3), What about adding an extra array to keep track of also the `original size`. It would be populated when some memory is mapped, just like with io_tlb_orig_addr (4). Then the validation can be added in (5) and (6). This swiotlb/virtio issue affects the AMD SEV features where swiotlb is always forcefully enabled. I can also see that SWIOTLB is also always enabled for s390 but I don't know their threat model. [1] https://elixir.bootlin.com/linux/v5.10/source/include/linux/swiotlb.h#L72 [2] https://elixir.bootlin.com/linux/v5.8/source/drivers/virtio/virtio_ring.c#L381 [3] https://elixir.bootlin.com/linux/v5.8/source/kernel/dma/swiotlb.c#L103 [4] https://elixir.bootlin.com/linux/v5.8/source/kernel/dma/swiotlb.c#L570 [5] https://elixir.bootlin.com/linux/v5.8/source/kernel/dma/swiotlb.c#L588 [6] https://elixir.bootlin.com/linux/v5.8/source/kernel/dma/swiotlb.c#L633 ________________________________ From: Konrad Rzeszutek Wilk Sent: Wednesday, December 16, 2020 2:07 PM To: Michael S. Tsirkin ; Jason Wang Cc: Felicitas Hetzelt ; ashish.kalra@amd.com ; jun.nakajima@intel.com ; hch@lst.de ; virtualization@lists.linux-foundation.org ; iommu@lists.linux-foundation.org ; Radev, Martin ; Morbitzer, Mathias ; Robert Buhren ; david.kaplan@amd.com Subject: Re: swiotlb/virtio: unchecked device dma address and length ..snip.. >> > > This raises two issues: >> > > 1) swiotlb_tlb_unmap_single fails to check whether the index >generated >> > > from the dma_addr is in range of the io_tlb_orig_addr array. >> > That is fairly simple to implement I would think. That is it can >check >> > that the dma_addr is from the PA in the io_tlb pool when >SWIOTLB=force >> > is used. >> >> >> I'm not sure this can fix all the cases. It looks to me we should map >> descriptor coherent but readonly (which is not supported by current >DMA >> API). > >Neither is this supported but encrypted memory technologies. -ECONFUSED. Could you state this once more please? I am not exactly sure what you are saying > >> Otherwise, device can modify the desc[i].addr/desc[i].len at any time >to >> pretend a valid mapping. >> >> Thanks >> >> >> >