[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