| Message ID | a57a3584ccc16f33b2e6e8a850b7cb7cf029dfb6.1599062230.git.mchehab+huawei@kernel.org |
|---|---|
| State | Accepted |
| Commit | 1faa39e0f3bcfe47dc7a61a72c234b24005c3a1a |
| Headers | show |
| Series | [01/38] media: tda10086: cleanup symbol_rate setting logic | expand |
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 --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; };
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(-)