Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [Critical] Incorrect offset parameter passed to dma_iova_link() causes all physical vectors to overwrite each other at the same IOVA address. - [Critical] Partial unmapping of DMA mappings via segmented scatterlist entries violates the DMA API. - [High] DIV_ROUND_UP macro wraps around on 32-bit platforms, causing calc_sg_nents to evaluate to 0 and failing the mapping. - [High] Setting sgt.orig_nents = 0 breaks sg_free_table, causing a silent memory leak of the scatterlist. - [High] Error path unmaps all uninitialized scatterlist entries due to an incorrect loop iterator macro shadowing the index. - [High] IOVA space allocated by dma_iova_try_alloc() is leaked on multiple error paths. -- commit 3ea75e22b2b7ea7287fcbeb0fa39a0b99a722743 Author: David Hu <[email protected]> dma-buf: Fix silent overflow for phys vec to sgt This commit changes mapped_len to a 64-bit size_t to prevent silent wrap-around during length accumulation for MMIO greater than 4G. It also introduces check_add_overflow() in calc_sg_nents() for safer scatterlist entry accumulation. > diff --git a/drivers/dma-buf/dma-buf-mapping.c > b/drivers/dma-buf/dma-buf-mapping.c > index 794acff2546a3..80f6ab2f4809d 100644 > --- a/drivers/dma-buf/dma-buf-mapping.c > +++ b/drivers/dma-buf/dma-buf-mapping.c [ ... ] > @@ -40,8 +41,12 @@ static unsigned int calc_sg_nents(struct dma_iova_state > *state, > size_t i; > > if (!state || !dma_use_iova(state)) { > - for (i = 0; i < nr_ranges; i++) > - nents += DIV_ROUND_UP(phys_vec[i].len, UINT_MAX); > + for (i = 0; i < nr_ranges; i++) { > + unsigned int added = DIV_ROUND_UP(phys_vec[i].len, > UINT_MAX); [Severity: High] This is a pre-existing issue, but could DIV_ROUND_UP wrap around on 32-bit platforms here? Since size_t is 32-bit on those architectures, phys_vec[i].len + UINT_MAX - 1 wraps around the maximum 32-bit value. For any chunk length larger than a page, added will evaluate to 0, completely failing the DMA mapping. > + > + if (check_add_overflow(nents, added, &nents)) > + return 0; > + } [ ... ] > @@ -133,6 +139,8 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct > dma_buf_attachment *attach, > } > > nents = calc_sg_nents(dma->state, phys_vec, nr_ranges, size); > + > + /* sg_alloc_table will cleanly fail and return -EINVAL if nents == 0 */ > ret = sg_alloc_table(&dma->sgt, nents, GFP_KERNEL | __GFP_ZERO); > if (ret) > goto err_free_state; [Severity: High] This is a pre-existing issue, but is it possible this jump leaks the allocated IOVA space? If sg_alloc_table fails, the jump to err_free_state frees dma->state, but doesn't call dma_iova_destroy(). Also, since this patch updates this function, I noticed a few other pre-existing issues further down in dma_buf_phys_vec_to_sgt(): [Severity: Critical] This is a pre-existing issue, but will the hardcoded zero offset overwrite mappings at the start of the IOVA space? In the loop later in this function: } else if (dma_use_iova(dma->state)) { ret = dma_iova_link(attach->dev, dma->state, phys_vec[i].paddr, 0, phys_vec[i].len, dir, DMA_ATTR_MMIO); Passing the literal 0 as the offset for every vector causes all physical vectors to be mapped to the exact same starting IOVA address. Should this use the accumulated mapped_len instead? [Severity: Critical] This is a pre-existing issue, but does this chunk-by-chunk unmap violate the DMA API? The code maps contiguous physical memory as a single chunk: addr = dma_map_phys(attach->dev, phys_vec[i].paddr, phys_vec[i].len, dir, DMA_ATTR_MMIO); But then in fill_sg_entry(), it splits this mapping into multiple scatterlist entries. During teardown in dma_buf_free_sgt() and err_unmap_dma, it unmaps these segmented pieces: 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); Can this partial unmapping of a single mapped region corrupt IOMMU state or cause undefined behavior? [Severity: High] This is a pre-existing issue, but does the error path iterate over uninitialized scatterlist entries and leak IOVA space? Looking at err_unmap_dma: err_unmap_dma: if (!i || !dma->state) { ; /* Do nothing */ If an error occurs on the first loop iteration, the !i check safely returns but skips dma_iova_destroy(), leaking the IOVA space here as well. Furthermore, the loop down in that error path shadows the index variable: } else { for_each_sgtable_dma_sg(&dma->sgt, sgl, i) dma_unmap_phys(...) } Because for_each_sgtable_dma_sg uses i as its internal iterator (iterating up to dma->sgt.nents), it resets i to 0 and processes all entries, including uninitialized zero-elements, rather than stopping at the count of successfully mapped segments. [Severity: High] This is a pre-existing issue, but would clearing orig_nents cause a silent memory leak? At the end of dma_buf_phys_vec_to_sgt(): /* * No CPU list included — set orig_nents = 0 so others can detect * this via SG table (use nents only). */ dma->sgt.orig_nents = 0; When dma_buf_free_sgt() eventually calls sg_free_table(), the underlying __sg_free_table() uses table->orig_nents to determine how many chunks to free. Since it is 0, won't the free loop terminate immediately and silently leak the entire scatterlist memory allocation? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
