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

[John Hubbard]

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@xxxxxxxxxx>
> ---
>   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,
--
John Hubbard
NVIDIA