Re: [RFC PATCH v2 08/25] iommu/dma: Use per-domain IOVA window to map PISA memory

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] iommu/dma: untrusted devices not excluded in __pisa_map_phys()
- [Medium] iommu/dma: pisa_domain_quiesce() races with PISA page
  retirement
--

commit 5ad49a194c307768ccdbdc801e7b8a723eed2bfc
Author: Luigi Rizzo <lrizzo@google.com>

iommu/dma: Use per-domain IOVA window to map PISA memory

This commit intercepts DMA map and unmap operations in dma-iommu to map
PISA pages using a dynamically assigned per-domain IOVA window and 2MB leaf
PTEs.

> diff --git a/drivers/iommu/pisa-map.c b/drivers/iommu/pisa-map.c
> index 2ba977bb68734..2c481e514575c 100644
> --- a/drivers/iommu/pisa-map.c
> +++ b/drivers/iommu/pisa-map.c
[ ... ]
> @@ -300,3 +303,184 @@ void pisa_domain_quiesce(struct iommu_domain *domain)
>  		pr_debug("pisa: unmapped %u PISA pages from idle domain %p (index %d)\n",
>  			 done, domain, idx);
>  }
[ ... ]
> +int pisa_window_assign(struct device *dev, struct iommu_domain *domain, struct pisa_window *win)
> +{
> +	u64 limit = min_not_zero(dma_get_mask(dev), dev->bus_dma_limit);
> +	u64 size = pisa_window_size;
> +	unsigned long flags;
> +	dma_addr_t base = 0;
> +	int idx, ret;
> +
> +	limit = min_not_zero(limit, dev->coherent_dma_mask);
> +
> +	if (!dev_is_dma_coherent(dev))
> +		return -EOPNOTSUPP;

[Severity: High]
Should pisa_window_assign() and __pisa_map_phys() also reject untrusted
devices when dev_is_untrusted(dev) is true?

In iommu_dma_map_phys(), SWIOTLB bounce buffering for untrusted PCI devices
is only used when the buffer is not aligned to the IOMMU granule, so
granule-aligned buffers reach __iommu_dma_map() -> pisa_map_phys()
directly:

drivers/iommu/dma-iommu.c:iommu_dma_map_phys() {
    ...
	if (!(attrs & DMA_ATTR_MMIO) && dev_use_swiotlb(dev, size, dir) &&
	    iova_unaligned(iovad, phys, size)) {
    ...
	iova = __iommu_dma_map(dev, phys, size, prot, dma_mask);
}

When an untrusted external PCI device maps a 4KB-aligned PISA buffer,
__pisa_map_phys() maps the entire 2MB PISA superpage into the device's
domain with meta->prot (including IOMMU_WRITE on bidirectional allocators
even for DMA_TO_DEVICE mappings), and iommu_dma_unmap_phys() and
__iommu_dma_unmap() leave the 2MB mapping active across unmap, exposing all
sibling blocks in the same 2MB page to the untrusted device.

[ ... ]

> +	/* First use of this PISA page in this domain: install the leaf PTE. */
> +	ret = 0;
> +	spin_lock_irqsave(&pisa_domains[domain_idx].map_lock, flags);
> +	if (!test_bit(domain_idx, meta->domains_mapped)) {
> +		phys_addr_t pisa_phys = phys & PISA_MASK;
> +
> +		ret = iommu_map(domain, pisa_phys_to_iova(win, meta, pisa_phys),
> +				pisa_phys, PISA_SIZE, block_prot, GFP_ATOMIC);
> +		if (likely(!ret)) {
> +			/* Ensure leaf PTE is visible before setting the domain bit. */
> +			smp_mb__before_atomic();
> +			set_bit(domain_idx, meta->domains_mapped);
> +		}
> +	}
> +	spin_unlock_irqrestore(&pisa_domains[domain_idx].map_lock, flags);

[Severity: Medium]
Once __pisa_map_phys() sets domain_idx in meta->domains_mapped, can
pisa_domain_quiesce() corrupt meta->rcu or pisa_free_list if an idle PISA
page is retired concurrently while an IOMMU unmap fails?

When an idle page is detached for retirement by pisa_reclaim_work_fn(),
pisa_place_meta(), or pisa_shrink_list(), meta->pa and pisa_slot2meta[slot]
remain set until pisa_release_page_rcu() runs after the RCU grace period.
If pisa_domain_quiesce() sees PISA_QUARANTINED set after an unmap failure,
it only checks meta->pa == pa under pa->lock before calling
pisa_place_meta(meta), which re-queues meta->llnode (unioned with
meta->rcu) onto pisa_free_list when all blocks are free:

CPU0 (pisa_reclaim_work_fn)       CPU1 (pisa_domain_quiesce)
spin_lock_irqsave(&pa->lock)
list_del_init(&meta->list)
meta->on_list = PISA_LIST_NONE
llist_add(&meta->llnode,
          &pisa_free_list)
spin_unlock_irqrestore(&pa->lock)
llist_del_all(&pisa_free_list)
pisa_release_page(meta)
  pisa_unmap_all(meta)
  call_rcu(&meta->rcu, ...)
                                  pisa_unmap_domain_locked(meta, ...)
                                    /* unmap fails, sets PISA_QUARANTINED */
                                  spin_lock_irqsave(&pa->lock)
                                  if (meta->pa == pa)
                                    pisa_place_meta(meta)
                                      llist_add(&meta->llnode,
                                                &pisa_free_list)
                                                <-- overwrites meta->rcu

Similarly, if pisa_shrink_list() moved meta onto pisa_shrink_scan()'s local
release_list (which decrements pa->num_idle_pages without clearing
meta->on_list), pisa_domain_quiesce() decrements pa->num_idle_pages a
second time and calls list_del_init(&meta->list) while pisa_shrink_scan()
iterates release_list without pa->lock.  In addition,
pisa_quarantine_meta() updates meta->flags with an unlocked
WRITE_ONCE(meta->flags, meta->flags | PISA_QUARANTINED) that can race with
WRITE_ONCE(meta->flags, meta->flags & ~PISA_ACTIVE) in pisa_release_page(),
losing PISA_QUARANTINED and allowing pisa_release_page_rcu() to free the
quarantined page to the buddy allocator.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109182533.3594984-1-lrizzo@google.com?part=8