Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Integer overflow in `udmabuf_create()` leads to undersized array allocation and a subsequent heap buffer overflow when pinning folios on 32-bit systems. - [High] Unprotected initialization of `ubuf->sg` in `begin_cpu_udmabuf` leads to memory leaks and data races. -- commit a036fc929a16b1c94a5ac12cbef23efece456576 Author: Samuel Moelius <[email protected]> dma-buf: udmabuf: validate create-list count before copying UDMABUF_CREATE_LIST copies a variable-length list using a byte count derived from head.count. The list_limit module parameter is signed and writable, so setting it negative lets a large unsigned count bypass the limit check. The u32 byte-count calculation can then wrap, causing only a small list to be copied while udmabuf_create() still iterates over the large count. Reject negative list_limit values and use checked size_t multiplication before copying the list. > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index bced421c0d658..264718676d485 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c [ ... ] > @@ -469,13 +470,14 @@ static long udmabuf_ioctl_create_list(struct file > *filp, unsigned long arg) > struct udmabuf_create_list head; > struct udmabuf_create_item *list; > int ret = -EINVAL; > - u32 lsize; > + size_t lsize; > > if (copy_from_user(&head, (void __user *)arg, sizeof(head))) > return -EFAULT; > - if (head.count > list_limit) > + if (list_limit < 0 || head.count > list_limit) > + return -EINVAL; > + if (check_mul_overflow(sizeof(struct udmabuf_create_item), head.count, > &lsize)) > return -EINVAL; > - lsize = sizeof(struct udmabuf_create_item) * head.count; > list = memdup_user((void __user *)(arg + sizeof(head)), lsize); > if (IS_ERR(list)) > return PTR_ERR(list); [Severity: Critical] This is a pre-existing issue, but can the 32-bit addition of subpgcnt wrap pgcnt? In udmabuf_create(), list[i].size is a user-controlled __u64 value. On 32-bit architectures, pgoff_t is a 32-bit unsigned integer: drivers/dma-buf/udmabuf.c:udmabuf_create() { ... subpgcnt = list[i].size >> PAGE_SHIFT; pgcnt += subpgcnt; if (pgcnt > pglimit) goto err_noinit; ... } If user space provides two items—one small (e.g., 2 pages) and one huge (e.g., size = 0xffffffff000ULL)—subpgcnt truncates to 32 bits and pgcnt can wrap around to a small positive value. This bypasses the pgcnt > pglimit check. init_udmabuf() then allocates arrays sized by the wrapped pgcnt: drivers/dma-buf/udmabuf.c:init_udmabuf() { ... ubuf->pages = kvmalloc_objs(*ubuf->pages, pgcnt); ... } Later, udmabuf_pin_folios() calculates a local 32-bit pgcnt = 0xffffffff and asks memfd_pin_folios() to pin up to that many folios. If memfd_pin_folios() returns a large number of folios, udmabuf_pin_folios() unconditionally writes these kernel pointers into the undersized arrays, resulting in an out-of-bounds write: drivers/dma-buf/udmabuf.c:udmabuf_pin_folios() { ... ubuf->pinned_folios[nr_pinned++] = folios[cur_folio]; ... ubuf->pages[upgcnt] = folio_page(folios[cur_folio], subpgoff >> PAGE_SHIFT); ... } Could we check for overflow when accumulating pgcnt? [Severity: High] This isn't a bug introduced by this patch, but does begin_cpu_udmabuf() have a race condition when allocating ubuf->sg? drivers/dma-buf/udmabuf.c:begin_cpu_udmabuf() { if (!ubuf->sg) { ubuf->sg = get_sg_table(dev, buf, direction); if (IS_ERR(ubuf->sg)) { ret = PTR_ERR(ubuf->sg); ubuf->sg = NULL; } else { ubuf->sg_dir = direction; } } else { ... } The DMA_BUF_IOCTL_SYNC ioctl triggers dma_buf_begin_cpu_access(), which calls the exporter's .begin_cpu_access hook (begin_cpu_udmabuf()) with the dma-buf reservation lock intentionally unlocked (as documented in dma-buf.h). If two user-space threads invoke DMA_BUF_IOCTL_SYNC concurrently on the same file descriptor, both can observe !ubuf->sg, allocate separate SG tables, and establish duplicate DMA mappings. The second thread will overwrite the ubuf->sg pointer, leaking the first allocated SG table and its DMA resources. Concurrent readers could also operate on partially written pointers. Is there a lock we can hold while checking and assigning ubuf->sg? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
