[PATCH 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range_buggy

[PATCH 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range_buggy

[Souptick Joarder]

Convert to use vm_insert_range_buggy to map range of kernel memory
to user vma.

This driver has ignored vm_pgoff. We could later "fix" these drivers
to behave according to the normal vm_pgoff offsetting simply by
removing the _buggy suffix on the function name and if that causes
regressions, it gives us an easy way to revert.

There is an existing bug inside gem_mmap_obj(), where user passed
length is not checked against buf->num_pages. For any value of
length > buf->num_pages it will end up overrun buf->pages[i],
which could lead to a potential bug.

This has been addressed by passing buf->num_pages as input to
vm_insert_range_buggy() and inside this API error condition is
checked which will avoid overrun the page boundary.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 015e737..ef046b4 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -328,28 +328,18 @@ static unsigned int vb2_dma_sg_num_users(void *buf_priv)
 static int vb2_dma_sg_mmap(void *buf_priv, struct vm_area_struct *vma)
 {
 	struct vb2_dma_sg_buf *buf = buf_priv;
-	unsigned long uaddr = vma->vm_start;
-	unsigned long usize = vma->vm_end - vma->vm_start;
-	int i = 0;
+	int err;
 
 	if (!buf) {
 		printk(KERN_ERR "No memory to map\n");
 		return -EINVAL;
 	}
 
-	do {
-		int ret;
-
-		ret = vm_insert_page(vma, uaddr, buf->pages[i++]);
-		if (ret) {
-			printk(KERN_ERR "Remapping memory, error: %d\n", ret);
-			return ret;
-		}
-
-		uaddr += PAGE_SIZE;
-		usize -= PAGE_SIZE;
-	} while (usize > 0);
-
+	err = vm_insert_range_buggy(vma, buf->pages, buf->num_pages);
+	if (err) {
+		printk(KERN_ERR "Remapping memory, error: %d\n", err);
+		return err;
+	}
 
 	/*
 	 * Use common vm_area operations to track buffer refcount.
-- 
1.9.1

Re: [PATCH 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range_buggy

[Marek Szyprowski]

Hi Souptick,

On 2019-01-11 16:11, Souptick Joarder wrote:
> Convert to use vm_insert_range_buggy to map range of kernel memory
> to user vma.
>
> This driver has ignored vm_pgoff. We could later "fix" these drivers
> to behave according to the normal vm_pgoff offsetting simply by
> removing the _buggy suffix on the function name and if that causes
> regressions, it gives us an easy way to revert.

Just a generic note about videobuf2: videobuf2-dma-sg is ignoring vm_pgoff by design. vm_pgoff is used as a 'cookie' to select a buffer to mmap and videobuf2-core already checks that. If userspace provides an offset, which doesn't match any of the registered 'cookies' (reported to userspace via separate v4l2 ioctl), an error is returned.

I'm sorry for the late reply.

> There is an existing bug inside gem_mmap_obj(), where user passed
> length is not checked against buf->num_pages. For any value of
> length > buf->num_pages it will end up overrun buf->pages[i],
> which could lead to a potential bug.
>
> This has been addressed by passing buf->num_pages as input to
> vm_insert_range_buggy() and inside this API error condition is
> checked which will avoid overrun the page boundary.
>
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> ---
>  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index 015e737..ef046b4 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -328,28 +328,18 @@ static unsigned int vb2_dma_sg_num_users(void *buf_priv)
>  static int vb2_dma_sg_mmap(void *buf_priv, struct vm_area_struct *vma)
>  {
>  	struct vb2_dma_sg_buf *buf = buf_priv;
> -	unsigned long uaddr = vma->vm_start;
> -	unsigned long usize = vma->vm_end - vma->vm_start;
> -	int i = 0;
> +	int err;
>  
>  	if (!buf) {
>  		printk(KERN_ERR "No memory to map\n");
>  		return -EINVAL;
>  	}
>  
> -	do {
> -		int ret;
> -
> -		ret = vm_insert_page(vma, uaddr, buf->pages[i++]);
> -		if (ret) {
> -			printk(KERN_ERR "Remapping memory, error: %d\n", ret);
> -			return ret;
> -		}
> -
> -		uaddr += PAGE_SIZE;
> -		usize -= PAGE_SIZE;
> -	} while (usize > 0);
> -
> +	err = vm_insert_range_buggy(vma, buf->pages, buf->num_pages);
> +	if (err) {
> +		printk(KERN_ERR "Remapping memory, error: %d\n", err);
> +		return err;
> +	}
>  
>  	/*
>  	 * Use common vm_area operations to track buffer refcount.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range_buggy

[Souptick Joarder]

Hi Marek,

On Tue, Jan 22, 2019 at 8:37 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Souptick,
>
> On 2019-01-11 16:11, Souptick Joarder wrote:
> > Convert to use vm_insert_range_buggy to map range of kernel memory
> > to user vma.
> >
> > This driver has ignored vm_pgoff. We could later "fix" these drivers
> > to behave according to the normal vm_pgoff offsetting simply by
> > removing the _buggy suffix on the function name and if that causes
> > regressions, it gives us an easy way to revert.
>
> Just a generic note about videobuf2: videobuf2-dma-sg is ignoring vm_pgoff by design. vm_pgoff is used as a 'cookie' to select a buffer to mmap and videobuf2-core already checks that. If userspace provides an offset, which doesn't match any of the registered 'cookies' (reported to userspace via separate v4l2 ioctl), an error is returned.

Ok, it means once the buf is selected, videobuf2-dma-sg should always
mapped buf->pages[i]
from index 0 ( irrespective of vm_pgoff value). So although we are
replacing the code with
vm_insert_range_buggy(), *_buggy* suffix will mislead others and
should not be used.
And if we replace this code with  vm_insert_range(), this will
introduce bug for *non zero*
value of vm_pgoff.

Please correct me if my understanding is wrong.

So what your opinion about this patch ? Shall I drop this patch from
current series ?
or,
There is any better way to handle this scenario ?


>
> > There is an existing bug inside gem_mmap_obj(), where user passed
> > length is not checked against buf->num_pages. For any value of
> > length > buf->num_pages it will end up overrun buf->pages[i],
> > which could lead to a potential bug.

It is not gem_mmap_obj(), it should be vb2_dma_sg_mmap().
Sorry about it.

What about this issue ? Does it looks like a valid issue ?


> >
> > This has been addressed by passing buf->num_pages as input to
> > vm_insert_range_buggy() and inside this API error condition is
> > checked which will avoid overrun the page boundary.
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > ---
> >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 22 ++++++----------------
> >  1 file changed, 6 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > index 015e737..ef046b4 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > @@ -328,28 +328,18 @@ static unsigned int vb2_dma_sg_num_users(void *buf_priv)
> >  static int vb2_dma_sg_mmap(void *buf_priv, struct vm_area_struct *vma)
> >  {
> >       struct vb2_dma_sg_buf *buf = buf_priv;
> > -     unsigned long uaddr = vma->vm_start;
> > -     unsigned long usize = vma->vm_end - vma->vm_start;
> > -     int i = 0;
> > +     int err;
> >
> >       if (!buf) {
> >               printk(KERN_ERR "No memory to map\n");
> >               return -EINVAL;
> >       }
> >
> > -     do {
> > -             int ret;
> > -
> > -             ret = vm_insert_page(vma, uaddr, buf->pages[i++]);
> > -             if (ret) {
> > -                     printk(KERN_ERR "Remapping memory, error: %d\n", ret);
> > -                     return ret;
> > -             }
> > -
> > -             uaddr += PAGE_SIZE;
> > -             usize -= PAGE_SIZE;
> > -     } while (usize > 0);
> > -
> > +     err = vm_insert_range_buggy(vma, buf->pages, buf->num_pages);
> > +     if (err) {
> > +             printk(KERN_ERR "Remapping memory, error: %d\n", err);
> > +             return err;
> > +     }
> >
> >       /*
> >        * Use common vm_area operations to track buffer refcount.
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range_buggy

[Marek Szyprowski]

Hi Souptick,

On 2019-01-25 05:55, Souptick Joarder wrote:
> On Tue, Jan 22, 2019 at 8:37 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 2019-01-11 16:11, Souptick Joarder wrote:
>>> Convert to use vm_insert_range_buggy to map range of kernel memory
>>> to user vma.
>>>
>>> This driver has ignored vm_pgoff. We could later "fix" these drivers
>>> to behave according to the normal vm_pgoff offsetting simply by
>>> removing the _buggy suffix on the function name and if that causes
>>> regressions, it gives us an easy way to revert.
>> Just a generic note about videobuf2: videobuf2-dma-sg is ignoring vm_pgoff by design. vm_pgoff is used as a 'cookie' to select a buffer to mmap and videobuf2-core already checks that. If userspace provides an offset, which doesn't match any of the registered 'cookies' (reported to userspace via separate v4l2 ioctl), an error is returned.
> Ok, it means once the buf is selected, videobuf2-dma-sg should always
> mapped buf->pages[i]
> from index 0 ( irrespective of vm_pgoff value). So although we are
> replacing the code with
> vm_insert_range_buggy(), *_buggy* suffix will mislead others and
> should not be used.
> And if we replace this code with  vm_insert_range(), this will
> introduce bug for *non zero*
> value of vm_pgoff.
>
> Please correct me if my understanding is wrong.

You are correct. IMHO the best solution in this case would be to add
following fix:


diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 70e8c3366f9c..ca4577a7d28a 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2175,6 +2175,13 @@ int vb2_mmap(struct vb2_queue *q, struct
vm_area_struct *vma)
         goto unlock;
     }
 
+    /*
+     * vm_pgoff is treated in V4L2 API as a 'cookie' to select a buffer,
+     * not as a in-buffer offset. We always want to mmap a whole buffer
+     * from its beginning.
+     */
+    vma->vm_pgoff = 0;
+
     ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
 
 unlock:
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index aff0ab7bf83d..46245c598a18 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -186,12 +186,6 @@ static int vb2_dc_mmap(void *buf_priv, struct
vm_area_struct *vma)
         return -EINVAL;
     }
 
-    /*
-     * dma_mmap_* uses vm_pgoff as in-buffer offset, but we want to
-     * map whole buffer
-     */
-    vma->vm_pgoff = 0;
-
     ret = dma_mmap_attrs(buf->dev, vma, buf->cookie,
         buf->dma_addr, buf->size, buf->attrs);
 
-- 

Then you can simply use non-buggy version of your function in
drivers/media/common/videobuf2/videobuf2-dma-sg.c.

I can send above as a formal patch if you want.

> So what your opinion about this patch ? Shall I drop this patch from
> current series ?
> or,
> There is any better way to handle this scenario ?
>
>
>>> There is an existing bug inside gem_mmap_obj(), where user passed
>>> length is not checked against buf->num_pages. For any value of
>>> length > buf->num_pages it will end up overrun buf->pages[i],
>>> which could lead to a potential bug.
> It is not gem_mmap_obj(), it should be vb2_dma_sg_mmap().
> Sorry about it.
>
> What about this issue ? Does it looks like a valid issue ?

It is already handled in vb2_mmap(). Such call will be rejected.


> ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range_buggy

[Souptick Joarder]

Hi Marek,

On Fri, Jan 25, 2019 at 5:58 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Souptick,
>
> On 2019-01-25 05:55, Souptick Joarder wrote:
> > On Tue, Jan 22, 2019 at 8:37 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 2019-01-11 16:11, Souptick Joarder wrote:
> >>> Convert to use vm_insert_range_buggy to map range of kernel memory
> >>> to user vma.
> >>>
> >>> This driver has ignored vm_pgoff. We could later "fix" these drivers
> >>> to behave according to the normal vm_pgoff offsetting simply by
> >>> removing the _buggy suffix on the function name and if that causes
> >>> regressions, it gives us an easy way to revert.
> >> Just a generic note about videobuf2: videobuf2-dma-sg is ignoring vm_pgoff by design. vm_pgoff is used as a 'cookie' to select a buffer to mmap and videobuf2-core already checks that. If userspace provides an offset, which doesn't match any of the registered 'cookies' (reported to userspace via separate v4l2 ioctl), an error is returned.
> > Ok, it means once the buf is selected, videobuf2-dma-sg should always
> > mapped buf->pages[i]
> > from index 0 ( irrespective of vm_pgoff value). So although we are
> > replacing the code with
> > vm_insert_range_buggy(), *_buggy* suffix will mislead others and
> > should not be used.
> > And if we replace this code with  vm_insert_range(), this will
> > introduce bug for *non zero*
> > value of vm_pgoff.
> >
> > Please correct me if my understanding is wrong.
>
> You are correct. IMHO the best solution in this case would be to add
> following fix:
>
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 70e8c3366f9c..ca4577a7d28a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -2175,6 +2175,13 @@ int vb2_mmap(struct vb2_queue *q, struct
> vm_area_struct *vma)
>          goto unlock;
>      }
>
> +    /*
> +     * vm_pgoff is treated in V4L2 API as a 'cookie' to select a buffer,
> +     * not as a in-buffer offset. We always want to mmap a whole buffer
> +     * from its beginning.
> +     */
> +    vma->vm_pgoff = 0;
> +
>      ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
>
>  unlock:
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index aff0ab7bf83d..46245c598a18 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -186,12 +186,6 @@ static int vb2_dc_mmap(void *buf_priv, struct
> vm_area_struct *vma)
>          return -EINVAL;
>      }
>
> -    /*
> -     * dma_mmap_* uses vm_pgoff as in-buffer offset, but we want to
> -     * map whole buffer
> -     */
> -    vma->vm_pgoff = 0;
> -
>      ret = dma_mmap_attrs(buf->dev, vma, buf->cookie,
>          buf->dma_addr, buf->size, buf->attrs);
>
> --
>
> Then you can simply use non-buggy version of your function in
> drivers/media/common/videobuf2/videobuf2-dma-sg.c.
>
> I can send above as a formal patch if you want.

Thanks for the patch.
I will fold this changes along with current patch in v2.

>
> > So what your opinion about this patch ? Shall I drop this patch from
> > current series ?
> > or,
> > There is any better way to handle this scenario ?
> >
> >
> >>> There is an existing bug inside gem_mmap_obj(), where user passed
> >>> length is not checked against buf->num_pages. For any value of
> >>> length > buf->num_pages it will end up overrun buf->pages[i],
> >>> which could lead to a potential bug.
> > It is not gem_mmap_obj(), it should be vb2_dma_sg_mmap().
> > Sorry about it.
> >
> > What about this issue ? Does it looks like a valid issue ?
>
> It is already handled in vb2_mmap(). Such call will be rejected.
>
>
> > ...
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>