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 <[email protected]> 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/[email protected]?part=1
