| Message ID | 20181025061546.hnhkv33diogf2uis@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup_benchmark: prevent integer overflow in ioctl | expand |
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>
> 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
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
> 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
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)
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(+)