[PATCH] kernel buffer overflow kmalloc_slab() fix

[PATCH] kernel buffer overflow kmalloc_slab() fix

[james_p_freyensee]

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
buffer overflow, and security bug issue (not sure how
likely this can happen, but this case does exist in the code).
This adds a check for -1 and completely prevents this from happening.

Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>
---
 include/linux/slub_def.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index 45ca123..558fa99 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -211,7 +211,7 @@ static __always_inline struct kmem_cache *kmalloc_slab(size_t size)
 {
 	int index = kmalloc_index(size);
 
-	if (index == 0)
+	if ((index == 0) || (index == -1))
 		return NULL;
 
 	return kmalloc_caches[index];
-- 
1.7.2.3

--
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 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.


--
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

[J Freyensee]

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.
> 
> 

Okay.  I thought it would still be good to check for -1 anyways, even if
machines today cannot go above 2M page sizes.  I would think it would be
better for software code to always make sure a case that this could
never happen instead of relying on whatever physical hardware limits the
linux kernel could be running on on today's machines or future machines,
because technology has shown limits can change.  I would think
regardless what this code runs on, this is still a software flaw that
can be considered not a good thing to allow lying around in software
code that can easily be fixed.

But again I'm fine with whatever is decided. 


--
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 Thu, 19 May 2011, J Freyensee 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.
> >
> >
>
> Okay.  I thought it would still be good to check for -1 anyways, even if
> machines today cannot go above 2M page sizes.  I would think it would be
> better for software code to always make sure a case that this could
> never happen instead of relying on whatever physical hardware limits the
> linux kernel could be running on on today's machines or future machines,
> because technology has shown limits can change.  I would think
> regardless what this code runs on, this is still a software flaw that
> can be considered not a good thing to allow lying around in software
> code that can easily be fixed.

This is basically macro style code that is mostly folded at compile time
and we have to obey certain restrictions to convince the compiler to
properly do that. Took us a long time to get that right.

Not sure what to do instead of returning -1 in kmalloc_slab. I'd be glad
if you could get the compiler to simply fail in kmalloc_slab() if a value
larger than 4M is passed to it. But please make sure that all versions of
GCC do proper constant folding and that the function also works if
constant folding is not possible for some reason. Consider esoteric arches
and compiler version also.

--
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

[J Freyensee]

On Thu, 2011-05-19 at 16:24 -0500, Christoph Lameter wrote:
> On Thu, 19 May 2011, J Freyensee 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.
> > >
> > >
> >
> > Okay.  I thought it would still be good to check for -1 anyways, even if
> > machines today cannot go above 2M page sizes.  I would think it would be
> > better for software code to always make sure a case that this could
> > never happen instead of relying on whatever physical hardware limits the
> > linux kernel could be running on on today's machines or future machines,
> > because technology has shown limits can change.  I would think
> > regardless what this code runs on, this is still a software flaw that
> > can be considered not a good thing to allow lying around in software
> > code that can easily be fixed.
> 
> This is basically macro style code that is mostly folded at compile time
> and we have to obey certain restrictions to convince the compiler to
> properly do that. Took us a long time to get that right.
> 
> Not sure what to do instead of returning -1 in kmalloc_slab. 

I think returning -1 is fine; I just think code using the function
should be checking for it and protect itself for errors in kernel space.


--
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

[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

[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


--
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 Thu, 19 May 2011, J Freyensee wrote:

> > Not sure what to do instead of returning -1 in kmalloc_slab.
>
> I think returning -1 is fine; I just think code using the function
> should be checking for it and protect itself for errors in kernel space.

The function never returns -1 as I explained before.


--
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.


--
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

[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:

--
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

[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:

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:


--
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>