* [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range @ 2018-12-24 13:26 Souptick Joarder 2018-12-24 13:26 ` Souptick Joarder 2019-01-02 10:53 ` Souptick Joarder 0 siblings, 2 replies; 7+ messages in thread From: Souptick Joarder @ 2018-12-24 13:26 UTC (permalink / raw) To: akpm, willy, mhocko, pawel, m.szyprowski, kyungmin.park, mchehab, linux, robin.murphy Cc: linux-media, linux-kernel, linux-mm Convert to use vm_insert_range to map range of kernel memory to user vma. Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> Reviewed-by: Matthew Wilcox <willy@infradead.org> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> --- 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; + } /* * Use common vm_area operations to track buffer refcount. -- 1.9.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range 2018-12-24 13:26 [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range Souptick Joarder @ 2018-12-24 13:26 ` Souptick Joarder 2019-01-02 10:53 ` Souptick Joarder 1 sibling, 0 replies; 7+ messages in thread From: Souptick Joarder @ 2018-12-24 13:26 UTC (permalink / raw) To: akpm, willy, mhocko, pawel, m.szyprowski, kyungmin.park, mchehab, linux, robin.murphy Cc: linux-media, linux-kernel, linux-mm Convert to use vm_insert_range to map range of kernel memory to user vma. Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> Reviewed-by: Matthew Wilcox <willy@infradead.org> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> --- 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; + } /* * Use common vm_area operations to track buffer refcount. -- 1.9.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range 2018-12-24 13:26 [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range Souptick Joarder 2018-12-24 13:26 ` Souptick Joarder @ 2019-01-02 10:53 ` Souptick Joarder 2019-01-02 10:53 ` Souptick Joarder 2019-01-02 11:15 ` Russell King - ARM Linux 1 sibling, 2 replies; 7+ messages in thread From: Souptick Joarder @ 2019-01-02 10:53 UTC (permalink / raw) To: Andrew Morton, Matthew Wilcox, Michal Hocko, pawel, Marek Szyprowski, Kyungmin Park, mchehab, Russell King - ARM Linux, robin.murphy Cc: linux-media, linux-kernel, Linux-MM On Mon, Dec 24, 2018 at 6:53 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > Convert to use vm_insert_range to map range of kernel memory > to user vma. > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > Reviewed-by: Matthew Wilcox <willy@infradead.org> > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > --- > 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 ? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range 2019-01-02 10:53 ` Souptick Joarder @ 2019-01-02 10:53 ` Souptick Joarder 2019-01-02 11:15 ` Russell King - ARM Linux 1 sibling, 0 replies; 7+ messages in thread From: Souptick Joarder @ 2019-01-02 10:53 UTC (permalink / raw) To: Andrew Morton, Matthew Wilcox, Michal Hocko, pawel, Marek Szyprowski, Kyungmin Park, mchehab, Russell King - ARM Linux, robin.murphy Cc: linux-media, linux-kernel, Linux-MM On Mon, Dec 24, 2018 at 6:53 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > Convert to use vm_insert_range to map range of kernel memory > to user vma. > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > Reviewed-by: Matthew Wilcox <willy@infradead.org> > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > --- > 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 ? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range 2019-01-02 10:53 ` Souptick Joarder 2019-01-02 10:53 ` Souptick Joarder @ 2019-01-02 11:15 ` Russell King - ARM Linux 2019-01-02 15:52 ` Souptick Joarder 1 sibling, 1 reply; 7+ messages in thread From: Russell King - ARM Linux @ 2019-01-02 11:15 UTC (permalink / raw) To: Souptick Joarder Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, pawel, Marek Szyprowski, Kyungmin Park, mchehab, robin.murphy, linux-media, linux-kernel, Linux-MM 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@gmail.com> wrote: > > > > Convert to use vm_insert_range to map range of kernel memory > > to user vma. > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > Reviewed-by: Matthew Wilcox <willy@infradead.org> > > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > > --- > > 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. 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range 2019-01-02 11:15 ` Russell King - ARM Linux @ 2019-01-02 15:52 ` Souptick Joarder 2019-01-02 15:52 ` Souptick Joarder 0 siblings, 1 reply; 7+ messages in thread From: Souptick Joarder @ 2019-01-02 15:52 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, pawel, Marek Szyprowski, Kyungmin Park, mchehab, robin.murphy, linux-media, linux-kernel, Linux-MM On Wed, Jan 2, 2019 at 4:46 PM Russell King - ARM Linux <linux@armlinux.org.uk> 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@gmail.com> wrote: > > > > > > Convert to use vm_insert_range to map range of kernel memory > > > to user vma. > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > Reviewed-by: Matthew Wilcox <willy@infradead.org> > > > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range 2019-01-02 15:52 ` Souptick Joarder @ 2019-01-02 15:52 ` Souptick Joarder 0 siblings, 0 replies; 7+ messages in thread From: Souptick Joarder @ 2019-01-02 15:52 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, pawel, Marek Szyprowski, Kyungmin Park, mchehab, robin.murphy, linux-media, linux-kernel, Linux-MM On Wed, Jan 2, 2019 at 4:46 PM Russell King - ARM Linux <linux@armlinux.org.uk> 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@gmail.com> wrote: > > > > > > Convert to use vm_insert_range to map range of kernel memory > > > to user vma. > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > Reviewed-by: Matthew Wilcox <willy@infradead.org> > > > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-02 15:48 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-24 13:26 [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range Souptick Joarder 2018-12-24 13:26 ` Souptick Joarder 2019-01-02 10:53 ` Souptick Joarder 2019-01-02 10:53 ` Souptick Joarder 2019-01-02 11:15 ` Russell King - ARM Linux 2019-01-02 15:52 ` Souptick Joarder 2019-01-02 15:52 ` Souptick Joarder
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox