Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Critical] Deterministic NULL pointer dereference on 32-bit architectures due to mismatch between scatterlist allocation and iteration count. Pre-existing issues: - [High] Piecewise unmapping of chunked DMA regions violates DMA API rules. - [High] Error handling path shadows the loop counter and unmaps uninitialized scatterlist entries. -- commit 352582e0a7698d4a99b7c620ded07fa0a65602c3 Author: David Hu <[email protected]> dma-buf: Split sgl by largest page-aligned chunk This commit changes fill_sg_entry() to split scatterlists by ALIGN_DOWN(UINT_MAX, PAGE_SIZE) rather than UINT_MAX to ensure entries remain page-aligned. Page-aligned entries prevent failures in hardware DMA engines that require explicit address alignment and avoid edge cases in ib_umem_find_best_pgsz(). Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/dma-buf/dma-buf-mapping.c > b/drivers/dma-buf/dma-buf-mapping.c > index 80f6ab2f4809d..833be519e1e6a 100644 > --- a/drivers/dma-buf/dma-buf-mapping.c > +++ b/drivers/dma-buf/dma-buf-mapping.c > @@ -6,16 +6,17 @@ > #include <linux/dma-buf-mapping.h> > #include <linux/dma-resv.h> > #include <linux/overflow.h> > +#include <linux/align.h> > + > +#define MAX_SG_ENT_SZ ALIGN_DOWN(UINT_MAX, PAGE_SIZE) > > static struct scatterlist *fill_sg_entry(struct scatterlist *sgl, size_t > length, > dma_addr_t addr) > { > - unsigned int len, nents; > - unsigned int i; > + size_t len; > > - nents = DIV_ROUND_UP(length, UINT_MAX); > - for (i = 0; i < nents; i++) { > - len = min_t(size_t, length, UINT_MAX); > + while (length) { > + len = min(length, MAX_SG_ENT_SZ); [Severity: High] This is a pre-existing issue, but does chunking the mapped physical region into smaller pieces here cause a violation of the DMA API during teardown? During dma_buf_free_sgt() or the err_unmap_dma path, the code iterates over this scatterlist and calls dma_unmap_phys() on each split chunk individually: drivers/dma-buf/dma-buf-mapping.c:dma_buf_free_sgt() { ... for_each_sgtable_dma_sg(sgt, sgl, i) dma_unmap_phys(attach->dev, sg_dma_address(sgl), sg_dma_len(sgl), dir, DMA_ATTR_MMIO); ... } The DMA API mandates that unmapping must use the exact identical start address and length that was originally passed to dma_map_phys(). > length -= len; > /* > * DMABUF abuses scatterlist to create a scatterlist > @@ -25,8 +26,10 @@ static struct scatterlist *fill_sg_entry(struct > scatterlist *sgl, size_t length, > * does not require the CPU list for mapping or unmapping. > */ > sg_set_page(sgl, NULL, 0, 0); [Severity: Critical] Since calc_sg_nents() calculates too few entries for lengths > 4096 on 32-bit architectures due to integer wrapping, does this sg_set_page() unconditionally dereference the NULL pointer returned by sg_next() when the undersized scatterlist allocation is exhausted? > - sg_dma_address(sgl) = addr + (dma_addr_t)i * UINT_MAX; > + sg_dma_address(sgl) = addr; > sg_dma_len(sgl) = len; > + addr += len; > + /* Unconditionally advance. On last segment, this becomes NULL > */ > sgl = sg_next(sgl); > } > > @@ -42,7 +45,7 @@ static unsigned int calc_sg_nents(struct dma_iova_state > *state, > > if (!state || !dma_use_iova(state)) { > for (i = 0; i < nr_ranges; i++) { > - unsigned int added = DIV_ROUND_UP(phys_vec[i].len, > UINT_MAX); > + unsigned int added = DIV_ROUND_UP(phys_vec[i].len, > MAX_SG_ENT_SZ); [Severity: Critical] Does this calculation severely under-calculate or return 0 for the number of required SG entries on 32-bit architectures? On 32-bit architectures, MAX_SG_ENT_SZ expands to 0xFFFFF000. For lengths greater than 4096, DIV_ROUND_UP() expands to (len + 0xFFFFF000 - 1) / 0xFFFFF000, which will wrap around the 32-bit addition and yield an artificially low or zero number of entries. [Severity: High] This is a pre-existing issue, but does the error handling path following this allocation unmap uninitialized scatterlist entries if dma_map_phys() fails? When dma_map_phys() fails, the err_unmap_dma path in dma_buf_phys_vec_to_sgt() attempts cleanup: drivers/dma-buf/dma-buf-mapping.c:dma_buf_phys_vec_to_sgt() { ... err_unmap_dma: ... } else { for_each_sgtable_dma_sg(&dma->sgt, sgl, i) dma_unmap_phys(attach->dev, sg_dma_address(sgl), sg_dma_len(sgl), dir, DMA_ATTR_MMIO); } ... } The for_each_sgtable_dma_sg macro internally shadows the outer failure index i and initializes its own counter to 0. It then unconditionally iterates through all sgt->nents allocated entries. Because the failure occurred early, trailing entries remain zero-initialized, causing dma_unmap_phys() to be invoked on addresses of 0. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
