Re: [RFC PATCH v2 14/25] iommu/dma: Support decrypted PISA pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] dma-direct: broken CoCo PISA SWIOTLB bypass in
  dma_direct_map_phys()
- [Medium] iommu/dma: broken CoCo coherent allocation in
  pisa_alloc_coherent()
- [Medium] iommu/dma: race on meta->flags in pisa_release_page() loses
  quarantine
- [Medium] iommu/dma: stale meta->on_list when pisa_shrink_scan() defers
  pages
- [Medium] iommu/dma: unprotected set_memory_encrypted() in
  pisa_alloc_split_page()
- [Low] iommu/dma: undocumented pisa.decrypt boot parameter in
  kernel-parameters.txt
--

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

iommu/dma: Support decrypted PISA pages

This commit adds support for decrypting 2MB PISA pages upfront via a
pisa.decrypt parameter and PISA_DECRYPTED flag so Confidential Computing
guests can map them without per-packet hypercalls or swiotlb bouncing.

> diff --git a/drivers/iommu/pisa-alloc.c b/drivers/iommu/pisa-alloc.c
> index 7b9d0a7acc101..b4a25c65bf97e 100644
> --- a/drivers/iommu/pisa-alloc.c
> +++ b/drivers/iommu/pisa-alloc.c
[ ... ]
> @@ -49,6 +51,10 @@ static LLIST_HEAD(pisa_free_list);
>  /* Dedicated WQ_MEM_RECLAIM workqueue, can run without allocations. */
>  static struct workqueue_struct *pisa_wq __ro_after_init;
>  
> +/* Mark active PISA pages decrypted for DMA (auto-enabled on CoCo guests). */
> +static bool pisa_decrypt __read_mostly;
> +module_param_named(decrypt, pisa_decrypt, bool, 0444);

[Severity: Low]
Since CONFIG_PISA is a bool Kconfig symbol, pisa.decrypt is a built-in
kernel command-line parameter and should be documented in
Documentation/admin-guide/kernel-parameters.txt.

[ ... ]

> @@ -128,9 +185,15 @@ void pisa_release_page(struct pisa_meta *meta)
>  
>  	/* Quarantined on unmap failure and leaked in pisa_release_page_rcu(). */
>  	pisa_unmap_all(meta);
> +	flags = meta->flags & ~PISA_ACTIVE;
>  
>  	/* Clear PISA_ACTIVE before the RCU grace period so new readers stop entering. */
> -	WRITE_ONCE(meta->flags, meta->flags & ~PISA_ACTIVE);
> +	WRITE_ONCE(meta->flags, flags);
> +	if ((flags & (PISA_DECRYPTED | PISA_QUARANTINED)) == PISA_DECRYPTED) {
> +		synchronize_rcu();
> +		pisa_page_unprepare(meta);
> +	}
>  
>  	call_rcu(&meta->rcu, pisa_release_page_rcu);
>  }

[Severity: Medium]
Can a concurrent pisa_domain_quiesce() race with pisa_release_page() on
meta->flags and lose PISA_QUARANTINED?

In pisa_unmap_domain_locked(), the domain bit in meta->domains_mapped is
cleared via test_and_clear_bit() before iommu_unmap_fast() runs:

CPU0                                  CPU1
pisa_domain_quiesce()                 pisa_release_page()
  rcu_read_lock()
  pisa_unmap_domain_locked()
    test_and_clear_bit(idx, ...)
                                        pisa_unmap_all() (sees bit 0)
                                        flags = meta->flags & ~PISA_ACTIVE
    iommu_unmap_fast() (fails)
    pisa_quarantine_meta()
      WRITE_ONCE(meta->flags,
                 ... | PISA_QUARANTINED)
                                        WRITE_ONCE(meta->flags, flags)
                                          <-- clobbers PISA_QUARANTINED
  rcu_read_unlock()
                                        synchronize_rcu()
                                        pisa_page_unprepare(meta)
                                        call_rcu(..., pisa_release_page_rcu)
                                          <-- frees slot and pages while
                                              IOMMU mapping is still live

Even if pisa_quarantine_meta() sets PISA_QUARANTINED after CPU1's
WRITE_ONCE(meta->flags, flags), CPU1 only checks the local pre-grace-period
flags variable and still calls pisa_page_unprepare(meta) after
synchronize_rcu(), re-encrypting a quarantined page that remains mapped in
the IOMMU.

[ ... ]

> @@ -773,6 +837,16 @@ static struct pisa_meta *pisa_alloc_split_page(unsigned int order, gfp_t gfp, in
>  	}
>  
>  	return meta;
> +
> +err_free:
> +	if (!can_block && (meta->flags & PISA_DECRYPTED)) {
> +		/* set_memory_encrypted() may sleep; defer to reclaim worker. */
> +		llist_add(&meta->llnode, &pisa_free_list);
> +		pisa_schedule_reclaim();
> +	} else {
> +		pisa_release_page(meta);
> +	}
> +	return ERR_PTR(ret);
>  }

[Severity: Medium]
When pisa_alloc_split_page() is called with GFP_NOFS or GFP_NOIO, can_block
is true, so can this err_free path call pisa_release_page() ->
pisa_page_unprepare() -> set_memory_encrypted() without memalloc_nofs/noio
protection?

Unlike __pisa_page_alloc(), which wraps pisa_page_prepare() in
pisa_memalloc_save(gfp), this unwind path runs outside any memalloc scope.
Because set_memory_encrypted() can perform internal GFP_KERNEL page-table or
hypercall buffer allocations, hitting -ENOSPC or -EBUSY here under GFP_NOFS
or GFP_NOIO can recurse into filesystem or block I/O reclaim and deadlock.
Deferring PISA_DECRYPTED pages to pisa_free_list whenever
(gfp & (__GFP_FS | __GFP_IO)) != (__GFP_FS | __GFP_IO) (or unconditionally)
would avoid this.

[ ... ]

> @@ -1135,7 +1209,7 @@ void *pisa_alloc_coherent(struct page **pagep, struct device *dev,
>  		if (!win || pisa_window_declined(win) || !(domain->pgsize_bitmap & PISA_SIZE))
>  			return NULL;
>  	} else if (is_swiotlb_for_alloc(dev) ||
> -		   force_dma_unencrypted(dev) ||
> +		   (force_dma_unencrypted(dev) && !pisa_want_decrypt()) ||
>  		   !dma_coherent_ok(dev, PFN_PHYS(max_pfn) - 1, 1)) {
>  		return NULL;
>  	}

[Severity: Medium]
Is this new force_dma_unencrypted(dev) && !pisa_want_decrypt() path
reachable when allocating coherent DMA memory on a CoCo guest?

Both dma_alloc_attrs() and dma_direct_alloc() set __DMA_ATTR_ALLOC_CC_SHARED
in attrs when force_dma_unencrypted(dev) is true, but pisa_alloc_coherent()
rejects any attrs other than DMA_ATTR_NO_WARN just above:

drivers/iommu/pisa-alloc.c:pisa_alloc_coherent() {
    ...
	if ((current_gfp_context(gfp) & ~(__GFP_ZERO | __GFP_NOWARN)) != GFP_KERNEL ||
	    (attrs & ~DMA_ATTR_NO_WARN))
		return NULL;
}

As a result, pisa_alloc_coherent() always returns NULL on CoCo guests before
reaching this check.

In addition, when pisa_alloc_coherent() succeeds in dma_direct_alloc() for a
decrypted PISA page, dma_direct_alloc() computes *dma_handle via
phys_to_dma_direct(dev, page_to_phys(page), false) with unencrypted
hardcoded to false, returning an encrypted DMA address for a page that was
decrypted via set_memory_decrypted().  And when pisa_alloc_coherent() scans
existing pages on pa->partial, pa->partial_dirty, and pa->idle, it does not
check that meta->flags has PISA_DECRYPTED set when force_dma_unencrypted(dev)
is true.

[ ... ]

> @@ -1294,10 +1368,15 @@ VISIBLE_IF_KUNIT unsigned long pisa_shrink_scan(struct shrinker *shrink, struct
>  		read_unlock(&pisa_allocs_lock);
>  	}
>  
> -	/* Retiring must not run under @pa->lock. */
> +	/* Unmap outside locks; defer decrypted pages to workqueue outside fs_reclaim. */
>  	list_for_each_entry_safe(meta, tmp, &release_list, list) {
>  		list_del_init(&meta->list);
> -		pisa_release_page(meta);
> +		if (meta->flags & PISA_DECRYPTED) {
> +			llist_add(&meta->llnode, &pisa_free_list);
> +			pisa_schedule_reclaim();
> +		} else {
> +			pisa_release_page(meta);
> +		}
>  	}

[Severity: Medium]
When pisa_shrink_scan() defers a PISA_DECRYPTED allocator page to
pisa_free_list, can a concurrent pisa_domain_quiesce() see a stale
meta->on_list and corrupt pisa_free_list?

Unlike pisa_place_meta() and pisa_reclaim_work_fn(), pisa_shrink_list()
detaches idle pages from pa->idle without setting meta->on_list to
PISA_LIST_NONE.  While the deferred page sits on pisa_free_list waiting for
pisa_reclaim_work_fn(), meta->pa is still non-NULL and meta->on_list is
still PISA_LIST_IDLE_CLEAN or PISA_LIST_IDLE_DIRTY:

CPU0                                  CPU1
pisa_shrink_scan()                    pisa_domain_quiesce()
  pisa_shrink_list()
    list_move(..., &release_list)
    pa->num_idle_pages--
    (meta->on_list left as IDLE)
  list_del_init(&meta->list)
  llist_add(&meta->llnode,
            &pisa_free_list)
                                        pisa_unmap_domain_locked()
                                          (unmap fails -> quarantined)
                                        spin_lock(&pa->lock)
                                        if (meta->on_list == IDLE_...)
                                          pa->num_idle_pages--
                                        pisa_place_meta(meta)
                                          llist_add(&meta->llnode,
                                                    &pisa_free_list)
                                            <-- duplicate llist_add

Resetting WRITE_ONCE(meta->on_list, PISA_LIST_NONE) inside
pisa_shrink_list() under lock would prevent both the pa->num_idle_pages
underflow and the double insertion onto pisa_free_list.

> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 11052b65e573b..a7fa1efc0ba09 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -707,8 +707,9 @@ dma_addr_t dma_direct_map_phys(struct device *dev, phys_addr_t phys,
>  	if (is_swiotlb_force_bounce(dev)) {
>  		if (attrs & (DMA_ATTR_MMIO | DMA_ATTR_REQUIRE_COHERENT))
>  			return DMA_MAPPING_ERROR;
> -
> -		return swiotlb_map(dev, phys, size, dir, attrs);
> +		if (!is_pisa_direct(phys, size))
> +			return swiotlb_map(dev, phys, size, dir, attrs);
> +		attrs |= DMA_ATTR_CC_SHARED;
>  	}

[Severity: High]
Does gating is_pisa_direct(phys, size) on is_swiotlb_force_bounce(dev)
prevent decrypted PISA pages from bypassing swiotlb on CoCo guests?

Since commit 30c5e45ee2c5 ("dma-direct: make dma_direct_map_phys() honor
DMA_ATTR_CC_SHARED"), CoCo guests do not set SWIOTLB_FORCE, so
is_swiotlb_force_bounce(dev) is false during normal CoCo operation.
Instead, CoCo guests enforce bouncing via force_dma_unencrypted(dev) inside
dma_capable():

include/linux/dma-direct.h:dma_capable() {
    ...
	if (!(attrs & DMA_ATTR_CC_SHARED) && force_dma_unencrypted(dev))
		return false;
    ...
}

Because is_pisa_direct(phys, size) is only checked inside
if (is_swiotlb_force_bounce(dev)), CoCo guests skip it, leave
DMA_ATTR_CC_SHARED unset, fail dma_capable(), and bounce every decrypted
PISA page through swiotlb_map().

Conversely, is_swiotlb_force_bounce(dev) is true when swiotlb=force is
passed on the command line or when a device is bound to a
restricted-dma-pool via rmem_swiotlb_device_init(); bypassing swiotlb_map()
there hands the device a general RAM address outside its restricted pool.
Checking force_dma_unencrypted(dev) && is_pisa_direct(phys, size) outside
the is_swiotlb_force_bounce(dev) block to set DMA_ATTR_CC_SHARED would fix
both cases.

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