| Message ID | 1288206788-21063-1-git-send-email-nelhage@ksplice.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ? :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ? :) > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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);
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(-)