Re: [PATCH v7 15/22] dma-direct: pass attrs to dma_capable() for DMA_ATTR_CC_SHARED checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] xen_swiotlb_map_phys() unconditionally rejects valid MMIO DMA mappings for devices requiring unencrypted DMA, breaking peer-to-peer DMA.
- [High] xen_swiotlb_map_phys() fails to strip the encryption bit from bounce buffer DMA addresses.

Pre-existing issues:
- [Critical] gart_alloc_coherent() and gart_free_coherent() pass an invalid dma_addr to dma_direct_free(), causing severe page allocator corruption and physical memory leaks.
--

commit d278e823d2c8033c6a6105a3f51b2763931c565f
Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

dma-direct: pass attrs to dma_capable() for DMA_ATTR_CC_SHARED checks
    
Teach dma_capable() about DMA_ATTR_CC_SHARED so the capability
check can reject encrypted DMA addresses for devices that require
unencrypted/shared DMA.

Also propagate DMA_ATTR_CC_SHARED in swiotlb_map() when the selected
SWIOTLB pool is decrypted so the capability check sees the correct DMA
address attribute.

Tested-by: Jiri Pirko <jiri@nvidia.com>
Tested-by: Michael Kelley <mhklinux@outlook.com>
Tested-by: Mostafa Saleh <smostafa@google.com>
Reviewed-by: Petr Tesarik <ptesarik@suse.com>
Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

> diff --git a/arch/x86/kernel/amd_gart_64.c b/arch/x86/kernel/amd_gart_64.c
> index e8000a56732e4..b5f1f031d45b8 100644
> --- a/arch/x86/kernel/amd_gart_64.c
> +++ b/arch/x86/kernel/amd_gart_64.c
[ ... ]
> @@ -473,7 +474,8 @@ gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
>  		return vaddr;
>  
>  	*dma_addr = dma_map_area(dev, virt_to_phys(vaddr), size,
> -			DMA_BIDIRECTIONAL, (1UL << get_order(size)) - 1);
> +				 DMA_BIDIRECTIONAL,
> +				 (1UL << get_order(size)) - 1, attrs);
>  	flush_gart();
>  	if (unlikely(*dma_addr == DMA_MAPPING_ERROR))
>  		goto out_free;

[Severity: Critical]
This is a pre-existing issue, but does the error path in gart_alloc_coherent()
and gart_free_coherent() pass an invalid dma_addr to dma_direct_free()?

If dma_map_area() fails, *dma_addr becomes DMA_MAPPING_ERROR, and the error
path at out_free passes it to dma_direct_free(). On success,
gart_free_coherent() later passes the GART-mapped dma_addr to
dma_direct_free().

Because dma_direct_free() uses the provided dma_addr to locate the backing
struct page (when dma_free_from_pool() is false), passing a GART aperture
address or DMA_MAPPING_ERROR instead of the physical DMA address causes it
to operate on bogus page structures, leading to severe memory corruption and
physical page leaks.

> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 8c4abe65cd49f..e2538824ef52e 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -212,7 +212,7 @@ static dma_addr_t xen_swiotlb_map_phys(struct device *dev, phys_addr_t phys,
>  	BUG_ON(dir == DMA_NONE);
>  
>  	if (attrs & DMA_ATTR_MMIO) {
> -		if (unlikely(!dma_capable(dev, phys, size, false))) {
> +		if (unlikely(!dma_capable(dev, phys, size, false, attrs))) {

[Severity: High]
Does this unconditionally reject valid MMIO DMA mappings for devices requiring
unencrypted DMA, breaking peer-to-peer DMA?

When mapping MMIO memory, attrs typically lacks DMA_ATTR_CC_SHARED. For CoCo
guests under Xen, force_dma_unencrypted(dev) is true, so dma_capable() will
evaluate !(attrs & DMA_ATTR_CC_SHARED) && force_dma_unencrypted(dev) as true,
thus rejecting the mapping.

While dma_direct_map_phys() was fixed elsewhere in this series to append
DMA_ATTR_CC_SHARED for MMIO, it appears xen_swiotlb_map_phys() was missed.

>  			dev_err_once(
>  				dev,
>  				"DMA addr %pa+%zu overflow (mask %llx, bus limit %llx).\n",
[ ... ]
> @@ -253,7 +253,7 @@ static dma_addr_t xen_swiotlb_map_phys(struct device *dev, phys_addr_t phys,
>  	/*
>  	 * Ensure that the address returned is DMA'ble
>  	 */
> -	if (unlikely(!dma_capable(dev, dev_addr, size, true))) {
> +	if (unlikely(!dma_capable(dev, dev_addr, size, true, attrs))) {

[Severity: High]
Does this fail to strip the encryption bit from bounce buffer DMA addresses?

When a DMA mapping requires a bounce buffer, swiotlb_tbl_map_single() allocates
an unencrypted buffer and injects DMA_ATTR_CC_SHARED into attrs. However,
dev_addr was set via xen_phys_to_dma(dev, map) right above this check, which
relies on phys_to_dma() and unconditionally applies the physical encryption bit
to the DMA address.

Because attrs now contains DMA_ATTR_CC_SHARED, dma_capable() will approve the
address. As a result, the device erroneously receives an encrypted DMA address
for decrypted memory.

>  		__swiotlb_tbl_unmap_single(dev, map, size, dir,
>  				attrs | DMA_ATTR_SKIP_CPU_SYNC,
>  				swiotlb_find_pool(dev, map));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701054926.825925-1-aneesh.kumar@kernel.org?part=15