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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <29cbe38a-4094-5d60-9f85-050bb44febcc@nvidia.com>
Date:   Thu, 3 Sep 2020 00:49:47 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Michel Lespinasse <walken@...gle.com>,
        Mike Rapoport <rppt@...nel.org>, <linux-media@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 16/38] media: videobuf-dma-sg: number of pages should be
 unsigned long

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