On Wed, Dec 16, 2020 at 10:07:31PM +0000, Radev, Martin wrote:
> 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
>

I believe the above example is without a SEV guest enabled/active, as SEV
guest debugging can only be done with SEV Debug patches applied.

> 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.

That seems to be true.

Thanks,
Ashish