Re: [PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unprotected concurrent accesses to `ubuf->sg` allow memory and DMA mapping leaks, and expose a TOCTOU race condition that can trigger a kernel panic via an invalid pointer dereference.
- [High] Mismatch between the initial DMA mapping direction and the cache synchronization direction for subsequent CPU accesses violates the DMA API contract.
--

commit aa38d7b2c22ba17dc9edaf6a59e4f9a1d76a592b
Author: Robert Mader <robert.mader@collabora.com>

udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

The message of commit 504e2b4ab97a ("dma-buf/udmabuf: skip redundant cpu sync to
fix cacheline EEXIST warning") says:

> The CPU sync at map/unmap time is also redundant for udmabuf:
> begin_cpu_udmabuf() and end_cpu_udmabuf() already perform explicit
> cache synchronization via dma_sync_sgtable_for_cpu/device() when CPU
> access is requested through the dma-buf interface.

This, however, does not apply to the first time begin_cpu_udmabuf() is
called on an udmabuf, in which case the implementation previously relied on
get_sg_table() to perform the cache synchronisation.

Ensure to call dma_sync_sgtable_for_cpu() in that case as well.

> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index bced421c0d658..08f57bc1294df 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -224,21 +224,22 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
>  {
>  	struct udmabuf *ubuf = buf->priv;
>  	struct device *dev = ubuf->device->this_device;
> -	int ret = 0;
>  
>  	if (!ubuf->sg) {
>  		ubuf->sg = get_sg_table(dev, buf, direction);
>  		if (IS_ERR(ubuf->sg)) {
> +			int ret;
> +
>  			ret = PTR_ERR(ubuf->sg);
>  			ubuf->sg = NULL;
> +			return ret;
>  		} else {
>  			ubuf->sg_dir = direction;
>  		}
> -	} else {
> -		dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction);
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this lack of synchronization allow
concurrent threads to see a transient error pointer?

If multiple threads invoke the sync ioctl concurrently, they can race
on the !ubuf->sg check.  If the first thread fails to allocate the
sg_table, it temporarily assigns an error pointer to ubuf->sg before
setting it back to null:

Thread 1:
	if (!ubuf->sg) {
		ubuf->sg = get_sg_table(dev, buf, direction);

Thread 2 could observe ubuf->sg as an error pointer, bypass the null
check, and pass the error pointer to dma_sync_sgtable_for_cpu() further
down, triggering a kernel panic when dereferenced.

Alternatively, if both threads see !ubuf->sg as true, they could both
allocate sg_table structures, leaking one and its dma mapping.

>  
> -	return ret;
> +	dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction);

[Severity: High]
This is a pre-existing issue, but does this allow syncing with a different
direction than the original mapping?

If a user issues a sync ioctl with one direction to establish the
sg_table mapping, and then issues a subsequent sync with a different
direction, dma_sync_sgtable_for_cpu() and dma_sync_sgtable_for_device()
will be called using the new direction.

Since the dma mapping rules require sync and map directions to exactly
match unless the mapping is bidirectional, could this lead to incorrect
cache maintenance operations?

> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627105725.9083-1-robert.mader@collabora.com?part=1