[PATCH] mm/gup_benchmark: prevent integer overflow in ioctl

[PATCH] mm/gup_benchmark: prevent integer overflow in ioctl

[Dan Carpenter]

The concern here is that "gup->size" is a u64 and "nr_pages" is unsigned
long.  On 32 bit systems we could trick the kernel into allocating fewer
pages than expected.

Fixes: 64c349f4ae78 ("mm: add infrastructure for get_user_pages_fast() benchmarking")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 mm/gup_benchmark.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
index debf11388a60..5b42d3d4b60a 100644
--- a/mm/gup_benchmark.c
+++ b/mm/gup_benchmark.c
@@ -27,6 +27,9 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
 	int nr;
 	struct page **pages;
 
+	if (gup->size > ULONG_MAX)
+		return -EINVAL;
+
 	nr_pages = gup->size / PAGE_SIZE;
 	pages = kvcalloc(nr_pages, sizeof(void *), GFP_KERNEL);
 	if (!pages)
-- 
2.11.0

Re: [PATCH] mm/gup_benchmark: prevent integer overflow in ioctl

[Kirill A. Shutemov]

On Thu, Oct 25, 2018 at 09:15:46AM +0300, Dan Carpenter wrote:
> The concern here is that "gup->size" is a u64 and "nr_pages" is unsigned
> long.  On 32 bit systems we could trick the kernel into allocating fewer
> pages than expected.
> 
> Fixes: 64c349f4ae78 ("mm: add infrastructure for get_user_pages_fast() benchmarking")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  mm/gup_benchmark.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index debf11388a60..5b42d3d4b60a 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -27,6 +27,9 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  	int nr;
>  	struct page **pages;
>  
> +	if (gup->size > ULONG_MAX)
> +		return -EINVAL;
> +

Strictly speaking gup->size / PAGE_SIZE has to be <= ULONG_MAX, but it
should be fine this way too.

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

Re: [PATCH] mm/gup_benchmark: prevent integer overflow in ioctl

[William Kucharski]

> On Oct 25, 2018, at 12:15 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> The concern here is that "gup->size" is a u64 and "nr_pages" is unsigned
> long.  On 32 bit systems we could trick the kernel into allocating fewer
> pages than expected.
> 
> Fixes: 64c349f4ae78 ("mm: add infrastructure for get_user_pages_fast() benchmarking")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> mm/gup_benchmark.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index debf11388a60..5b42d3d4b60a 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -27,6 +27,9 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> 	int nr;
> 	struct page **pages;
> 
> +	if (gup->size > ULONG_MAX)
> +		return -EINVAL;
> +
> 	nr_pages = gup->size / PAGE_SIZE;
> 	pages = kvcalloc(nr_pages, sizeof(void *), GFP_KERNEL);
> 	if (!pages)

Given gup->size is in bytes, if your goal is to avoid an overflow of nr_pages on 32-bit
systems, shouldn't you be checking something like:

    if ((gup_size / PAGE_SIZE) > ULONG_MAX)

instead?

    William Kucharski

Re: [PATCH] mm/gup_benchmark: prevent integer overflow in ioctl

[Dan Carpenter]

On Thu, Nov 01, 2018 at 12:38:22AM -0600, William Kucharski wrote:
> 
> 
> > On Oct 25, 2018, at 12:15 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > 
> > The concern here is that "gup->size" is a u64 and "nr_pages" is unsigned
> > long.  On 32 bit systems we could trick the kernel into allocating fewer
> > pages than expected.
> > 
> > Fixes: 64c349f4ae78 ("mm: add infrastructure for get_user_pages_fast() benchmarking")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > mm/gup_benchmark.c | 3 +++
> > 1 file changed, 3 insertions(+)
> > 
> > diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> > index debf11388a60..5b42d3d4b60a 100644
> > --- a/mm/gup_benchmark.c
> > +++ b/mm/gup_benchmark.c
> > @@ -27,6 +27,9 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> > 	int nr;
> > 	struct page **pages;
> > 
> > +	if (gup->size > ULONG_MAX)
> > +		return -EINVAL;
> > +
> > 	nr_pages = gup->size / PAGE_SIZE;
> > 	pages = kvcalloc(nr_pages, sizeof(void *), GFP_KERNEL);
> > 	if (!pages)
> 
> Given gup->size is in bytes, if your goal is to avoid an overflow of nr_pages on 32-bit
> systems, shouldn't you be checking something like:
> 
>     if ((gup_size / PAGE_SIZE) > ULONG_MAX)

My patch lets people allocate 4MB.  (U32_MAX / 4096 * sizeof(void *)).
Surely, that's enough?  I liked my check because it avoids the divide so
it's faster and it is a no-op on 64bit systems.

regards,
dan carpenter

Re: [PATCH] mm/gup_benchmark: prevent integer overflow in ioctl

[William Kucharski]

> On Nov 1, 2018, at 1:16 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> My patch lets people allocate 4MB.  (U32_MAX / 4096 * sizeof(void *)).
> Surely, that's enough?  I liked my check because it avoids the divide so
> it's faster and it is a no-op on 64bit systems.

It should be enough, and you're right, it does avoid extra math.

However, in that case I'd like to see a comment added so that anyone looking
at the code in the future knows why you limited the allocation to ULONG_MAX
bytes.

Thanks,
    William Kucharski