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
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 [email protected]
> 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
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
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
[ 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 <[email protected]>
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: [email protected]
Signed-off-by: Michael Buesch <[email protected]>
Acked-by: Jack Steiner <[email protected]>
--- 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;
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))
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;
}
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
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.
> 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.
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
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
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
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 <[email protected]>
--- jack