Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Removing the default size limit exposes an integer overflow on 32-bit architectures during `pgcnt` accumulation, leading to a critical heap buffer overflow. - [High] Removing the default `size_limit_mb` allows an unprivileged user to force arbitrary unbound kernel memory allocations via `kvmalloc()`, leading to a global OOM. --
--- Patch [1]: [PATCH v1] dma-buf/udmabuf: Disable the size limit by default --- commit 99ebd05d75131f1777563136d3ba9224beb0dc68 Author: Robert Mader <[email protected]> dma-buf/udmabuf: Disable the size limit by default This commit changes the default size limit for udmabuf to 0, which disables the limit. It modifies the size check in udmabuf_create() to bypass validation when the limit is 0. > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 08f57bc1294df..3ebc1742f3ff7 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c [ ... ] > @@ -374,7 +374,7 @@ static long udmabuf_create(struct miscdevice *device, > > subpgcnt = list[i].size >> PAGE_SHIFT; > pgcnt += subpgcnt; > - if (pgcnt > pglimit) > + if (pglimit && pglimit < pgcnt) > goto err_noinit; [Severity: High] Does removing this default size limit allow unprivileged users to force arbitrary unbound kernel memory allocations and trigger a global OOM? With pglimit bypassed, a user could submit an ioctl(UDMABUF_CREATE) with a massive size. The unbound pgcnt is then passed to init_udmabuf(), which allocates metadata arrays via kvmalloc_objs() using GFP_KERNEL: drivers/dma-buf/udmabuf.c:init_udmabuf() { ... ubuf->pages = kvmalloc_objs(*ubuf->pages, pgcnt); ... } For huge requests, the vmalloc fallback executes with GFP_KERNEL and without __GFP_RETRY_MAYFAIL or __GFP_ACCOUNT. Could this force the page allocator to invoke the global OOM killer? [Severity: Critical] Does bypassing this size check also expose an integer overflow regression on 32-bit architectures? On 32-bit systems, pgcnt is a 32-bit pgoff_t. A user could submit an ioctl with multiple items whose subpgcnt values sum to exceed U32_MAX. This would wrap pgcnt around to a small positive value. init_udmabuf() would then allocate a tiny array for ubuf->pages: drivers/dma-buf/udmabuf.c:init_udmabuf() { ... ubuf->pages = kvmalloc_objs(*ubuf->pages, pgcnt); ... } However, udmabuf_pin_folios() processes each item individually based on its original size and writes sequentially into ubuf->pages. This could massively overflow the undersized allocation: drivers/dma-buf/udmabuf.c:udmabuf_pin_folios() { ... ubuf->pages[upgcnt] = folio_page(folios[cur_folio], subpgoff >> PAGE_SHIFT); ++upgcnt; ... } Could this lead to a heap buffer overflow? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
