diff mbox

net_sched: stack info leak in cbq_dump_wrr()

Message ID 20130729193651.GA12525@elgon.mountain
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Dan Carpenter July 29, 2013, 7:36 p.m. UTC 
opt.__reserved isn't cleared so we leak a byte of stack information.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

--
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

Comments

Joe Perches July 29, 2013, 7:44 p.m. UTC | #1 
On Mon, 2013-07-29 at 22:36 +0300, Dan Carpenter wrote:
> opt.__reserved isn't cleared so we leak a byte of stack information.
[]
> diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
[]
> @@ -1469,6 +1469,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
>  	opt.allot = cl->allot;
>  	opt.priority = cl->priority + 1;
>  	opt.cpriority = cl->cpriority + 1;
> +	opt.__reserved = 0;
>  	opt.weight = cl->weight;
>  	if (nla_put(skb, TCA_CBQ_WRROPT, sizeof(opt), &opt))
>  		goto nla_put_failure;

Alignment isn't guaranteed here so it'd
probably be better with a memset.


--
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
Dan Carpenter July 29, 2013, 8:01 p.m. UTC | #2 
On Mon, Jul 29, 2013 at 12:44:32PM -0700, Joe Perches wrote:
> On Mon, 2013-07-29 at 22:36 +0300, Dan Carpenter wrote:
> > opt.__reserved isn't cleared so we leak a byte of stack information.
> []
> > diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
> []
> > @@ -1469,6 +1469,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
> >  	opt.allot = cl->allot;
> >  	opt.priority = cl->priority + 1;
> >  	opt.cpriority = cl->cpriority + 1;
> > +	opt.__reserved = 0;
> >  	opt.weight = cl->weight;
> >  	if (nla_put(skb, TCA_CBQ_WRROPT, sizeof(opt), &opt))
> >  		goto nla_put_failure;
> 
> Alignment isn't guaranteed here so it'd
> probably be better with a memset.
> 

Hm...  Which arches would align it differently?

regards,
dan carpenter
--
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
Joe Perches July 29, 2013, 8:12 p.m. UTC | #3 
On Mon, 2013-07-29 at 23:01 +0300, Dan Carpenter wrote:
> On Mon, Jul 29, 2013 at 12:44:32PM -0700, Joe Perches wrote:
> > On Mon, 2013-07-29 at 22:36 +0300, Dan Carpenter wrote:
> > > opt.__reserved isn't cleared so we leak a byte of stack information.
> > []
> > > diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
> > []
> > > @@ -1469,6 +1469,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
> > >  	opt.allot = cl->allot;
> > >  	opt.priority = cl->priority + 1;
> > >  	opt.cpriority = cl->cpriority + 1;
> > > +	opt.__reserved = 0;
> > >  	opt.weight = cl->weight;
> > >  	if (nla_put(skb, TCA_CBQ_WRROPT, sizeof(opt), &opt))
> > >  		goto nla_put_failure;
> > 
> > Alignment isn't guaranteed here so it'd
> > probably be better with a memset.
> > 
> 
> Hm...  Which arches would align it differently?

Hey Dan.

None so far as I know, but what difference does it make
when it's a general correctness issue?


--
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
David Miller July 29, 2013, 9:17 p.m. UTC | #4 
From: Joe Perches <joe@perches.com>
Date: Mon, 29 Jul 2013 13:12:31 -0700

> On Mon, 2013-07-29 at 23:01 +0300, Dan Carpenter wrote:
>> On Mon, Jul 29, 2013 at 12:44:32PM -0700, Joe Perches wrote:
>> > On Mon, 2013-07-29 at 22:36 +0300, Dan Carpenter wrote:
>> > > opt.__reserved isn't cleared so we leak a byte of stack information.
>> > []
>> > > diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
>> > []
>> > > @@ -1469,6 +1469,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
>> > >  	opt.allot = cl->allot;
>> > >  	opt.priority = cl->priority + 1;
>> > >  	opt.cpriority = cl->cpriority + 1;
>> > > +	opt.__reserved = 0;
>> > >  	opt.weight = cl->weight;
>> > >  	if (nla_put(skb, TCA_CBQ_WRROPT, sizeof(opt), &opt))
>> > >  		goto nla_put_failure;
>> > 
>> > Alignment isn't guaranteed here so it'd
>> > probably be better with a memset.
>> > 
>> 
>> Hm...  Which arches would align it differently?
> 
> Hey Dan.
> 
> None so far as I know, but what difference does it make
> when it's a general correctness issue?

Should see if the compiler optimizes the spurious stores away,
and if not we can use an initializer.


--
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
Jiri Pirko July 29, 2013, 9:20 p.m. UTC | #5 
Mon, Jul 29, 2013 at 09:36:51PM CEST, dan.carpenter@oracle.com wrote:
>opt.__reserved isn't cleared so we leak a byte of stack information.
>
>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
>diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
>index 71a5688..6398a61 100644
>--- a/net/sched/sch_cbq.c
>+++ b/net/sched/sch_cbq.c
>@@ -1469,6 +1469,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
> 	opt.allot = cl->allot;
> 	opt.priority = cl->priority + 1;
> 	opt.cpriority = cl->cpriority + 1;
>+	opt.__reserved = 0;

There's probably better to zero whole opt at the beginning of the function.
--
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
Joe Perches July 29, 2013, 9:50 p.m. UTC | #6 
On Mon, 2013-07-29 at 14:17 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Mon, 29 Jul 2013 13:12:31 -0700
> 
> > On Mon, 2013-07-29 at 23:01 +0300, Dan Carpenter wrote:
> >> On Mon, Jul 29, 2013 at 12:44:32PM -0700, Joe Perches wrote:
> >> > On Mon, 2013-07-29 at 22:36 +0300, Dan Carpenter wrote:
> >> > > opt.__reserved isn't cleared so we leak a byte of stack information.
> >> > []
> >> > > diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
> >> > []
> >> > > @@ -1469,6 +1469,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
> >> > >          opt.allot = cl->allot;
> >> > >          opt.priority = cl->priority + 1;
> >> > >          opt.cpriority = cl->cpriority + 1;
> >> > > +        opt.__reserved = 0;
> >> > >          opt.weight = cl->weight;
> >> > >          if (nla_put(skb, TCA_CBQ_WRROPT, sizeof(opt), &opt))
> >> > >                  goto nla_put_failure;
> >> > 
> >> > Alignment isn't guaranteed here so it'd
> >> > probably be better with a memset.
> >> > 
> >> 
> >> Hm...  Which arches would align it differently?
> > 
> > Hey Dan.
> > 
> > None so far as I know, but what difference does it make
> > when it's a general correctness issue?
> 
> Should see if the compiler optimizes the spurious stores away,
> and if not we can use an initializer.

If the initializer is

	struct foo = {0};

then as far as I know, the compiler is free to 
not initialize any padding.

However, it looks like gcc 4.7 generates the same
code for this with or without the __aligned__ use.

(with gcc -O2 -S t.c)

$ cat t.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct foo {
	int a;
	char b __attribute__((__aligned__(256)));
	long c;
};

void init1(void)
{
	struct foo bar = {0};
	printf("%p\n", &bar);
}

void init2(void)
{
	struct foo bar;
	memset(&bar, 0, sizeof(bar));
	printf("%p\n", &bar);
}


--
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
Dan Carpenter July 30, 2013, 6:55 a.m. UTC | #7 
On Mon, Jul 29, 2013 at 01:12:31PM -0700, Joe Perches wrote:
> On Mon, 2013-07-29 at 23:01 +0300, Dan Carpenter wrote:
> > On Mon, Jul 29, 2013 at 12:44:32PM -0700, Joe Perches wrote:
> > > On Mon, 2013-07-29 at 22:36 +0300, Dan Carpenter wrote:
> > > > opt.__reserved isn't cleared so we leak a byte of stack information.
> > > []
> > > > diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
> > > []
> > > > @@ -1469,6 +1469,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
> > > >  	opt.allot = cl->allot;
> > > >  	opt.priority = cl->priority + 1;
> > > >  	opt.cpriority = cl->cpriority + 1;
> > > > +	opt.__reserved = 0;
> > > >  	opt.weight = cl->weight;
> > > >  	if (nla_put(skb, TCA_CBQ_WRROPT, sizeof(opt), &opt))
> > > >  		goto nla_put_failure;
> > > 
> > > Alignment isn't guaranteed here so it'd
> > > probably be better with a memset.
> > > 
> > 
> > Hm...  Which arches would align it differently?
> 
> Hey Dan.
> 
> None so far as I know, but what difference does it make
> when it's a general correctness issue?
> 

Because I would assume if these aren't aligned the same way we have
far more serious problems than just this one case.  It would change
the user space API and break network protocols.

regards,
dan carpenter
--
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
Joe Perches July 30, 2013, 7:12 a.m. UTC | #8 
On Tue, 2013-07-30 at 09:55 +0300, Dan Carpenter wrote:
> On Mon, Jul 29, 2013 at 01:12:31PM -0700, Joe Perches wrote:
> > On Mon, 2013-07-29 at 23:01 +0300, Dan Carpenter wrote:
> > > On Mon, Jul 29, 2013 at 12:44:32PM -0700, Joe Perches wrote:
> > > > On Mon, 2013-07-29 at 22:36 +0300, Dan Carpenter wrote:
> > > > > opt.__reserved isn't cleared so we leak a byte of stack information.
> > > > []
> > > > > diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
> > > > []
> > > > > @@ -1469,6 +1469,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
> > > > >  	opt.allot = cl->allot;
> > > > >  	opt.priority = cl->priority + 1;
> > > > >  	opt.cpriority = cl->cpriority + 1;
> > > > > +	opt.__reserved = 0;
> > > > >  	opt.weight = cl->weight;
> > > > >  	if (nla_put(skb, TCA_CBQ_WRROPT, sizeof(opt), &opt))
> > > > >  		goto nla_put_failure;
> > > > 
> > > > Alignment isn't guaranteed here so it'd
> > > > probably be better with a memset.
> > > > 
> > > 
> > > Hm...  Which arches would align it differently?
> > 
> > Hey Dan.
> > 
> > None so far as I know, but what difference does it make
> > when it's a general correctness issue?
> > 
> 
> Because I would assume if these aren't aligned the same way we have
> far more serious problems than just this one case.  It would change
> the user space API and break network protocols.

<shrug>

I didn't say it was necessary to be done here, I said it
was a correctness issue.  I still believe that's true.

The nla_put here is by structure, the struct is unpacked,
and it's local to the arch, not a particular endian type.

btw: to answer David's question, gcc 4.7 is smart enough
to elide resetting values when the struct is initialized
to 0 either with a memset or using {0}.

$ cat t.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct foo {
	int a;
	char b;
	long c;
};

void init1(void)
{
	struct foo bar = {0};
	bar.a = 1;
	bar.b = 2;
	bar.c = 3;
	printf("%p\n", &bar);
}

void init2(void)
{
	struct foo bar;
	memset(&bar, 0, sizeof(bar));
	bar.a = 1;
	bar.b = 2;
	bar.c = 3;
	printf("%p\n", &bar);

$ gcc -S -O2 t.c
$ cat t.s
	.file	"t.c"
	.section	.rodata.str1.1,"aMS",@progbits,1
.LC0:
	.string	"%p\n"
	.text
	.p2align 4,,15
	.globl	init1
	.type	init1, @function
init1:
.LFB60:
	.cfi_startproc
	subl	$44, %esp
	.cfi_def_cfa_offset 48
	leal	20(%esp), %eax
	movl	%eax, 8(%esp)
	movl	$.LC0, 4(%esp)
	movl	$1, (%esp)
	movl	$0, 24(%esp)
	movl	$1, 20(%esp)
	movb	$2, 24(%esp)
	movl	$3, 28(%esp)
	call	__printf_chk
	addl	$44, %esp
	.cfi_def_cfa_offset 4
	ret
	.cfi_endproc
.LFE60:
	.size	init1, .-init1
	.p2align 4,,15
	.globl	init2
	.type	init2, @function
init2:
.LFB61:
	.cfi_startproc
	subl	$44, %esp
	.cfi_def_cfa_offset 48
	leal	20(%esp), %eax
	movl	%eax, 8(%esp)
	movl	$.LC0, 4(%esp)
	movl	$1, (%esp)
	movl	$0, 24(%esp)
	movl	$1, 20(%esp)
	movb	$2, 24(%esp)
	movl	$3, 28(%esp)
	call	__printf_chk
	addl	$44, %esp
	.cfi_def_cfa_offset 4
	ret
	.cfi_endproc
.LFE61:
	.size	init2, .-init2
	.ident	"GCC: (Ubuntu/Linaro 4.7.3-1ubuntu1) 4.7.3"
	.section	.note.GNU-stack,"",@progbits


--
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
diff mbox

Patch

diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 71a5688..6398a61 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1469,6 +1469,7 @@  static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
 	opt.allot = cl->allot;
 	opt.priority = cl->priority + 1;
 	opt.cpriority = cl->cpriority + 1;
+	opt.__reserved = 0;
 	opt.weight = cl->weight;
 	if (nla_put(skb, TCA_CBQ_WRROPT, sizeof(opt), &opt))
 		goto nla_put_failure;