* [patch] net_sched: stack info leak in cbq_dump_wrr()
@ 2013-07-29 19:36 Dan Carpenter
2013-07-29 19:44 ` Joe Perches
2013-07-29 21:20 ` Jiri Pirko
0 siblings, 2 replies; 11+ messages in thread
From: Dan Carpenter @ 2013-07-29 19:36 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: David S. Miller, netdev, kernel-janitors
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;
opt.weight = cl->weight;
if (nla_put(skb, TCA_CBQ_WRROPT, sizeof(opt), &opt))
goto nla_put_failure;
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-29 19:36 [patch] net_sched: stack info leak in cbq_dump_wrr() Dan Carpenter
@ 2013-07-29 19:44 ` Joe Perches
2013-07-29 20:01 ` Dan Carpenter
2013-07-29 21:20 ` Jiri Pirko
1 sibling, 1 reply; 11+ messages in thread
From: Joe Perches @ 2013-07-29 19:44 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jamal Hadi Salim, David S. Miller, netdev, kernel-janitors
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-29 19:44 ` Joe Perches
@ 2013-07-29 20:01 ` Dan Carpenter
2013-07-29 20:12 ` Joe Perches
0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2013-07-29 20:01 UTC (permalink / raw)
To: Joe Perches; +Cc: Jamal Hadi Salim, David S. Miller, netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-29 20:01 ` Dan Carpenter
@ 2013-07-29 20:12 ` Joe Perches
2013-07-29 21:17 ` David Miller
2013-07-30 6:55 ` Dan Carpenter
0 siblings, 2 replies; 11+ messages in thread
From: Joe Perches @ 2013-07-29 20:12 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jamal Hadi Salim, David S. Miller, netdev, kernel-janitors
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?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-29 20:12 ` Joe Perches
@ 2013-07-29 21:17 ` David Miller
2013-07-29 21:50 ` Joe Perches
2013-07-30 6:55 ` Dan Carpenter
1 sibling, 1 reply; 11+ messages in thread
From: David Miller @ 2013-07-29 21:17 UTC (permalink / raw)
To: joe; +Cc: dan.carpenter, jhs, netdev, kernel-janitors
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-29 19:36 [patch] net_sched: stack info leak in cbq_dump_wrr() Dan Carpenter
2013-07-29 19:44 ` Joe Perches
@ 2013-07-29 21:20 ` Jiri Pirko
1 sibling, 0 replies; 11+ messages in thread
From: Jiri Pirko @ 2013-07-29 21:20 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jamal Hadi Salim, David S. Miller, netdev, kernel-janitors
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-29 21:17 ` David Miller
@ 2013-07-29 21:50 ` Joe Perches
0 siblings, 0 replies; 11+ messages in thread
From: Joe Perches @ 2013-07-29 21:50 UTC (permalink / raw)
To: David Miller; +Cc: dan.carpenter, jhs, netdev, kernel-janitors
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);
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-29 20:12 ` Joe Perches
2013-07-29 21:17 ` David Miller
@ 2013-07-30 6:55 ` Dan Carpenter
2013-07-30 7:12 ` Joe Perches
1 sibling, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2013-07-30 6:55 UTC (permalink / raw)
To: Joe Perches; +Cc: Jamal Hadi Salim, David S. Miller, netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-30 6:55 ` Dan Carpenter
@ 2013-07-30 7:12 ` Joe Perches
2013-07-30 7:18 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Joe Perches @ 2013-07-30 7:12 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jamal Hadi Salim, David S. Miller, netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-30 7:12 ` Joe Perches
@ 2013-07-30 7:18 ` David Miller
2013-07-30 10:18 ` walter harms
0 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2013-07-30 7:18 UTC (permalink / raw)
To: joe; +Cc: dan.carpenter, jhs, netdev, kernel-janitors
From: Joe Perches <joe@perches.com>
Date: Tue, 30 Jul 2013 00:12:32 -0700
> 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}.
Ok, I've just commited the following, thanks everyone.
--------------------
>From a0db856a95a29efb1c23db55c02d9f0ff4f0db48 Mon Sep 17 00:00:00 2001
From: "David S. Miller" <davem@davemloft.net>
Date: Tue, 30 Jul 2013 00:16:21 -0700
Subject: [PATCH] net_sched: Fix stack info leak in cbq_dump_wrr().
Make sure the reserved fields, and padding (if any), are
fully initialized.
Based upon a patch by Dan Carpenter and feedback from
Joe Perches.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/sched/sch_cbq.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 71a5688..7a42c81 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1465,6 +1465,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
unsigned char *b = skb_tail_pointer(skb);
struct tc_cbq_wrropt opt;
+ memset(&opt, 0, sizeof(opt));
opt.flags = 0;
opt.allot = cl->allot;
opt.priority = cl->priority + 1;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [patch] net_sched: stack info leak in cbq_dump_wrr()
2013-07-30 7:18 ` David Miller
@ 2013-07-30 10:18 ` walter harms
0 siblings, 0 replies; 11+ messages in thread
From: walter harms @ 2013-07-30 10:18 UTC (permalink / raw)
To: David Miller; +Cc: joe, dan.carpenter, netdev, kernel-janitors
Am 30.07.2013 09:18, schrieb David Miller:
> From: Joe Perches <joe@perches.com>
> Date: Tue, 30 Jul 2013 00:12:32 -0700
>
>> 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}.
>
> Ok, I've just commited the following, thanks everyone.
>
> --------------------
>>From a0db856a95a29efb1c23db55c02d9f0ff4f0db48 Mon Sep 17 00:00:00 2001
> From: "David S. Miller" <davem@davemloft.net>
> Date: Tue, 30 Jul 2013 00:16:21 -0700
> Subject: [PATCH] net_sched: Fix stack info leak in cbq_dump_wrr().
>
> Make sure the reserved fields, and padding (if any), are
> fully initialized.
>
> Based upon a patch by Dan Carpenter and feedback from
> Joe Perches.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
> net/sched/sch_cbq.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
> index 71a5688..7a42c81 100644
> --- a/net/sched/sch_cbq.c
> +++ b/net/sched/sch_cbq.c
> @@ -1465,6 +1465,7 @@ static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl)
> unsigned char *b = skb_tail_pointer(skb);
> struct tc_cbq_wrropt opt;
>
> + memset(&opt, 0, sizeof(opt));
> opt.flags = 0;
> opt.allot = cl->allot;
> opt.priority = cl->priority + 1;
You can remove opt.flags = 0; then :=)
re,
wh
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-07-30 10:18 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-29 19:36 [patch] net_sched: stack info leak in cbq_dump_wrr() Dan Carpenter
2013-07-29 19:44 ` Joe Perches
2013-07-29 20:01 ` Dan Carpenter
2013-07-29 20:12 ` Joe Perches
2013-07-29 21:17 ` David Miller
2013-07-29 21:50 ` Joe Perches
2013-07-30 6:55 ` Dan Carpenter
2013-07-30 7:12 ` Joe Perches
2013-07-30 7:18 ` David Miller
2013-07-30 10:18 ` walter harms
2013-07-29 21:20 ` Jiri Pirko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).