Re: swiotlb/virtio: unchecked device dma address and length

From: Ashish Kalra <ashish.kalra@amd.com>
To: "Radev, Martin" <martin.radev@aisec.fraunhofer.de>
Cc: Thomas.Lendacky@amd.com, Jon.Grimm@amd.com,
	Felicitas Hetzelt <file@sect.tu-berlin.de>,
	"david.kaplan@amd.com" <david.kaplan@amd.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	"virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>,
	Robert Buhren <robert@sect.tu-berlin.de>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	brijesh.singh@amd.com, "Morbitzer,
	Mathias" <mathias.morbitzer@aisec.fraunhofer.de>,
	"hch@lst.de" <hch@lst.de>
Subject: Re: swiotlb/virtio: unchecked device dma address and length
Date: Thu, 17 Dec 2020 23:17:17 +0000	[thread overview]
Message-ID: <20201217231717.GB14861@ashkalra_ubuntu_server> (raw)
In-Reply-To: <AM7P194MB0900948E02C21FB13B722CD5D9C50@AM7P194MB0900.EURP194.PROD.OUTLOOK.COM>

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu