RE: [PATCH v4 5/5] iommu/dma: Force swiotlb_max_mapping_size on an untrusted device

[Michael Kelley]

From: Nicolin Chen <nicolinc@xxxxxxxxxx> Sent: Friday, February 23, 2024 11:58 AM
> 
> On Wed, Feb 21, 2024 at 11:39:29PM +0000, Michael Kelley wrote:
> > From: Will Deacon <will@xxxxxxxxxx> Sent: Wednesday, February 21, 2024
> 3:35 AM
> > > +static size_t iommu_dma_max_mapping_size(struct device *dev)
> > > +{
> > > +     if (is_swiotlb_active(dev) && dev_is_untrusted(dev))
> > > +             return swiotlb_max_mapping_size(dev);
> > > +     return SIZE_MAX;
> > > +}
> > > +
> >
> > In this [1] email, Nicolin had a version of this function that incorporated
> > the IOMMU granule.  For granules bigger than 4K, I think that's needed
> > so that when IOMMU code sets the swiotlb alloc_align_mask to the
> > IOMMU granule - 1, the larger offset plus the size won't exceed the
> > max number of slots.  swiotlb_max_mapping_size() by itself may
> > return a value that's too big when alloc_align_mask is used.
> >
> > Michael
> >
> > [1] https://lore.kernel.org/linux-iommu/SN6PR02MB415727E61B5295C259CCB268D4512@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/T/#m14dd359c5a4dd13e6cb0c35cf94f8d746257ae48
> 
> Yea, I just confirmed that with 64KB PAGE_SIZE the alloc_size
> can be over swiotlb_max_mapping_size, i.e. 256KB > 252KB. Yet,
> the max size corresponding to the max number of slots should
> be 256KB. So, I feel that this is marginally safe?

Yes, in the specific case you tested.  But I don't think the general
case is safe.  In your specific case, the "size" argument to
iommu_dma_map_page() is presumably 252 Kbytes because that's
what your new iommu_dma_max_mapping_size() returns. 
iommu_dma_map_page() calls iova_align() to round up the 252K
to 256K.  Also in your specific case, the "offset" argument to 
iommu_dma_map_page() is 0, so the "phys" variable (which embeds
the offset) passed to swiotlb_tbl_map_single() is aligned on a
64 Kbyte page boundary.   swiotlb_tbl_map_single() then
computes an offset in the orig_addr (i.e., "phys") based on the
DMA min_align_mask (4 Kbytes), and that value is 0 in your specific
case.  swiotlb_tbl_map_single() then calls swiotlb_find_slots()
specifying a size that is 256K bytes plus an offset of 0, so everything
works.

But what if the original offset passed to iommu_dma_map_page()
is 3 Kbytes (for example)?   swiotlb_tbl_map_single() will have an
orig_addr that is offset from a page boundary by 3 Kbytes.  The
value passed to swiotlb_find_slots() will be 256 Kbytes plus an
offset of 3 Kbytes, which is too big.

> 
> With that being said, there seems to be a 4KB size waste, due
> to aligning with the iommu_domain granule, in this particular
> alloc_size=256KB case?
> 
> On the other hand, if swiotlb_max_mapping_size was subtracted
> by 64KB (granule=64KB), i.e. alloc_size=192KB, which seems to
> generate more swiotlb fragments?

dma_max_mapping_size() returns a fixed value for a particular
device, so the fixed value must be conversative enough to handle
dma_map_page() calls with a non-zero offset (or the similar
dma_map_sg() where the scatter/gather list has non-zero
offsets).  So yes, the higher layers must limit I/O size, which
can produce more separate I/Os and swiotlb fragments to
fulfill an original request that is 256 Kbytes or larger.  The
same thing happens with 4K page size in that a 256K I/O
gets broken into a 252K I/O and a 4K I/O if the swiotlb is
being used.

I'm trying to think through if there's a way to make things
work with iommu_max_mapping_size() being less
conversative than subtracting the full granule size.  I'm
doubtful that there is.

Michael