Re: [RFC PATCH 1/1] x86/VMBus: DMA transfer with encrypted memory in Coco VM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] The fallback paths in the allocation and deallocation functions recursively call the high-level DMA API, causing infinite loops.
- [Critical] The custom DMA allocator ignores the device's coherent DMA mask, allocating memory above the hardware's addressing limits.
- [Critical] Streaming DMA mappings do not apply the mandatory memory encryption bit to physical addresses.
- [Critical] Bypass of architectural DMA cache synchronization in streaming mappings causes data corruption on non-coherent systems.
- [High] Globally overriding the `dma_ops` pointer breaks DMA for all non-VMBus devices on the system, and fails to restore the state on initialization errors.
- [High] The deallocation logic completely mismatches the allocation APIs, causing memory leaks, WARN_ONs, and skipped DMA unmapping.
- [Medium] The `dma_supported` map op callback incorrectly modifies device state instead of performing a read-only query.
- [Low] Referencing the global `dma_ops` pointer causes an undefined symbol linker error on ARM64.
- [Low] The `else` branch managing unencrypted memory mappings in `hyperv_dma_alloc_coherent` is unreachable.
--

--- Patch [1]: [RFC PATCH 1/1] x86/VMBus: DMA transfer with encrypted memory in Coco VM ---
commit 7e352d385400a7e2fcde56e56c2991270a9e565d
Author: Tianyu Lan <ltykernel@gmail.com>
Subject: x86/VMBus: DMA transfer with encrypted memory in Coco VM

This commit introduces Hyper-V DMA ops to support DMA transfers directly with
private/encrypted memory in CoCo VMs. It bypasses SWIOTLB bounce buffers for
certain PCI and Confidential VMBus devices.

> diff --git a/drivers/hv/hv_dma_ops.c b/drivers/hv/hv_dma_ops.c
> new file mode 100644
> index 0000000000000..ef53a42c0450b
> --- /dev/null
> +++ b/drivers/hv/hv_dma_ops.c
> @@ -0,0 +1,140 @@
[ ... ]
> +#include <asm/mshyperv.h>
> +#include "hyperv_vmbus.h"
> +#include "../../kernel/dma/direct.h"
> +
> +extern const struct dma_map_ops *dma_ops;

[Severity: Low]
Is this extern declaration safe on ARM64?

The CONFIG_HYPERV option supports ARM64, but the global dma_ops variable is
only exported on certain architectures like x86. On ARM64, this does not exist,
which could result in an undefined symbol linker error.

[ ... ]
> +static int hyperv_dma_map_sg(struct device *dev, struct scatterlist *sgl,
> +		int nelems, enum dma_data_direction dir,
> +		unsigned long attrs)
> +{
> +	struct scatterlist *sg;
> +	dma_addr_t dma_addr;
> +	int i;
> +
> +	if (hyperv_private_memory_dma(dev)) {
> +		for_each_sg(sgl, sg, nelems, i) {
> +			dma_addr = __phys_to_dma(dev, sg_phys(sg));
> +			sg_dma_address(sg) = dma_addr;

[Severity: Critical]
Does this mapping miss the mandatory memory encryption bit?

For private memory transfers in CoCo VMs, physical addresses must be mapped
with a specific encryption bit. Returning the unencrypted physical address
here could cause the hardware to access the memory as shared or decrypted.

[Severity: Critical]
Is it safe to skip cache synchronization here?

By returning early after translation without calling generic wrappers, and
lacking .sync_single_for_cpu and .sync_single_for_device callbacks in the
hyperv_dma_ops structure, this bypasses architectural DMA cache flushing. On
architectures that are not hardware cache-coherent, this could lead to data
corruption.

> +			sg_dma_len(sg) = sg->length;
> +		}
> +
> +		return nelems;
> +	} else {
> +		return dma_direct_map_sg(dev, sgl, nelems, dir, attrs);
> +	}
> +}
[ ... ]
> +static int hyperv_dma_supported(struct device *dev, u64 mask)
> +{
> +	dev->coherent_dma_mask = mask;

[Severity: Medium]
Should a dma_supported callback modify the device state?

The ops->dma_supported callback is strictly a read-only query evaluated by
functions like dma_supported(). Assigning dev->coherent_dma_mask here silently
alters the device state when the driver only intends to query supported
capabilities.

> +	return 1;
> +}
[ ... ]
> +/* allocate and map a coherent mapping */
> +static void *
> +hyperv_dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
> +		    gfp_t flag, unsigned long attrs)
> +{
> +	phys_addr_t phys;
> +	void *ret;
> +
> +	if (!hyperv_private_memory_dma(dev))
> +		return dma_alloc_coherent(dev, size, dma_handle, flag);

[Severity: Critical]
Can this fallback path cause an infinite recursion?

If hyperv_private_memory_dma() is false, this calls dma_alloc_coherent().
The core API looks up the device's DMA ops via get_dma_ops(), which now returns
hyperv_dma_ops due to the global override. This would recursively call back into
hyperv_dma_alloc_coherent(), leading to a stack overflow.

> +
> +	size = ALIGN(size, PAGE_SIZE);
> +	ret = (void *)__get_free_pages(flag, get_order(size));

[Severity: Critical]
Could this allocation bypass the device's DMA boundary constraints?

The core DMA API strips memory placement zone flags (like __GFP_DMA32) before
calling ops->alloc, expecting the implementation to allocate from the correct
zone based on dev->coherent_dma_mask. Blindly allocating via __get_free_pages()
here could result in allocating memory above the hardware's addressing limits
for devices with constrained DMA masks.

> +	if (!ret)
> +		return ret;
> +	phys = virt_to_phys(ret);
> +
> +	if (hyperv_private_memory_dma(dev))
> +		*dma_handle = dma_addr_encrypted(__phys_to_dma(dev, phys));
> +	else
> +		*dma_handle = phys_to_dma_unencrypted(dev, phys);

[Severity: Low]
Is this else branch reachable?

The function returns early at the top if hyperv_private_memory_dma(dev) is
false. By the time execution reaches this block, the condition is guaranteed
to be true, making this else branch dead code.

> +
> +	memset(ret, 0, size);
> +	return ret;
> +}
> +
> +/* free a coherent mapping */
> +static void
> +hyperv_dma_free_coherent(struct device *dev, size_t size, void *vaddr,
> +		   dma_addr_t dma_addr, unsigned long attrs)
> +{
> +	if (hyperv_private_memory_dma(dev))
> +		dmam_free_coherent(dev, size, vaddr, dma_addr);

[Severity: Critical]
Can the dmam_free_coherent() call result in infinite recursion?

Since the memory was not allocated via devres, dmam_free_coherent() will fall
back to dma_free_coherent(). This looks up ops->free and infinitely recurses
back into hyperv_dma_free_coherent().

> +	else
> +		free_pages((unsigned long)vaddr, get_order(size));

[Severity: High]
Are the allocation and deallocation APIs correctly paired here?

The branches appear to use inverted APIs. For private memory, it is allocated
via __get_free_pages() but freed using the devres-managed dmam_free_coherent(),
which could fail to find a devres entry.

For non-private memory, it is allocated via dma_alloc_coherent() but
incorrectly freed using free_pages(). Bypassing the DMA API on free skips
SWIOTLB bounce buffer cleanup and IOMMU unmapping, potentially causing resource
leaks.

> +}
> +
> +static dma_addr_t hyperv_dma_map_phys(struct device *dev, phys_addr_t phys,
> +		size_t size, enum dma_data_direction dir,
> +		unsigned long attrs)
> +{
> +	if (hyperv_private_memory_dma(dev))
> +		return __phys_to_dma(dev, phys);

[Severity: Critical]
Does this mapping miss the memory encryption bit?

Similar to hyperv_dma_map_sg, returning the unencrypted physical address for
private memory transfers could cause the hardware to access the memory as shared
or decrypted.

[Severity: Critical]
Is it safe to bypass generic cache synchronization wrappers here?

As with hyperv_dma_map_sg, missing the cache flushing could cause data
corruption on non-coherent architectures.

> +	else
> +		return dma_direct_map_phys(dev, phys, size, dir, attrs, true);
> +}
[ ... ]
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 6824bd7cb3c4f..3479528d3b963 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -40,6 +40,10 @@
>  #include <clocksource/hyperv_timer.h>
>  #include <asm/mshyperv.h>
>  #include "hyperv_vmbus.h"
> +#include "../../kernel/dma/direct.h"
> +
> +extern const struct dma_map_ops *dma_ops;

[Severity: Low]
Would this extern declaration cause build issues on ARM64?

The global dma_ops pointer does not exist on ARM64, which may result in a linker
error on architectures that do not export it.

[ ... ]
> @@ -1518,8 +1527,11 @@ static int vmbus_bus_init(void)
>  	 * doing that on each VP while initializing SynIC's wastes time.
>  	 */
>  	is_confidential = ms_hyperv.confidential_vmbus_available;
> -	if (is_confidential)
> +	if (is_confidential) {
> +		dma_ops = &hyperv_dma_ops;

[Severity: High]
Could overriding the global dma_ops pointer affect other devices?

Setting the system-wide dma_ops pointer forces all standard non-VMBus devices
to use these Hyper-V DMA ops. Additionally, if vmbus_alloc_synic_and_connect()
fails later, this global pointer is left hijacked on the error path, which could
lead to system failures for non-VMBus devices.

>  		pr_info("Establishing connection to the confidential VMBus\n");
> +	}
> +
>  	hv_para_set_sint_proxy(!is_confidential);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803033520.12930-1-tiala@microsoft.com?part=1