Re: [PATCH] kernel buffer overflow kmalloc_slab() fix

Re: [PATCH] kernel buffer overflow kmalloc_slab() fix

[James Bottomley]

On Thu, 2011-05-19 at 15:51 -0500, Christoph Lameter wrote:
> On Thu, 19 May 2011, james_p_freyensee@linux.intel.com wrote:
> 
> > From: J Freyensee <james_p_freyensee@linux.intel.com>
> >
> > Currently, kmalloc_index() can return -1, which can be
> > passed right to the kmalloc_caches[] array, cause a
> 
> No kmalloc_index() cannot return -1 for the use case that you are
> considering here. The value passed as a size to
> kmalloc_slab is bounded by 2 * PAGE_SIZE and kmalloc_slab will only return
> -1 for sizes > 4M. So we will have to get machines with page sizes > 2M
> before this can be triggered.

Please don't make x86 centric assumptions like this.  I was vaguely
thinking about hugepages in parisc.  Like most risc machines, we have
(and have had for over a decade) a vast number of variable size pages
(actually from 4k to 64MB in power of 4 steps) and I think sparc is
similar, so I was wondering what to choose.  You'd have been deeply
annoyed if I'd chosen 4MB and had slub fall over (again).

linux-arch cc'd just so everyone else is aware of these limitations when
they implement hugepages.

James

Re: [PATCH] kernel buffer overflow kmalloc_slab() fix

[Christoph Lameter]

On Fri, 20 May 2011, James Bottomley wrote:

> On Thu, 2011-05-19 at 15:51 -0500, Christoph Lameter wrote:
> > On Thu, 19 May 2011, james_p_freyensee@linux.intel.com wrote:
> >
> > > From: J Freyensee <james_p_freyensee@linux.intel.com>
> > >
> > > Currently, kmalloc_index() can return -1, which can be
> > > passed right to the kmalloc_caches[] array, cause a
> >
> > No kmalloc_index() cannot return -1 for the use case that you are
> > considering here. The value passed as a size to
> > kmalloc_slab is bounded by 2 * PAGE_SIZE and kmalloc_slab will only return
> > -1 for sizes > 4M. So we will have to get machines with page sizes > 2M
> > before this can be triggered.
>
> Please don't make x86 centric assumptions like this.  I was vaguely
> thinking about hugepages in parisc.  Like most risc machines, we have
> (and have had for over a decade) a vast number of variable size pages
> (actually from 4k to 64MB in power of 4 steps) and I think sparc is
> similar, so I was wondering what to choose.  You'd have been deeply
> annoyed if I'd chosen 4MB and had slub fall over (again).
>
> linux-arch cc'd just so everyone else is aware of these limitations when
> they implement hugepages.

Well the simple solution would to put a BUG() in kmalloc_slab()
instead of returning -1 (whereupon the compiler will complain that we are
not returning a value). We tried the BUILD_BUG before but some compilers
are not playing ball there.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] kernel buffer overflow kmalloc_slab() fix

[Christoph Lameter]

On Fri, 20 May 2011, James Bottomley wrote:

> On Thu, 2011-05-19 at 15:51 -0500, Christoph Lameter wrote:
> > On Thu, 19 May 2011, james_p_freyensee@linux.intel.com wrote:
> >
> > > From: J Freyensee <james_p_freyensee@linux.intel.com>
> > >
> > > Currently, kmalloc_index() can return -1, which can be
> > > passed right to the kmalloc_caches[] array, cause a
> >
> > No kmalloc_index() cannot return -1 for the use case that you are
> > considering here. The value passed as a size to
> > kmalloc_slab is bounded by 2 * PAGE_SIZE and kmalloc_slab will only return
> > -1 for sizes > 4M. So we will have to get machines with page sizes > 2M
> > before this can be triggered.
>
> Please don't make x86 centric assumptions like this.  I was vaguely
> thinking about hugepages in parisc.  Like most risc machines, we have
> (and have had for over a decade) a vast number of variable size pages
> (actually from 4k to 64MB in power of 4 steps) and I think sparc is
> similar, so I was wondering what to choose.  You'd have been deeply
> annoyed if I'd chosen 4MB and had slub fall over (again).
>
> linux-arch cc'd just so everyone else is aware of these limitations when
> they implement hugepages.

Well the simple solution would to put a BUG() in kmalloc_slab()
instead of returning -1 (whereupon the compiler will complain that we are
not returning a value). We tried the BUILD_BUG before but some compilers
are not playing ball there.

Re: [PATCH] kernel buffer overflow kmalloc_slab() fix

[Christoph Lameter]

Subject: slub: Deal with hyperthetical case of PAGE_SIZE > 2M

kmalloc_index() currently returns -1 if the PAGE_SIZE is larger than 2M
which seems to cause some concern since the callers do not check for -1.

Insert a BUG() and add a comment to the -1 explaining that the code
cannot be reached.

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 include/linux/slub_def.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6/include/linux/slub_def.h
===================================================================
--- linux-2.6.orig/include/linux/slub_def.h	2011-05-20 09:37:02.000000000 -0500
+++ linux-2.6/include/linux/slub_def.h	2011-05-20 09:39:07.000000000 -0500
@@ -179,7 +179,8 @@ static __always_inline int kmalloc_index
 	if (size <=   4 * 1024) return 12;
 /*
  * The following is only needed to support architectures with a larger page
- * size than 4k.
+ * size than 4k. We need to support 2 * PAGE_SIZE here. So for a 64k page
+ * size we would have to go up to 128k.
  */
 	if (size <=   8 * 1024) return 13;
 	if (size <=  16 * 1024) return 14;
@@ -190,7 +191,8 @@ static __always_inline int kmalloc_index
 	if (size <= 512 * 1024) return 19;
 	if (size <= 1024 * 1024) return 20;
 	if (size <=  2 * 1024 * 1024) return 21;
-	return -1;
+	BUG();
+	return -1; /* Will never be reached */

 /*
  * What we really wanted to do and cannot do because of compiler issues is:

Re: [PATCH] kernel buffer overflow kmalloc_slab() fix

[J Freyensee]

Thank you for the collaboration work on the fix.  I like it.

Jay

On Fri, 2011-05-20 at 09:42 -0500, Christoph Lameter wrote:
> Subject: slub: Deal with hyperthetical case of PAGE_SIZE > 2M
> 
> kmalloc_index() currently returns -1 if the PAGE_SIZE is larger than 2M
> which seems to cause some concern since the callers do not check for -1.
> 
> Insert a BUG() and add a comment to the -1 explaining that the code
> cannot be reached.
> 
> Signed-off-by: Christoph Lameter <cl@linux.com>
> 
> ---
>  include/linux/slub_def.h |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/include/linux/slub_def.h
> ===================================================================
> --- linux-2.6.orig/include/linux/slub_def.h	2011-05-20 09:37:02.000000000 -0500
> +++ linux-2.6/include/linux/slub_def.h	2011-05-20 09:39:07.000000000 -0500
> @@ -179,7 +179,8 @@ static __always_inline int kmalloc_index
>  	if (size <=   4 * 1024) return 12;
>  /*
>   * The following is only needed to support architectures with a larger page
> - * size than 4k.
> + * size than 4k. We need to support 2 * PAGE_SIZE here. So for a 64k page
> + * size we would have to go up to 128k.
>   */
>  	if (size <=   8 * 1024) return 13;
>  	if (size <=  16 * 1024) return 14;
> @@ -190,7 +191,8 @@ static __always_inline int kmalloc_index
>  	if (size <= 512 * 1024) return 19;
>  	if (size <= 1024 * 1024) return 20;
>  	if (size <=  2 * 1024 * 1024) return 21;
> -	return -1;
> +	BUG();
> +	return -1; /* Will never be reached */
> 
>  /*
>   * What we really wanted to do and cannot do because of compiler issues is: