linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Souptick Joarder <jrdr.linux@gmail.com>
To: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Matthew Wilcox <willy@infradead.org>,
	Michal Hocko <mhocko@suse.com>,
	pawel@osciak.com, Kyungmin Park <kyungmin.park@samsung.com>,
	mchehab@kernel.org,
	Russell King - ARM Linux <linux@armlinux.org.uk>,
	robin.murphy@arm.com, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org, Linux-MM <linux-mm@kvack.org>
Subject: Re: [PATCH 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range_buggy
Date: Fri, 25 Jan 2019 10:25:48 +0530	[thread overview]
Message-ID: <CAFqt6zbYHq-pS=rGx+3ncJ7rO-LvL5=iOou21oguKjrc=3qouA@mail.gmail.com> (raw)
In-Reply-To: <241810e0-2288-c59b-6c21-6d853d9fe84a@samsung.com>

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
>

  reply	other threads:[~2019-01-25  4:56 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20190111150806epcas2p4ecaac58547db019e7dc779349d495f4d@epcas2p4.samsung.com>
2019-01-11 15:11 ` [PATCH 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range_buggy Souptick Joarder
2019-01-22 15:06   ` Marek Szyprowski
2019-01-25  4:55     ` Souptick Joarder [this message]
2019-01-25 12:27       ` Marek Szyprowski
2019-01-27 16:31         ` Souptick Joarder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAFqt6zbYHq-pS=rGx+3ncJ7rO-LvL5=iOou21oguKjrc=3qouA@mail.gmail.com' \
    --to=jrdr.linux@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux@armlinux.org.uk \
    --cc=m.szyprowski@samsung.com \
    --cc=mchehab@kernel.org \
    --cc=mhocko@suse.com \
    --cc=pawel@osciak.com \
    --cc=robin.murphy@arm.com \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).