CVE-2009-2584

CVE-2009-2584

[Michael Gilbert]

Hi,

CVE-2009-2584 [0],[1] has been disclosed for quite a while now (with
existing exploit code by Brad Spengler [2]).  A patch has also been
available for the same amount of time [3], but as of 2.6.32-rc6 it is 
still not applied.  Did this slip through the cracks?  Thanks upfront
for any info on the matter.

Best wishes,
Mike

[0] http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-2584
[1] http://xorl.wordpress.com/2009/07/21/linux-kernel-sgi-gru-driver-off-by-one-overwrite/
[2] http://grsecurity.net/~spender/exploit_demo.c
[3] http://lkml.org/lkml/2009/7/20/348

Re: CVE-2009-2584

[Justin P. Mattock]

Michael Gilbert wrote:
> Hi,
>
> CVE-2009-2584 [0],[1] has been disclosed for quite a while now (with
> existing exploit code by Brad Spengler [2]).  A patch has also been
> available for the same amount of time [3], but as of 2.6.32-rc6 it is
> still not applied.  Did this slip through the cracks?  Thanks upfront
> for any info on the matter.
>
> Best wishes,
> Mike
>
> [0] http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-2584
> [1] http://xorl.wordpress.com/2009/07/21/linux-kernel-sgi-gru-driver-off-by-one-overwrite/
> [2] http://grsecurity.net/~spender/exploit_demo.c
> [3] http://lkml.org/lkml/2009/7/20/348
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>    
just read something today which might
be similar/same as what you might
be referring too.
http://www.theregister.co.uk/2009/11/03/linux_kernel_vulnerability/

Justin P. Mattock

Re: CVE-2009-2584

[Michael Gilbert]

On Wed, 04 Nov 2009 14:08:41 -0800, Justin P. Mattock wrote:
> just read something today which might
> be similar/same as what you might
> be referring too.
> http://www.theregister.co.uk/2009/11/03/linux_kernel_vulnerability/

Hi,

Thank you very much for the quick response, but that link refers to
CVE-2009-3547 (not CVE-2009-2584), which is a different issue
altogether.

Best wishes,
Mike

Re: CVE-2009-2584

[Justin P. Mattock]

Michael Gilbert wrote:
> On Wed, 04 Nov 2009 14:08:41 -0800, Justin P. Mattock wrote:
>    
>> just read something today which might
>> be similar/same as what you might
>> be referring too.
>> http://www.theregister.co.uk/2009/11/03/linux_kernel_vulnerability/
>>      
>
> Hi,
>
> Thank you very much for the quick response, but that link refers to
> CVE-2009-3547 (not CVE-2009-2584), which is a different issue
> altogether.
>
> Best wishes,
> Mike
>
>    
alright..
wasn't sure or not.
hopefully somebody gives some info
on this(I don't like seeing these things).

Justin P. Mattock

Re: CVE-2009-2584

[Jiri Kosina]

[ adding some more CCs and including patch below for completness, 
  obviously it got lost in space ]

On Wed, 4 Nov 2009, Michael Gilbert wrote:

> CVE-2009-2584 [0],[1] has been disclosed for quite a while now (with 
> existing exploit code by Brad Spengler [2]).  A patch has also been 
> available for the same amount of time [3], but as of 2.6.32-rc6 it is 
> still not applied.  Did this slip through the cracks?  Thanks upfront 
> for any info on the matter.
[ ... ]
> [0] http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-2584
> [1] http://xorl.wordpress.com/2009/07/21/linux-kernel-sgi-gru-driver-off-by-one-overwrite/
> [2] http://grsecurity.net/~spender/exploit_demo.c
> [3] http://lkml.org/lkml/2009/7/20/348



From: Michael Buesch <mb@bu3sch.de>
Subject: sgi-gru: Fix kernel stack buffer overrun

This patch fixes a kernel stack buffer overrun in the sgi-gru procfs 
interface implementation. The "count" parameter to options_write() is user 
controlled. So this bug can be used to write '\0' bytes to almost 
arbitrary places on the kernel stack.

Cc: stable@kernel.org
Signed-off-by: Michael Buesch <mb@bu3sch.de>
Acked-by: Jack Steiner <steiner@sgi.com>

--- linux-2.6.orig/drivers/misc/sgi-gru/gruprocfs.c
+++ linux-2.6/drivers/misc/sgi-gru/gruprocfs.c
@@ -157,23 +157,23 @@ static int options_show(struct seq_file 
 	seq_printf(s, "0x%lx\n", gru_options);
 	return 0;
 }
 
 static ssize_t options_write(struct file *file, const char __user *userbuf,
 			     size_t count, loff_t *data)
 {
 	unsigned long val;
 	char buf[80];
 
+	memset(buf, 0, sizeof(buf));
 	if (strncpy_from_user(buf, userbuf, sizeof(buf) - 1) < 0)
 		return -EFAULT;
-	buf[count - 1] = '\0';
 	if (!strict_strtoul(buf, 10, &val))
 		gru_options = val;
 
 	return count;
 }
 
 static int cch_seq_show(struct seq_file *file, void *data)
 {
 	long gid = *(long *)data;
 	int i;

  

Re: CVE-2009-2584

[Linus Torvalds]

On Thu, 5 Nov 2009, Jiri Kosina wrote:
>  
> +	memset(buf, 0, sizeof(buf));
>  	if (strncpy_from_user(buf, userbuf, sizeof(buf) - 1) < 0)
>  		return -EFAULT;
> -	buf[count - 1] = '\0';

Why isn't this just

	buf[sizeof(buf) - 1] = 0;

which was almost certainly the _intention_ of the code in the first 
place, since 'count' has absolutely nothing to do with the copy from user 
space as it is written.

I'm also convinced we do _not_ want 'strncpy' there, since we do have the 
size of the write. Doing a strncpy_from_user() is actually _wrong_ if the 
user buffer itself is smaller than the kernel buffer and isn't 
NUL-terminated.

So that strncpy crap needs to go. It's wrong.

In other words it would all look a whole lot more natural (and correct) 
like the appended patch.

Untested, of course. And since almost nobody has the hardware, so it's not 
like it's ever likely to _be_ tested.

		Linus

---
 drivers/misc/sgi-gru/gruprocfs.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/misc/sgi-gru/gruprocfs.c b/drivers/misc/sgi-gru/gruprocfs.c
index ccd4408..c0e17b0 100644
--- a/drivers/misc/sgi-gru/gruprocfs.c
+++ b/drivers/misc/sgi-gru/gruprocfs.c
@@ -164,7 +164,9 @@ static ssize_t options_write(struct file *file, const char __user *userbuf,
 	unsigned long val;
 	char buf[80];
 
-	if (strncpy_from_user(buf, userbuf, sizeof(buf) - 1) < 0)
+	if (count >= sizeof(buf))
+		count = sizeof(buf)-1;
+	if (copy_from_user(buf, userbuf, count))
 		return -EFAULT;
 	buf[count - 1] = '\0';
 	if (!strict_strtoul(buf, 10, &val))

Re: CVE-2009-2584

[Linus Torvalds]

On Thu, 5 Nov 2009, Linus Torvalds wrote:
> 
> Untested, of course. And since almost nobody has the hardware, so it's not 
> like it's ever likely to _be_ tested.
> 
> 		Linus
> 
> ---
>  drivers/misc/sgi-gru/gruprocfs.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/misc/sgi-gru/gruprocfs.c b/drivers/misc/sgi-gru/gruprocfs.c
> index ccd4408..c0e17b0 100644
> --- a/drivers/misc/sgi-gru/gruprocfs.c
> +++ b/drivers/misc/sgi-gru/gruprocfs.c
> @@ -164,7 +164,9 @@ static ssize_t options_write(struct file *file, const char __user *userbuf,
>  	unsigned long val;
>  	char buf[80];
>  
> -	if (strncpy_from_user(buf, userbuf, sizeof(buf) - 1) < 0)
> +	if (count >= sizeof(buf))
> +		count = sizeof(buf)-1;
> +	if (copy_from_user(buf, userbuf, count))
>  		return -EFAULT;
>  	buf[count - 1] = '\0';

And it's wrong. That 'count-1' was always wrong. It seems to _depend_ on 
people doing things like

	echo number > /proc/..

and the 'echo' adding a '\n' at the end, and then 'buf[count-1] = 0' 
clearing away the '\n'.

Which is pointless, since '\n' at the end is the _one_ thing 
strict_strtoul() accepts. And it's wrong, because it means that

	echo -n number > /proc..

fails.

In other words, the whole function is utter sh*t as far as I can tell. 
There's basically not a single correct line in the whole thing.

Even the 'strict_strtoul()' line is absolute and utter crap, since it 
forces decimal numbers, but 'options_show()' will then show it as a hex 
number (and a hex number is natural, since it's a set of flags). It also 
doesn't actually return an error if you write some invalid value, and it's 
totally pointless to have that 'val' temporary, since the strict_strtoul 
function only writes the result if it is successful _anyway_.

The size of the buffer is also insane. Since the _only_ thing we will ever 
accept is a number, there's no point in allowing all that many characters. 
And silently ignoring the extra characters kind of makes the whole 
'strict' part of 'strict_strtoul()' pointless.

So here's a second try. I guess the 'return count/-EFAULT' lines were
actually correct after all. So it wasn't _all_ buggy or insane.

Still entirely untested.

		Linus

---
 drivers/misc/sgi-gru/gruprocfs.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/sgi-gru/gruprocfs.c b/drivers/misc/sgi-gru/gruprocfs.c
index ccd4408..762f179 100644
--- a/drivers/misc/sgi-gru/gruprocfs.c
+++ b/drivers/misc/sgi-gru/gruprocfs.c
@@ -161,14 +161,15 @@ static int options_show(struct seq_file *s, void *p)
 static ssize_t options_write(struct file *file, const char __user *userbuf,
 			     size_t count, loff_t *data)
 {
-	unsigned long val;
-	char buf[80];
+	char buf[16];
 
-	if (strncpy_from_user(buf, userbuf, sizeof(buf) - 1) < 0)
+	if (count >= sizeof(buf))
+		return -EINVAL;
+	if (copy_from_user(buf, userbuf, count))
 		return -EFAULT;
-	buf[count - 1] = '\0';
-	if (!strict_strtoul(buf, 10, &val))
-		gru_options = val;
+	buf[count] = '\0';
+	if (strict_strtoul(buf, 0, &gru_options))
+		return -EINVAL;
 
 	return count;
 }

Re: CVE-2009-2584

[Linus Torvalds]

On Thu, 5 Nov 2009, Linus Torvalds wrote:
>  {
> -	unsigned long val;
> -	char buf[80];
> +	char buf[16];

On third thought, this was too aggressive.

Using "0x%16ul" as a format on 64-bit machines is reasonable, so 19 bytes 
of buffer is not insane (with the terminating NUL). Of course, it never 
used to accept hex numbers, so it's not like it would have worked before, 
but the point is that I cut down the buffer unnecessarily strictly.

Can anybody see anything else wrong in that suggested fix?

		Linus

Re: CVE-2009-2584

[Michael Buesch]

On Thursday 05 November 2009 18:38:21 Linus Torvalds wrote:
> @@ -161,14 +161,15 @@ static int options_show(struct seq_file *s, void *p)
>  static ssize_t options_write(struct file *file, const char __user *userbuf,
>  			     size_t count, loff_t *data)
>  {
> -	unsigned long val;
> -	char buf[80];
> +	char buf[16];
>  
> -	if (strncpy_from_user(buf, userbuf, sizeof(buf) - 1) < 0)
> +	if (count >= sizeof(buf))
> +		return -EINVAL;
> +	if (copy_from_user(buf, userbuf, count))
>  		return -EFAULT;
> -	buf[count - 1] = '\0';
> -	if (!strict_strtoul(buf, 10, &val))
> -		gru_options = val;
> +	buf[count] = '\0';
> +	if (strict_strtoul(buf, 0, &gru_options))
> +		return -EINVAL;
>  
>  	return count;
>  }
> 
> 

Looks OK to me. I can't test it however, as I don't own the hardware.

-- 
Greetings, Michael.

Re: CVE-2009-2584

[Roland Dreier]

 > So here's a second try. I guess the 'return count/-EFAULT' lines were
 > actually correct after all. So it wasn't _all_ buggy or insane.

The blank lines seem fine too.

 - R.

Re: CVE-2009-2584

[Jack Steiner]

On Thu, Nov 05, 2009 at 06:47:33PM +0100, Michael Buesch wrote:
> On Thursday 05 November 2009 18:38:21 Linus Torvalds wrote:
> > @@ -161,14 +161,15 @@ static int options_show(struct seq_file *s, void *p)
> >  static ssize_t options_write(struct file *file, const char __user *userbuf,
> >  			     size_t count, loff_t *data)
> >  {
> > -	unsigned long val;
> > -	char buf[80];
> > +	char buf[16];
> >  
> > -	if (strncpy_from_user(buf, userbuf, sizeof(buf) - 1) < 0)
> > +	if (count >= sizeof(buf))
> > +		return -EINVAL;
> > +	if (copy_from_user(buf, userbuf, count))
> >  		return -EFAULT;
> > -	buf[count - 1] = '\0';
> > -	if (!strict_strtoul(buf, 10, &val))
> > -		gru_options = val;
> > +	buf[count] = '\0';
> > +	if (strict_strtoul(buf, 0, &gru_options))
> > +		return -EINVAL;
> >  
> >  	return count;
> >  }
> > 
> > 
> 
> Looks OK to me. I can't test it however, as I don't own the hardware.

"buf" should be larger than 16. The string could be "0x" + 16 characters.

I'll verify the the rest.


We have the hardware :-)


--- jack

Re: CVE-2009-2584

[Linus Torvalds]

On Thu, 5 Nov 2009, Michael Buesch wrote:
> 
> Looks OK to me. I can't test it however, as I don't own the hardware.

Heh. Even the people who wanted to write exploit examples had the same 
small problem. I doubt it really matters for anybody.

I'm committing it, just because I don't think it can really be any worse 
than the status quo. But I'll happily take further patches, especially 
from anybody who actually has access to the hardware.

		Linus

Re: CVE-2009-2584

[Linus Torvalds]

On Thu, 5 Nov 2009, Jack Steiner wrote:
> 
> "buf" should be larger than 16. The string could be "0x" + 16 characters.

I have 'char buf[20];' in my tree now.

> I'll verify the the rest.
> 
> We have the hardware :-)

Thanks. I've committed it locally, but if I get a tested-by or an ack (or 
a fix) soon enough, I'll update the commit before I push it out.

		Linus

Re: CVE-2009-2584

[Jack Steiner]

On Thu, Nov 05, 2009 at 10:22:43AM -0800, Linus Torvalds wrote:
> 
> 
> On Thu, 5 Nov 2009, Jack Steiner wrote:
> > 
> > "buf" should be larger than 16. The string could be "0x" + 16 characters.
> 
> I have 'char buf[20];' in my tree now.
> 
> > I'll verify the the rest.
> > 
> > We have the hardware :-)
> 
> Thanks. I've committed it locally, but if I get a tested-by or an ack (or 
> a fix) soon enough, I'll update the commit before I push it out.

Tested on real hardware.

Acked-by: Jack Steiner <steiner@sgi.com>


--- jack