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

[Souptick Joarder]

On Wed, Jan 2, 2019 at 4:46 PM Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxx> wrote:
>
> On Wed, Jan 02, 2019 at 04:23:15PM +0530, Souptick Joarder wrote:
> > On Mon, Dec 24, 2018 at 6:53 PM Souptick Joarder <jrdr.linux@xxxxxxxxx> wrote:
> > >
> > > Convert to use vm_insert_range to map range of kernel memory
> > > to user vma.
> > >
> > > Signed-off-by: Souptick Joarder <jrdr.linux@xxxxxxxxx>
> > > Reviewed-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > > Acked-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > > Acked-by: Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx>
> > > ---
> > >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 23 +++++++----------------
> > >  1 file changed, 7 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..898adef 100644
> > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > @@ -328,28 +328,19 @@ 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;
> > > +       unsigned long page_count = vma_pages(vma);
> > > +       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(vma, vma->vm_start, buf->pages, page_count);
> > > +       if (err) {
> > > +               printk(KERN_ERR "Remapping memory, error: %d\n", err);
> > > +               return err;
> > > +       }
> > >
> >
> > Looking into the original code -
> > drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >
> > Inside vb2_dma_sg_alloc(),
> >            ...
> >            buf->num_pages = size >> PAGE_SHIFT;
> >            buf->dma_sgt = &buf->sg_table;
> >
> >            buf->pages = kvmalloc_array(buf->num_pages, sizeof(struct page *),
> >                                                        GFP_KERNEL | __GFP_ZERO);
> >            ...
> >
> > buf->pages has index upto  *buf->num_pages*.
> >
> > now inside vb2_dma_sg_mmap(),
> >
> >            unsigned long usize = vma->vm_end - vma->vm_start;
> >            int i = 0;
> >            ...
> >            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);
> >            ...
> > is it possible for any value of  *i  > (buf->num_pages)*,
> > buf->pages[i] is going to overrun the page boundary ?
>
> Yes it is, and you've found an array-overrun condition that is
> triggerable from userspace - potentially non-root userspace too.
> Depending on what it can cause to be mapped without oopsing the
> kernel, it could be very serious.  At best, it'll oops the kernel.
> At worst, it could expose pages of memory that userspace should
> not have access to.
>
> This is why I've been saying that we need a helper that takes the
> _object_ and the user request, and does all the checking internally,
> so these kinds of checks do not get overlooked.

ok, while replacing this code with the suggested vm_insert_range_buggy(),
we could fixed this issue.


>
> A good API is one that helpers authors avoid bugs.
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up