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
