Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] deinit_udmabuf() triggers UAF and mlock leak by improperly targeting the releasing task's ucounts instead of the creator's. - [High] Integer overflow in `pgcnt << PAGE_SHIFT` on 32-bit systems bypasses RLIMIT_MEMLOCK validation. - [High] Mlock quota leak when `udmabuf_pin_folios` successfully pins fewer pages than requested. Pre-existing issues: - [Critical] Integer overflow in `pgcnt += subpgcnt` on 32-bit systems causes severe Heap Buffer Overflow during pinning. -- commit 80f5d1e351e60ace6655d7afd00204458578dbbb Author: Vivek Kasireddy <[email protected]> udmabuf: Add mlock accounting for udmabuf pages/folios This commit introduces mlock accounting for udmabuf pages and folios to prevent unprivileged users from exhausting system memory. It tracks and limits pinned memory by checking the user's mlock limits via user_shm_lock(). > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 7c4339130eff7..e78947f01656d 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -203,9 +203,10 @@ static __always_inline int init_udmabuf(struct udmabuf > *ubuf, pgoff_t pgcnt) > return 0; > } > > -static __always_inline void deinit_udmabuf(struct udmabuf *ubuf) > +static __always_inline void deinit_udmabuf(struct udmabuf *ubuf, pgoff_t > pgcnt) > { > unpin_all_folios(ubuf); > + user_shm_unlock(pgcnt << PAGE_SHIFT, current_ucounts()); [Severity: High] Does this code safely handle unlocking the mlock quota when the dma_buf is released by a different task? The dma_buf release callback executes the deinit logic in an arbitrary task context. Passing current_ucounts() to user_shm_unlock() in this context drops a reference on an unrelated ucounts, which can lead to a use-after-free. This also permanently leaks the original creator's mlock quota, since the creator's ucounts is locked during creation but never saved in the ubuf structure for teardown. > kvfree(ubuf->pages); > } > > @@ -217,7 +218,7 @@ static void release_udmabuf(struct dma_buf *buf) > if (ubuf->sg) > put_sg_table(dev, ubuf->sg, ubuf->sg_dir); > > - deinit_udmabuf(ubuf); > + deinit_udmabuf(ubuf, ubuf->pagecount); [Severity: High] Does this successfully unlock the entire mlock quota if udmabuf_pin_folios() stops early and updates ubuf->pagecount? During creation, user_shm_lock() locks the requested pgcnt. If a memfd is smaller than requested, udmabuf_pin_folios() pins fewer pages and overwrites the requested count: ubuf->pagecount = upgcnt; When the buffer is later released, unlocking only the successfully pinned pages abandons the remaining locked quota, permanently leaking the unpinned mlock delta. > kfree(ubuf); > } > > @@ -385,6 +386,15 @@ static long udmabuf_create(struct miscdevice *device, > if (!pgcnt) > goto err_noinit; > [Severity: Critical] This is a pre-existing issue, but can the 32-bit pgcnt accumulation wrap around and bypass limits? In the loop calculating the total pages: subpgcnt = list[i].size >> PAGE_SHIFT; pgcnt += subpgcnt; If a user passes multiple items that cause pgcnt to wrap around to a small value, the check against pglimit passes. This small wrapped total is then passed to init_udmabuf(), which allocates tiny heap arrays based on the wrapped 32-bit total. Later, when udmabuf_pin_folios() executes, it loops using the non-truncated per-item size, writing millions of pointers out of bounds of the undersized allocation: ubuf->pages[upgcnt] = folio_page(folios[cur_folio], subpgoff >> PAGE_SHIFT); > + if (!user_shm_lock(pgcnt << PAGE_SHIFT, current_ucounts())) { [Severity: High] Does the bitwise shift on the 32-bit pgcnt value overflow on 32-bit systems? If a user requests an allocation of exactly 4GB, pgcnt becomes 0x100000. Shifting this by PAGE_SHIFT yields 0 on 32-bit architectures. This instantly succeeds the zero-byte lock check while successfully pinning the full 4GB of physical memory, bypassing the RLIMIT_MEMLOCK validation. > + pr_warn_once("%s: User not permitted to pin more than %lu KB " > + "of memory, either increase ulimits -l, or " > + "obtain CAP_IPC_LOCK capability\n", current->comm, > + rlimit(RLIMIT_MEMLOCK) >> 10); > + ret = -EPERM; > + goto err_noinit; > + } > + > ret = init_udmabuf(ubuf, pgcnt); > if (ret) > goto err; [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
