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

From: Jason Wang <jasowang@redhat.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: ashish kalra <ashish.kalra@amd.com>,
	Felicitas Hetzelt <file@sect.tu-berlin.de>,
	Martin Radev <martin.radev@aisec.fraunhofer.de>,
	david kaplan <david.kaplan@amd.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	virtualization@lists.linux-foundation.org,
	Robert Buhren <robert@sect.tu-berlin.de>,
	iommu@lists.linux-foundation.org,
	Mathias Morbitzer <mathias.morbitzer@aisec.fraunhofer.de>,
	hch@lst.de
Subject: Re: swiotlb/virtio: unchecked device dma address and length
Date: Thu, 17 Dec 2020 12:19:16 +0800	[thread overview]
Message-ID: <44650cf2-a56c-43e2-7041-5ea3c3f2a202@redhat.com> (raw)
In-Reply-To: <2CCB61BD-19F0-48A3-A8D2-0D22E51D3677@oracle.com>

On 2020/12/16 下午9:04, Konrad Rzeszutek Wilk wrote:
> On December 16, 2020 1:41:48 AM EST, Jason Wang <jasowang@redhat.com> wrote:
>>
>> ----- Original Message -----
>>>
>>> ----- Original Message -----
>>>> .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).
>>>> I think I am missing something obvious here. The attacker is the
>>>> hypervisor,
>>>> aka
>>>> the owner of the VirtIO device (ring0). The attacker is the one
>> that
>>>> provides the addr/len - having that readonly from a guest
>> perspective
>>>> does not change the fact that the hypervisor can modify the memory
>> range
>>>> by mapping it via a different virtual address in the hypervisor?
>> (aka
>>>> aliasing it).
>>> Right, but if we allow hypervisor to provide arbitrary addr/len, does
>>> it mean hypervisor can read encrypted content of encrypted memory of
>>> guest through swiotlb?
> Yes .
>>> Thanks
>> Actually not. I think you're right.
>
> Your sentence is very confusing.

Sorry for  being unclear. This is all a reply to your suggestion of 
adding checks in the swiotlb.

>
> 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.
>
> Think original PA having a mapping to a PA in the SWIOTLB pool.
>
>
> 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.

How can we read in this case? It looks to me we don't try to read during 
dma_unmap().

Thanks

>
>
>
>
>> Thanks
>>
>>>>> Otherwise, device can modify the desc[i].addr/desc[i].len at any
>> time to
>>>>> pretend a valid mapping.
>>>> With the swiotlb=force as long as addr/len are within the PA
>> boundaries
>>>> within the SWIOTLB pool this should be OK?
>>>>
>>>> After all that whole area is in cleartext and visible to the
>> attacker.
>>>>

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