Re: [PATCH] dma-buf: Fix silent overflow for phys vec to sgt

[David Hu]

On Tue, May 26, 2026 at 1:33 PM Pranjal Shrivastava <praan@xxxxxxxxxx> wrote:
>
> On Mon, May 11, 2026, David Hu wrote:
> > In case MMIO size is bigger than 4G, and peer2peer
> > dma goes through host bridge, we trigger the code
> > path to assign total linked IVOA, greater than 4G
>
> Nit: s/IVOA/IOVA
>
> > to mapped_len, and leading to a silent overflow
>
> > Fixes: 3aa31a8bb11e ("dma-buf: provide phys_vec to scatter-gather mapping routine")
> > Signed-off-by: David Hu <xuehaohu@xxxxxxxxxx>
> > ---
> > drivers/dma-buf/dma-buf-mapping.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
>
> > diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> > index 794acff2546a..658064140357 100644
> > --- a/drivers/dma-buf/dma-buf-mapping.c
> > +++ b/drivers/dma-buf/dma-buf-mapping.c
> > @@ -95,7 +95,8 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct dma_buf_attachment *attach,
> >                                        size_t nr_ranges, size_t size,
> >                                        enum dma_data_direction dir)
> > {
> > -     unsigned int nents, mapped_len = 0;
> > +     unsigned int nents = 0;
> > +     size_t mapped_len = 0;
> >       struct dma_buf_dma *dma;
> >       struct scatterlist *sgl;
> >       dma_addr_t addr;
>
> Minor nit: Let's follow the reverse xmas tree format?
> This looks correct to me, for this change:
>
> Reviewed-by: Pranjal Shrivastava <praan@xxxxxxxxxx>
>
> Apart from this, I see similar issues at other places:
>
>   1. In calc_sg_nents(), nents is accumulated as an unsigned int. [1]
>      If nr_ranges is very large, nents could also overflow, potentially
>      leading to a small allocation in sg_alloc_table() and a subsequent
>      out-of-bounds access in the mapping loop. It might be worth changing
>      nents to size_t there and adding a check against UINT_MAX.
>
>    2. In fill_sg_entry(), the loop variable i is an int [2]. Changing
>      it to unsigned int would be more consistent with the nents type
>      and safer for extremely large mappings.
>
>
> Maybe, we should also fix these? For example:
>
> diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> index 794acff2546a..ecf07ffca2b9 100644
> --- a/drivers/dma-buf/dma-buf-mapping.c
> +++ b/drivers/dma-buf/dma-buf-mapping.c
> @@ -10,7 +10,7 @@ static struct scatterlist *fill_sg_entry(struct scatterlist *sgl, size_t length,
>                                          dma_addr_t addr)
>  {
>         unsigned int len, nents;
> -       int i;
> +       unsigned int i;
>
>         nents = DIV_ROUND_UP(length, UINT_MAX);
>         for (i = 0; i < nents; i++) {
> @@ -36,7 +36,7 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
>                                   struct phys_vec *phys_vec, size_t nr_ranges,
>                                   size_t size)
>  {
> -       unsigned int nents = 0;
> +       size_t nents = 0;
>         size_t i;
>
>         if (!state || !dma_use_iova(state)) {
> @@ -51,6 +51,9 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
>                 nents = DIV_ROUND_UP(size, UINT_MAX);
>         }
>
> +       if (nents > UINT_MAX)
> +               return 0;
> +
>         return nents;
>  }
>
> Thanks,
> Praan
>
> [1] https://elixir.bootlin.com/linux/v7.1-rc3/source/drivers/dma-buf/dma-buf-mapping.c#L39
> [2] https://elixir.bootlin.com/linux/v7.1-rc3/source/drivers/dma-buf/dma-buf-mapping.c#L13
Thank you Pranjal for the review ! Good catch on other potential
overflow sites. I have folded in your suggestions for calc_sg_nents(),
and fill_sg_entry(), and applied reverse xmas tree formatting. Sending
out a v2 shortly.