diff mbox series

[16/38] media: videobuf-dma-sg: number of pages should be unsigned long

Message ID a57a3584ccc16f33b2e6e8a850b7cb7cf029dfb6.1599062230.git.mchehab+huawei@kernel.org (mailing list archive)
State New, archived
Headers show
Series media sparse/smatch warn fixes | expand

Commit Message

Mauro Carvalho Chehab Sept. 2, 2020, 4:10 p.m. UTC 
As reported by smatch:

	drivers/media/v4l2-core/videobuf-dma-sg.c:245 videobuf_dma_init_kernel() warn: should 'nr_pages << 12' be a 64 bit type?

The printk should not be using %d for the number of pages.

After looking better, the real problem here is that the
number of pages should be long int.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/v4l2-core/videobuf-dma-sg.c | 22 ++++++++++++----------
 include/media/videobuf-dma-sg.h           |  2 +-
 2 files changed, 13 insertions(+), 11 deletions(-)

Comments

John Hubbard Sept. 3, 2020, 7:49 a.m. UTC | #1 
On 9/2/20 9:10 AM, Mauro Carvalho Chehab wrote:
> As reported by smatch:
> 
> 	drivers/media/v4l2-core/videobuf-dma-sg.c:245 videobuf_dma_init_kernel() warn: should 'nr_pages << 12' be a 64 bit type?
> 
> The printk should not be using %d for the number of pages.
> 
> After looking better, the real problem here is that the
> number of pages should be long int.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>   drivers/media/v4l2-core/videobuf-dma-sg.c | 22 ++++++++++++----------
>   include/media/videobuf-dma-sg.h           |  2 +-
>   2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
> index 46ff19df9f53..8dd0562de287 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-sg.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
> @@ -180,7 +180,7 @@ static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,
>   	if (rw == READ)
>   		flags |= FOLL_WRITE;
>   
> -	dprintk(1, "init user [0x%lx+0x%lx => %d pages]\n",
> +	dprintk(1, "init user [0x%lx+0x%lx => %lu pages]\n",
>   		data, size, dma->nr_pages);
>   
>   	err = pin_user_pages(data & PAGE_MASK, dma->nr_pages,


One pre-existing detail to remember is that the gup/pup routines,
specifically pin_user_pages() in this case, use an "int" for the
incoming nr_pages. (I wonder if that should be changed? It's now
becoming a pitfall.) So it's now possible to overflow.

In other situations like this (see xsdfec_table_write() in
drivers/misc/xilinx_sdfec.c), we've added checks such as:

	u32 n;
	...

	if (WARN_ON_ONCE(n > INT_MAX))
		return -EINVAL;

	nr_pages = n;

	res = pin_user_pages_fast((unsigned long)src_ptr, nr_pages, 0, pages);

...in other words, check the value while it's stored in a 64-bit type,
before sending it down into a 32-bit API.

...other than that, everything else looks fine.

thanks,
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
index 46ff19df9f53..8dd0562de287 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -180,7 +180,7 @@  static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,
 	if (rw == READ)
 		flags |= FOLL_WRITE;
 
-	dprintk(1, "init user [0x%lx+0x%lx => %d pages]\n",
+	dprintk(1, "init user [0x%lx+0x%lx => %lu pages]\n",
 		data, size, dma->nr_pages);
 
 	err = pin_user_pages(data & PAGE_MASK, dma->nr_pages,
@@ -188,7 +188,7 @@  static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,
 
 	if (err != dma->nr_pages) {
 		dma->nr_pages = (err >= 0) ? err : 0;
-		dprintk(1, "pin_user_pages: err=%d [%d]\n", err,
+		dprintk(1, "pin_user_pages: err=%d [%lu]\n", err,
 			dma->nr_pages);
 		return err < 0 ? err : -EINVAL;
 	}
@@ -208,11 +208,11 @@  static int videobuf_dma_init_user(struct videobuf_dmabuf *dma, int direction,
 }
 
 static int videobuf_dma_init_kernel(struct videobuf_dmabuf *dma, int direction,
-			     int nr_pages)
+				    unsigned long nr_pages)
 {
 	int i;
 
-	dprintk(1, "init kernel [%d pages]\n", nr_pages);
+	dprintk(1, "init kernel [%lu pages]\n", nr_pages);
 
 	dma->direction = direction;
 	dma->vaddr_pages = kcalloc(nr_pages, sizeof(*dma->vaddr_pages),
@@ -238,11 +238,11 @@  static int videobuf_dma_init_kernel(struct videobuf_dmabuf *dma, int direction,
 	dma->vaddr = vmap(dma->vaddr_pages, nr_pages, VM_MAP | VM_IOREMAP,
 			  PAGE_KERNEL);
 	if (NULL == dma->vaddr) {
-		dprintk(1, "vmalloc_32(%d pages) failed\n", nr_pages);
+		dprintk(1, "vmalloc_32(%lu pages) failed\n", nr_pages);
 		goto out_free_pages;
 	}
 
-	dprintk(1, "vmalloc is at addr %p, size=%d\n",
+	dprintk(1, "vmalloc is at addr %p, size=%lu\n",
 		dma->vaddr, nr_pages << PAGE_SHIFT);
 
 	memset(dma->vaddr, 0, nr_pages << PAGE_SHIFT);
@@ -267,9 +267,9 @@  static int videobuf_dma_init_kernel(struct videobuf_dmabuf *dma, int direction,
 }
 
 static int videobuf_dma_init_overlay(struct videobuf_dmabuf *dma, int direction,
-			      dma_addr_t addr, int nr_pages)
+			      dma_addr_t addr, unsigned long nr_pages)
 {
-	dprintk(1, "init overlay [%d pages @ bus 0x%lx]\n",
+	dprintk(1, "init overlay [%lu pages @ bus 0x%lx]\n",
 		nr_pages, (unsigned long)addr);
 	dma->direction = direction;
 
@@ -500,9 +500,11 @@  static int __videobuf_iolock(struct videobuf_queue *q,
 			     struct videobuf_buffer *vb,
 			     struct v4l2_framebuffer *fbuf)
 {
-	int err, pages;
-	dma_addr_t bus;
 	struct videobuf_dma_sg_memory *mem = vb->priv;
+	unsigned long pages;
+	dma_addr_t bus;
+	int err;
+
 	BUG_ON(!mem);
 
 	MAGIC_CHECK(mem->magic, MAGIC_SG_MEM);
diff --git a/include/media/videobuf-dma-sg.h b/include/media/videobuf-dma-sg.h
index 34450f7ad510..930ff8d454fc 100644
--- a/include/media/videobuf-dma-sg.h
+++ b/include/media/videobuf-dma-sg.h
@@ -60,7 +60,7 @@  struct videobuf_dmabuf {
 	/* common */
 	struct scatterlist  *sglist;
 	int                 sglen;
-	int                 nr_pages;
+	unsigned long       nr_pages;
 	int                 direction;
 };