[PATCH] pktgen: Remove a dangerous debug print.

[PATCH] pktgen: Remove a dangerous debug print.

[Nelson Elhage]

We were allocating an arbitrarily-large buffer on the stack, which would allow a
buggy or malicious userspace program to overflow the kernel stack.

Since the debug printk() was just printing exactly the text passed from
userspace, it's probably just as easy for anyone who might use it to augment (or
just strace(1)) the program writing to the pktgen file, so let's just not bother
trying to print the whole buffer.

Signed-off-by: Nelson Elhage <nelhage@ksplice.com>
---
 net/core/pktgen.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 10a1ea7..de8e0da 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -888,14 +888,9 @@ static ssize_t pktgen_if_write(struct file *file,
 
 	i += len;
 
-	if (debug) {
-		char tb[count + 1];
-		if (copy_from_user(tb, user_buffer, count))
-			return -EFAULT;
-		tb[count] = 0;
-		printk(KERN_DEBUG "pktgen: %s,%lu  buffer -:%s:-\n", name,
-		       (unsigned long)count, tb);
-	}
+	if (debug)
+		printk(KERN_DEBUG "pktgen: %s,%lu\n", name,
+		       (unsigned long)count);
 
 	if (!strcmp(name, "min_pkt_size")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-- 
1.7.1.31.g6297e

Re: [PATCH] pktgen: Remove a dangerous debug print.

[David Miller]

From: Nelson Elhage <nelhage@ksplice.com>
Date: Wed, 27 Oct 2010 15:13:08 -0400

> We were allocating an arbitrarily-large buffer on the stack, which would allow a
> buggy or malicious userspace program to overflow the kernel stack.
> 
> Since the debug printk() was just printing exactly the text passed from
> userspace, it's probably just as easy for anyone who might use it to augment (or
> just strace(1)) the program writing to the pktgen file, so let's just not bother
> trying to print the whole buffer.
> 
> Signed-off-by: Nelson Elhage <nelhage@ksplice.com>

Only root can write to the pktgen control file.

Also, the debug feature really is used by people's pktgen scripts, you
can't just turn it off.

Re: [PATCH] pktgen: Remove a dangerous debug print.

[Nelson Elhage]

How would you feel about limiting the debug print to at most, say, 512 or 1024
bytes? Even if it's only accessible to root by default, I don't a userspace
program should be able to accidentally corrupt the kernel stack by writing too
many bytes to a file in /proc.

- Nelson

On Wed, Oct 27, 2010 at 12:21:43PM -0700, David Miller wrote:
> From: Nelson Elhage <nelhage@ksplice.com>
> Date: Wed, 27 Oct 2010 15:13:08 -0400
> 
> > We were allocating an arbitrarily-large buffer on the stack, which would allow a
> > buggy or malicious userspace program to overflow the kernel stack.
> > 
> > Since the debug printk() was just printing exactly the text passed from
> > userspace, it's probably just as easy for anyone who might use it to augment (or
> > just strace(1)) the program writing to the pktgen file, so let's just not bother
> > trying to print the whole buffer.
> > 
> > Signed-off-by: Nelson Elhage <nelhage@ksplice.com>
> 
> Only root can write to the pktgen control file.
> 
> Also, the debug feature really is used by people's pktgen scripts, you
> can't just turn it off.

Re: [PATCH] pktgen: Remove a dangerous debug print.

[David Miller]

From: Nelson Elhage <nelhage@ksplice.com>
Date: Wed, 27 Oct 2010 15:28:08 -0400

> How would you feel about limiting the debug print to at most, say, 512 or 1024
> bytes? Even if it's only accessible to root by default, I don't a userspace
> program should be able to accidentally corrupt the kernel stack by writing too
> many bytes to a file in /proc.

Why not?  He can just as easily "cat whatever >/dev/kmem" or similar?
And I'm sure there are other proc files that can cause similar damage
such as the PCI device control files.

Re: [PATCH] pktgen: Remove a dangerous debug print.

[Eric Dumazet]

Le mercredi 27 octobre 2010 à 15:28 -0400, Nelson Elhage a écrit :
> How would you feel about limiting the debug print to at most, say, 512 or 1024
> bytes? Even if it's only accessible to root by default, I don't a userspace
> program should be able to accidentally corrupt the kernel stack by writing too
> many bytes to a file in /proc.

Arent /proc writes limited to PAGE_SIZE anyway ?

On x86 at least, you cannot corrupt kernel stack, since its bigger than
PAGE_SIZE.

I agree pktgen code is a bit ugly and needs a cleanup, but who
cares ? :)

Re: [PATCH] pktgen: Remove a dangerous debug print.

[Nelson Elhage]

I tested this and was able to oops both amd64 and i386 test machines with 8k
writes to the pktgen file. I haven't investigated whether that's because there's
no PAGE_SIZE limit, or because one page ends up being enough to cause a problem
on all my test machines.

- Nelson

On Wed, Oct 27, 2010 at 09:41:39PM +0200, Eric Dumazet wrote:
> Le mercredi 27 octobre 2010 à 15:28 -0400, Nelson Elhage a écrit :
> > How would you feel about limiting the debug print to at most, say, 512 or 1024
> > bytes? Even if it's only accessible to root by default, I don't a userspace
> > program should be able to accidentally corrupt the kernel stack by writing too
> > many bytes to a file in /proc.
> 
> Arent /proc writes limited to PAGE_SIZE anyway ?
> 
> On x86 at least, you cannot corrupt kernel stack, since its bigger than
> PAGE_SIZE.
> 
> I agree pktgen code is a bit ugly and needs a cleanup, but who
> cares ? :)
> 
> 
> 
> 

Re: [PATCH] pktgen: Remove a dangerous debug print.

[Ben Greear]

On 10/27/2010 12:13 PM, Nelson Elhage wrote:
> We were allocating an arbitrarily-large buffer on the stack, which would allow a
> buggy or malicious userspace program to overflow the kernel stack.
>
> Since the debug printk() was just printing exactly the text passed from
> userspace, it's probably just as easy for anyone who might use it to augment (or
> just strace(1)) the program writing to the pktgen file, so let's just not bother
> trying to print the whole buffer.

Maybe just allocate that buffer on the heap instead of stack?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

[patch] fix stack overflow in pktgen_if_write()

[Dan Carpenter]

Nelson Elhage says he was able to oops both amd64 and i386 test 
machines with 8k writes to the pktgen file.  Let's just allocate the
buffer on the heap instead of on the stack.

This can only be triggered by root so there are no security issues here.

Reported-by: Nelson Elhage <nelhage@ksplice.com>
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
I saw this on twitter.  Hi Nelson, could you test this?

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 2c0df0f..b5d3c70 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -887,12 +887,14 @@ static ssize_t pktgen_if_write(struct file *file,
 	i += len;
 
 	if (debug) {
-		char tb[count + 1];
-		if (copy_from_user(tb, user_buffer, count))
-			return -EFAULT;
-		tb[count] = 0;
+		char *tb;
+
+		tb = strndup_user(user_buffer, count + 1);
+		if (IS_ERR(tb))
+			return PTR_ERR(tb);
 		printk(KERN_DEBUG "pktgen: %s,%lu  buffer -:%s:-\n", name,
 		       (unsigned long)count, tb);
+		kfree(tb);
 	}
 
 	if (!strcmp(name, "min_pkt_size")) {

Re: [patch] fix stack overflow in pktgen_if_write()

[Dan Carpenter]

On Thu, Oct 28, 2010 at 12:12:35AM +0200, Dan Carpenter wrote:
> -		char tb[count + 1];
> -		if (copy_from_user(tb, user_buffer, count))
> -			return -EFAULT;
> -		tb[count] = 0;
> +		char *tb;
> +
> +		tb = strndup_user(user_buffer, count + 1);

Crap...  This should be memdup_user().

Sorry about that.  I'll send v2.

regards,
dan carpenter

> +		if (IS_ERR(tb))
> +			return PTR_ERR(tb);
>  		printk(KERN_DEBUG "pktgen: %s,%lu  buffer -:%s:-\n", name,
>  		       (unsigned long)count, tb);
> +		kfree(tb);
>  	}
>  
>  	if (!strcmp(name, "min_pkt_size")) {

[patch v2] fix stack overflow in pktgen_if_write()

[Dan Carpenter]

Nelson Elhage says he was able to oops both amd64 and i386 test 
machines with 8k writes to the pktgen file.  Let's just allocate the
buffer on the heap instead of on the stack.

This can only be triggered by root so there are no security issues here.

Reported-by: Nelson Elhage <nelhage@ksplice.com>
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
I saw this on twitter.  Hi Nelson, could you test this?

V2:  strndup_user() => memdup_user()  

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 2c0df0f..b5d3c70 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -887,12 +887,14 @@ static ssize_t pktgen_if_write(struct file *file,
 	i += len;
 
 	if (debug) {
-		char tb[count + 1];
-		if (copy_from_user(tb, user_buffer, count))
-			return -EFAULT;
-		tb[count] = 0;
+		char *tb;
+
+		tb = memdup_user(user_buffer, count + 1);
+		if (IS_ERR(tb))
+			return PTR_ERR(tb);
 		printk(KERN_DEBUG "pktgen: %s,%lu  buffer -:%s:-\n", name,
 		       (unsigned long)count, tb);
+		kfree(tb);
 	}
 
 	if (!strcmp(name, "min_pkt_size")) {

Re: [patch v2] fix stack overflow in pktgen_if_write()

[Nelson Elhage]

You want to add a trailing NUL, or else printk will read off the end of the
buffer.

Also, by memdup()ing count + 1 bytes, you're technically reading one more byte
than userspace asked for, which could in principle lead to a spurious EFAULT.

- Nelson

On Thu, Oct 28, 2010 at 12:43:02AM +0200, Dan Carpenter wrote:
> Nelson Elhage says he was able to oops both amd64 and i386 test 
> machines with 8k writes to the pktgen file.  Let's just allocate the
> buffer on the heap instead of on the stack.
> 
> This can only be triggered by root so there are no security issues here.
> 
> Reported-by: Nelson Elhage <nelhage@ksplice.com>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> I saw this on twitter.  Hi Nelson, could you test this?
> 
> V2:  strndup_user() => memdup_user()  
> 
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 2c0df0f..b5d3c70 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -887,12 +887,14 @@ static ssize_t pktgen_if_write(struct file *file,
>  	i += len;
>  
>  	if (debug) {
> -		char tb[count + 1];
> -		if (copy_from_user(tb, user_buffer, count))
> -			return -EFAULT;
> -		tb[count] = 0;
> +		char *tb;
> +
> +		tb = memdup_user(user_buffer, count + 1);
> +		if (IS_ERR(tb))
> +			return PTR_ERR(tb);
>  		printk(KERN_DEBUG "pktgen: %s,%lu  buffer -:%s:-\n", name,
>  		       (unsigned long)count, tb);
> +		kfree(tb);
>  	}
>  
>  	if (!strcmp(name, "min_pkt_size")) {
> 

Re: [patch v2] fix stack overflow in pktgen_if_write()

[Dan Carpenter]

On Wed, Oct 27, 2010 at 07:06:57PM -0400, Nelson Elhage wrote:
> You want to add a trailing NUL, or else printk will read off the end of the
> buffer.
> 
> Also, by memdup()ing count + 1 bytes, you're technically reading one more byte
> than userspace asked for, which could in principle lead to a spurious EFAULT.
> 

That's a lot of bugs per line.  :(  I'm eating humble pie today...

regards,
dan carpenter

[patch v3] fix stack overflow in pktgen_if_write()

[Dan Carpenter]

Nelson Elhage says he was able to oops both amd64 and i386 test 
machines with 8k writes to the pktgen file.  Let's just allocate the
buffer on the heap instead of on the stack.

This can only be triggered by root so there are no security issues here.

Reported-by: Nelson Elhage <nelhage@ksplice.com>
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
v3:  just use kmalloc()

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 2c0df0f..c8d3620 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -887,12 +887,17 @@ static ssize_t pktgen_if_write(struct file *file,
 	i += len;
 
 	if (debug) {
-		char tb[count + 1];
+		char *tb;
+
+		tb = kmalloc(count + 1, GFP_KERNEL);
+		if (!tb)
+			return -ENOMEM;
 		if (copy_from_user(tb, user_buffer, count))
 			return -EFAULT;
 		tb[count] = 0;
 		printk(KERN_DEBUG "pktgen: %s,%lu  buffer -:%s:-\n", name,
 		       (unsigned long)count, tb);
+		kfree(tb);
 	}
 
 	if (!strcmp(name, "min_pkt_size")) {

Re: [patch v3] fix stack overflow in pktgen_if_write()

[Nelson Elhage]

You've got a leak if copy_user fails.

While testing this, I realized that printk() won't print more than 1k in a
single call, anyways, so I've sent along a patch that just copies up to 1k onto
the stack, which should prevent the overflow without changing behavior or
needing a heap allocation.

- Nelson

On Thu, Oct 28, 2010 at 08:05:29AM +0200, Dan Carpenter wrote:
> Nelson Elhage says he was able to oops both amd64 and i386 test 
> machines with 8k writes to the pktgen file.  Let's just allocate the
> buffer on the heap instead of on the stack.
> 
> This can only be triggered by root so there are no security issues here.
> 
> Reported-by: Nelson Elhage <nelhage@ksplice.com>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> v3:  just use kmalloc()
> 
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 2c0df0f..c8d3620 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -887,12 +887,17 @@ static ssize_t pktgen_if_write(struct file *file,
>  	i += len;
>  
>  	if (debug) {
> -		char tb[count + 1];
> +		char *tb;
> +
> +		tb = kmalloc(count + 1, GFP_KERNEL);
> +		if (!tb)
> +			return -ENOMEM;
>  		if (copy_from_user(tb, user_buffer, count))
>  			return -EFAULT;
>  		tb[count] = 0;
>  		printk(KERN_DEBUG "pktgen: %s,%lu  buffer -:%s:-\n", name,
>  		       (unsigned long)count, tb);
> +		kfree(tb);
>  	}
>  
>  	if (!strcmp(name, "min_pkt_size")) {

Re: [patch v3] fix stack overflow in pktgen_if_write()

[Dan Carpenter]

On Thu, Oct 28, 2010 at 11:22:22AM -0400, Nelson Elhage wrote:
> You've got a leak if copy_user fails.
> 

My QC scripts should have caught that, but they didn't...  I'll figure
it out.  It shouldn't happen again.

> While testing this, I realized that printk() won't print more than 1k in a
> single call, anyways, so I've sent along a patch that just copies up to 1k onto
> the stack, which should prevent the overflow without changing behavior or
> needing a heap allocation.
> 

Ok.  Good to hear.  Sorry I wasted people's time.

regards,
dan carpenter

Re: [patch v3] fix stack overflow in pktgen_if_write()

[Nelson Elhage]

On Thu, Oct 28, 2010 at 06:28:25PM +0200, Dan Carpenter wrote:
> On Thu, Oct 28, 2010 at 11:22:22AM -0400, Nelson Elhage wrote:
> > You've got a leak if copy_user fails.
> > 
> 
> My QC scripts should have caught that, but they didn't...  I'll figure
> it out.  It shouldn't happen again.
> 
> > While testing this, I realized that printk() won't print more than 1k in a
> > single call, anyways, so I've sent along a patch that just copies up to 1k onto
> > the stack, which should prevent the overflow without changing behavior or
> > needing a heap allocation.
> > 
> 
> Ok.  Good to hear.  Sorry I wasted people's time.

No worries. I appreciate you jumping in to help, even if it looks like the
approach wasn't needed in the end.

- Nelson

> 
> regards,
> dan carpenter
> 
> 

Re: [patch v3] fix stack overflow in pktgen_if_write()

[Andi Kleen]

Dan Carpenter <error27@gmail.com> writes:

> Reported-by: Nelson Elhage <nelhage@ksplice.com>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> v3:  just use kmalloc()
>
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 2c0df0f..c8d3620 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -887,12 +887,17 @@ static ssize_t pktgen_if_write(struct file *file,
>  	i += len;
>  
>  	if (debug) {
> -		char tb[count + 1];
> +		char *tb;
> +
> +		tb = kmalloc(count + 1, GFP_KERNEL);


This is still trivially exploitable (for root) -- think what happens
when count is near ULONG_MAX

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [patch v3] fix stack overflow in pktgen_if_write()

[Dan Carpenter]

> > @@ -887,12 +887,17 @@ static ssize_t pktgen_if_write(struct file *file,
> >  	i += len;
> >  
> >  	if (debug) {
> > -		char tb[count + 1];
> > +		char *tb;
> > +
> > +		tb = kmalloc(count + 1, GFP_KERNEL);
> 
> 
> This is still trivially exploitable (for root) -- think what happens
> when count is near ULONG_MAX
> 

In vfs_write() we call rw_verify_area() which caps count at INT_MAX or
LONG_MAX.

        if (unlikely((ssize_t) count < 0))
                return retval;

So I get lucky this time...  ;)

regards,
dan carpenter

[PATCH] pktgen: Limit how much data we copy onto the stack.

[Nelson Elhage]

A program that accidentally writes too much data to the pktgen file can overflow
the kernel stack and oops the machine. This is only triggerable by root, so
there's no security issue, but it's still an unfortunate bug.

printk() won't print more than 1024 bytes in a single call, anyways, so let's
just never copy more than that much data. We're on a fairly shallow stack, so
that should be safe even with CONFIG_4KSTACKS.

Signed-off-by: Nelson Elhage <nelhage@ksplice.com>
---
While testing Dan's patch, I realized the printk() limitation, so I don't think
there's any reason to need a kmalloc() at all. I've tested this on a
CONFIG_4KSTACKS kernel, and copying 1k works fine.

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 10a1ea7..d6667cf 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -889,10 +889,11 @@ static ssize_t pktgen_if_write(struct file *file,
 	i += len;
 
 	if (debug) {
-		char tb[count + 1];
-		if (copy_from_user(tb, user_buffer, count))
+		size_t copy = min(count, 1023);
+		char tb[copy + 1];
+		if (copy_from_user(tb, user_buffer, copy))
 			return -EFAULT;
-		tb[count] = 0;
+		tb[copy] = 0;
 		printk(KERN_DEBUG "pktgen: %s,%lu  buffer -:%s:-\n", name,
 		       (unsigned long)count, tb);
 	}
-- 
1.7.1.31.g6297e

Re: [PATCH] pktgen: Limit how much data we copy onto the stack.

[David Miller]

From: Nelson Elhage <nelhage@ksplice.com>
Date: Thu, 28 Oct 2010 11:20:42 -0400

> A program that accidentally writes too much data to the pktgen file can overflow
> the kernel stack and oops the machine. This is only triggerable by root, so
> there's no security issue, but it's still an unfortunate bug.
> 
> printk() won't print more than 1024 bytes in a single call, anyways, so let's
> just never copy more than that much data. We're on a fairly shallow stack, so
> that should be safe even with CONFIG_4KSTACKS.
> 
> Signed-off-by: Nelson Elhage <nelhage@ksplice.com>

Applied, thanks a lot.