Re: [PATCH v7 16/22] dma-direct: make dma_direct_map_phys() honor DMA_ATTR_CC_SHARED

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dma_map_resource() fails for devices using a dma_range_map that does not cover MMIO regions, as DMA_ATTR_MMIO mappings are now unconditionally subjected to phys_to_dma() translation.
- [High] Peer-to-Peer DMA (MMIO) breaks on AMD SME systems because the memory encryption mask (C-bit) is erroneously applied to MMIO addresses, potentially causing memory corruption.
--

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

dma-direct: make dma_direct_map_phys() honor DMA_ATTR_CC_SHARED

This commit modifies dma_direct_map_phys() to select the DMA address
encoding based on DMA_ATTR_CC_SHARED. It uses phys_to_dma_unencrypted() for
decrypted mappings and phys_to_dma_encrypted() otherwise, while preventing
the swiotlb force option for several architectures' secure-guest setups.

> diff --git a/kernel/dma/direct.h b/kernel/dma/direct.h
> index e05dc7649366a..f3fc28f352ba1 100644
> --- a/kernel/dma/direct.h
> +++ b/kernel/dma/direct.h
> @@ -88,37 +88,40 @@ static inline dma_addr_t dma_direct_map_phys(struct device *dev,
>  {
>  	dma_addr_t dma_addr;
>  
> +	/*
> +	 * For a device requiring unencrypted DMA, MMIO memory is treated
> +	 * as shared by default.
> +	 */
> +	if (force_dma_unencrypted(dev) && (attrs & DMA_ATTR_MMIO))
> +		attrs |= DMA_ATTR_CC_SHARED;
> +
>  	if (is_swiotlb_force_bounce(dev)) {
> -		if (!(attrs & DMA_ATTR_CC_SHARED)) {
> -			if (attrs & (DMA_ATTR_MMIO | DMA_ATTR_REQUIRE_COHERENT))
> -				return DMA_MAPPING_ERROR;
> +		if (attrs & (DMA_ATTR_MMIO | DMA_ATTR_REQUIRE_COHERENT))
> +			return DMA_MAPPING_ERROR;
>  
> -			return swiotlb_map(dev, phys, size, dir, attrs);
> -		}
> -	} else if (attrs & DMA_ATTR_CC_SHARED) {
> -		return DMA_MAPPING_ERROR;
>  	}
>  
> -	if (attrs & DMA_ATTR_MMIO) {
> -		dma_addr = phys;
> -		if (unlikely(!dma_capable(dev, dma_addr, size, false, attrs)))
> -			goto err_overflow;
> -	} else if (attrs & DMA_ATTR_CC_SHARED) {
> +	if (attrs & DMA_ATTR_CC_SHARED)
>  		dma_addr = phys_to_dma_unencrypted(dev, phys);
> +	else
> +		dma_addr = phys_to_dma_encrypted(dev, phys);
> +
> +	if (attrs & DMA_ATTR_MMIO) {

[Severity: High]
Will this break dma_map_resource() for devices that use a dma_range_map?

Previously, DMA_ATTR_MMIO addresses bypassed translation and directly used
dma_addr = phys. By routing MMIO addresses through phys_to_dma_encrypted()
or phys_to_dma_unencrypted(), they now pass through __phys_to_dma().

Since MMIO addresses do not reside in system RAM, they will be absent from
the device's dma_range_map. Doesn't this cause translate_phys_to_dma() to
return DMA_MAPPING_ERROR and fail the mapping?

[Severity: High]
On AMD SME systems, does this inadvertently apply the memory encryption
mask (C-bit) to MMIO addresses?

If a capable device performs Peer-to-Peer DMA, force_dma_unencrypted(dev)
returns false, so DMA_ATTR_CC_SHARED is not set. The MMIO address then
passes through phys_to_dma_encrypted(), which injects the C-bit via
__sme_set().

If the PCIe Root Complex receives an MMIO address with the C-bit set,
will it misroute the transaction to system RAM instead of the target PCI
BAR, potentially causing memory corruption?

>  		if (unlikely(!dma_capable(dev, dma_addr, size, false, attrs)))
>  			goto err_overflow;
> -	} else {
> -		dma_addr = phys_to_dma(dev, phys);
[ ... ]

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