Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Integer Overflow / Heap Buffer Overflow on 32-bit systems - [Medium] Incomplete Fix / Missing __GFP_NOWARN for folios array -- commit 8833c30cdb25571807590bf3d21df9402d38121d Author: Vivek Kasireddy <[email protected]> udmabuf: Do not warn when pages/folios array allocation fails This commit adds the __GFP_NOWARN flag to the allocation of ubuf->pages and ubuf->pinned_folios in init_udmabuf(). The goal is to prevent warning traces when creating extremely large udmabuf arrays. > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 4a9ab5822ffc3..7c4339130eff7 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -190,11 +190,13 @@ static void unpin_all_folios(struct udmabuf *ubuf) > > static __always_inline int init_udmabuf(struct udmabuf *ubuf, pgoff_t pgcnt) > { [Severity: Critical] This is a pre-existing issue, but can pgcnt overflow on 32-bit systems? In udmabuf_create(), pgcnt and subpgcnt are pgoff_t (32-bit on 32-bit systems). When accumulating the total size from the user-provided list: udmabuf_create() { for (i = 0; i < head->count; i++) { pgoff_t subpgcnt = list[i].size >> PAGE_SHIFT; pgcnt += subpgcnt; if (pgcnt > pglimit) goto err_noinit; } ... ret = init_udmabuf(ubuf, pgcnt); } If a user passes many elements, pgcnt += subpgcnt can wrap around to a small number (e.g., 1), bypassing the pglimit check. This causes init_udmabuf() to allocate very small arrays for ubuf->pages and ubuf->pinned_folios. Later, in udmabuf_pin_folios(), a single list item's size is processed: udmabuf_pin_folios() { pgcnt = size >> PAGE_SHIFT; end = start + (pgcnt << PAGE_SHIFT) - 1; ... for (; subpgoff < fsize; subpgoff += PAGE_SIZE) { ubuf->pages[upgcnt] = folio_page(folios[cur_folio], ...); ++upgcnt; } } The truncated end value still results in pinning thousands of pages, which the loop blindly writes into the undersized ubuf->pages array, causing a heap buffer overflow. Could we add an overflow check when accumulating pgcnt in udmabuf_create()? > - ubuf->pages = kvmalloc_objs(*ubuf->pages, pgcnt); > + gfp_t gfp = GFP_KERNEL | __GFP_NOWARN; > + > + ubuf->pages = kvmalloc_objs(*ubuf->pages, pgcnt, gfp); [Severity: Medium] This isn't a bug introduced by this patch, but since we are adding __GFP_NOWARN to suppress warnings on massive user-requested allocations, should we also add it to the folios array allocation in udmabuf_create()? udmabuf_create() { ... folios = kvmalloc_array(max_nr_folios, sizeof(*folios), GFP_KERNEL); ... } A privileged user (with CAP_IPC_LOCK) can still request an extremely large size, which could cause this folios allocation to fail and trigger the same warning this patch intends to fix. > if (!ubuf->pages) > return -ENOMEM; > > - ubuf->pinned_folios = kvmalloc_objs(*ubuf->pinned_folios, pgcnt); > + ubuf->pinned_folios = kvmalloc_objs(*ubuf->pinned_folios, pgcnt, gfp); > if (!ubuf->pinned_folios) > return -ENOMEM; > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
