Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 16/38] media: videobuf-dma-sg: number of pages should be
 unsigned long
To:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Michel Lespinasse <walken@google.com>,
        Mike Rapoport <rppt@kernel.org>, <linux-media@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <cover.1599062230.git.mchehab+huawei@kernel.org>
 <a57a3584ccc16f33b2e6e8a850b7cb7cf029dfb6.1599062230.git.mchehab+huawei@kernel.org>
From:   John Hubbard <jhubbard@nvidia.com>
Message-ID: <29cbe38a-4094-5d60-9f85-050bb44febcc@nvidia.com>
Date:   Thu, 3 Sep 2020 00:49:47 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <a57a3584ccc16f33b2e6e8a850b7cb7cf029dfb6.1599062230.git.mchehab+huawei@kernel.org>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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