On 6/25/26 08:10, Bryam Vargas via B4 Relay wrote:
From: Bryam Vargas hexlabsecurity@proton.me
begin_cpu_udmabuf() builds and caches ubuf->sg with an unserialised check-then-set, and end_cpu_udmabuf() reads the same field unlocked. The core invokes both cpu-access hooks without holding the reservation lock and DMA_BUF_IOCTL_SYNC is unlocked, so concurrent SYNC ioctls on a shared udmabuf fd race on ubuf->sg: two begins can both observe NULL and both call get_sg_table(), and the later store orphans the earlier table and its DMA mapping, which release_udmabuf() never frees. Each won race permanently leaks an sg_table and an unbalanced DMA mapping.
Serialize both hooks under the buffer's reservation lock, as panfrost and panthor do. dma_buf_begin/end_cpu_access() already annotate might_lock() on that lock, so taking it here matches the documented contract. Single-threaded callers are unaffected.
Fixes: 284562e1f348 ("udmabuf: implement begin_cpu_access/end_cpu_access hooks") Cc: stable@vger.kernel.org Signed-off-by: Bryam Vargas hexlabsecurity@proton.me
Same leak-with-dangling-pointer class as CVE-2024-56712 (export_udmabuf() error path) -- a distinct site the 2024 fix does not cover.
udmabuf is the only exporter that lazily builds its sg_table cache inside the cpu-access hook without serialising the check-then-set. The exporters that do comparable in-hook cache work all take a lock first: panfrost and panthor dma_resv_lock() (both hooks), omapdrm omap_obj->lock around its lazy page-get, the dma-heaps buffer->lock, and the TTM/GEM exporters (amdgpu, i915, xe) their object's reservation lock. tegra and videobuf2 take no lock here because they only sync an sg_table built earlier, so there is nothing to serialise.
Confirmed with an out-of-tree A/B exercising the begin/begin race: this driver built as a module with get_sg_table()/put_sg_table() counting allocations against frees, driven by a userspace racer that creates 3000 udmabufs and fires DMA_BUF_IOCTL_SYNC(SYNC_START) from N threads on each shared fd.
arm leaked sg_tables (of 3000 buffers) vulnerable, 4 threads 4761 control, 1 thread 0 patched (resv lock), 4 threads 0
One sg_table and its DMA mapping leak per won race; the single-thread control does not leak, isolating the race; with the lock the lazy-init runs once per buffer (3000 allocations, zero leaked). end_cpu_udmabuf() is locked for the same field too: an unlocked end could otherwise observe the transient IS_ERR store begin makes before resetting ubuf->sg to NULL, and dereference it. In a tighter 5000-iteration loop the unpatched leak runs around 15-20 MB/s of slab.
drivers/dma-buf/udmabuf.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index bced421c0d65..702ae13b97d1 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -226,6 +226,8 @@ static int begin_cpu_udmabuf(struct dma_buf *buf, struct device *dev = ubuf->device->this_device; int ret = 0;
dma_resv_lock(buf->resv, NULL);
Good catch, but we eventually wait for HW to finish while holding this lock.
So if possible lock it interruptible here.
Apart from that looks good to me, Christian.
if (!ubuf->sg) { ubuf->sg = get_sg_table(dev, buf, direction); if (IS_ERR(ubuf->sg)) {@@ -238,6 +240,8 @@ static int begin_cpu_udmabuf(struct dma_buf *buf, dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction); }
dma_resv_unlock(buf->resv);return ret;}
@@ -246,12 +250,18 @@ static int end_cpu_udmabuf(struct dma_buf *buf, { struct udmabuf *ubuf = buf->priv; struct device *dev = ubuf->device->this_device;
int ret = 0;dma_resv_lock(buf->resv, NULL); if (!ubuf->sg)
return -EINVAL;
ret = -EINVAL;elsedma_sync_sgtable_for_device(dev, ubuf->sg, direction);
dma_sync_sgtable_for_device(dev, ubuf->sg, direction);return 0;
dma_resv_unlock(buf->resv);return ret;}
static const struct dma_buf_ops udmabuf_ops = {
base-commit: 7eed1fb17959e721031555e5b5654083fe6a7d02 change-id: 20260625-b4-disp-67d1f3db-0082918fdcb5
Best regards,
Bryam Vargas hexlabsecurity@proton.me