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

[Ashish Kalra <ashish.kalra@amd.com>]

On Thu, Dec 17, 2020 at 12:19:16PM +0800, Jason Wang wrote:
> 
> 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().
> 

That seems to be correct as in the unmap path, swiotlb_bounce() is being
called with DMA_FROM_DEVICE flag, so there is no read involved during
dma_unmap().

Thanks,
Ashish

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