Re: [PATCH 3/3] dma-buf: validate the attachment before asserting its reservation lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] dma_iova_link() is called with a hardcoded offset of 0, causing all physical ranges to overlap at the start of the IOVA space.
- [High] IOVA space allocated by dma_iova_try_alloc() is leaked on multiple error paths.
- [High] The fallback error path unconditionally unmaps unpopulated, zero-initialized scatterlist entries by overwriting the loop counter.
- [High] Mapping a large region as a single chunk but unmapping it in smaller split chunks violates the DMA API.
--

commit 139d9e764ec01d51d2adf94f612620ee800a32bb
Author: Davidlohr Bueso <dave@stgolabs.net>

dma-buf: validate the attachment before asserting its reservation lock

dma_buf_phys_vec_to_sgt() asserts the reservation lock before its
NULL checks run. Validate first, then assert.

> diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> index 833be519e1e6a..12674d89b23eb 100644
> --- a/drivers/dma-buf/dma-buf-mapping.c
> +++ b/drivers/dma-buf/dma-buf-mapping.c
> @@ -111,12 +111,12 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct dma_buf_attachment *attach,
>  	size_t i;
>  	int ret;
>  
> -	dma_resv_assert_held(attach->dmabuf->resv);
> -
>  	if (WARN_ON(!attach || !attach->dmabuf || !provider))
>  		/* This function is supposed to work on MMIO memory only */
>  		return ERR_PTR(-EINVAL);
>  
> +	dma_resv_assert_held(attach->dmabuf->resv);
> +
>  	dma = kzalloc_obj(*dma);
>  	if (!dma)
>  		return ERR_PTR(-ENOMEM);

[Severity: High]
This is a pre-existing issue, but does this code leak the IOVA space allocated
by dma_iova_try_alloc() on multiple error paths?

If sg_alloc_table() fails, the code jumps to err_free_state:

    ret = sg_alloc_table(&dma->sgt, nents, GFP_KERNEL | __GFP_ZERO);
    if (ret)
        goto err_free_state;

This frees dma->state but skips calling dma_iova_free() to release the IOVA
allocation.

Additionally, if the very first physical range fails to link (when i == 0),
the error handler skips destruction:

    err_unmap_dma:
        if (!i || !dma->state) {
            ; /* Do nothing */
        } else if (dma_use_iova(dma->state)) {
            dma_iova_destroy(attach->dev, dma->state, mapped_len, dir,
                             DMA_ATTR_MMIO);

Can this exhaust device IOMMU IOVA space under memory pressure?

[Severity: Critical]
This is a pre-existing issue, but does this loop incorrectly map all
physical ranges to the exact same starting IOVA address?

    } 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);

When nr_ranges > 1, dma_iova_link() is called repeatedly with the offset
hardcoded to 0. Since the target IOVA address is computed as
(state->addr + offset), each physical range overwrites the previous mapping.

Should the offset be passed as mapped_len to ensure segments are appended
sequentially?

[Severity: High]
This is a pre-existing issue, but could this teardown logic violate the DMA
API by mapping a large region as a single chunk and unmapping it
piece-by-piece?

During mapping, a physical range might be mapped in a single contiguous block:

    addr = dma_map_phys(attach->dev, phys_vec[i].paddr,
                        phys_vec[i].len, dir,
                        DMA_ATTR_MMIO);

Later during teardown, the fallback error path iterates over the individual
split SG entries and unmaps them separately:

    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 same piece-by-piece unmapping also happens in dma_buf_free_sgt().

Since the DMA API requires that the size and address passed to dma_unmap_phys()
exactly match the original call to dma_map_phys(), could these partial unmaps
corrupt IOMMU tracking structures?

[Severity: High]
This is a pre-existing issue, but does the fallback error path unconditionally
unmap unpopulated, zero-initialized scatterlist entries?

    } 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 shadows and resets 'i' to 0, looping over
all allocated entries (dma->sgt.nents) rather than just the successfully
mapped ranges.

Since the table was allocated with __GFP_ZERO, the unpopulated entries will
have a length and address of 0. Could passing these to dma_unmap_phys()
trigger IOMMU faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914212258.2657788-1-dave@stgolabs.net?part=3