1. Patchew
  2. linux
  3. View series

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

David Hu posted 1 patch 1 month ago
There is a newer version of this series
drivers/dma-buf/dma-buf-mapping.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] dma-buf: Fix silent overflow for phys vec to sgt
Posted by David Hu 1 month ago 
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
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@google.com>
---
 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;
-- 
2.54.0.563.g4f69b47b94-goog
Re: [PATCH] dma-buf: Fix silent overflow for phys vec to sgt
Posted by Pranjal Shrivastava 2 weeks, 3 days ago 
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@google.com>
> ---
> 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@google.com>

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
Re: [PATCH] dma-buf: Fix silent overflow for phys vec to sgt
Posted by David Hu 2 weeks, 2 days ago 
On Tue, May 26, 2026 at 1:33 PM Pranjal Shrivastava <praan@google.com> 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@google.com>
> > ---
> > 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@google.com>
>
> 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.

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.