Hi Marek,
On Tue, Jan 22, 2019 at 8:37 PM Marek Szyprowski
<m.szyprow...@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.li...@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
>
