net_sched: stack info leak in cbq_dump_wrr()

PATCH In review Dan Carpenter 5 participants 11 messages Jul 29, 2013 – Jul 30, 2013

Subsystems (4)

Networking Network drivers Kernel janitors Traffic control (tc)

lore

Share & tools

lore

Dan Carpenter

author

2013-07-29 19:36 UTC #0 lore raw

PATCH 1/1 net_sched: stack info leak in cbq_dump_wrr()

opt.__reserved isn't cleared so we leak a byte of stack information.

Signed-off-by Dan Carpenter <[email protected]>

net/sched/sch_cbq.c +1 -0

index 71a5688..6398a61 100644
@@ -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;

Joe Perches

2013-07-29 19:44 UTC #1 lore raw

Quoted 1 line · Dan Carpenter

opt.__reserved isn't cleared so we leak a byte of stack information.

[]

net/sched/sch_cbq.c

Quoted 1 line

net/sched/sch_cbq.c +0 -0

[]

net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1475

Quoted 8 lines

net/sched/sch_cbq.c +1 -0

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

Dan Carpenter

author

2013-07-29 20:01 UTC #2 lore raw

Quoted 5 lines · Joe Perches

Quoted 1 line · Dan Carpenter
opt.__reserved isn't cleared so we leak a byte of stack information.
[]
net/sched/sch_cbq.c
Quoted 1 line
net/sched/sch_cbq.c +0 -0
[]

Quoted 12 lines

net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1475
Quoted 8 lines
net/sched/sch_cbq.c +1 -0
@@ -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

Joe Perches

2013-07-29 20:12 UTC #3 lore raw

Quoted 19 lines · Dan Carpenter

Quoted 5 lines · Joe Perches
Quoted 1 line · Dan Carpenter
opt.__reserved isn't cleared so we leak a byte of stack information.
[]
net/sched/sch_cbq.c
Quoted 1 line
net/sched/sch_cbq.c +0 -0
[]
Quoted 12 lines
net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1475
Quoted 8 lines
net/sched/sch_cbq.c +1 -0
@@ -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.

Quoted 1 line

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?

David Miller

maintainer

2013-07-29 21:17 UTC #4 lore raw

From: Joe Perches <[email protected]>
Date: Mon, 29 Jul 2013 13:12:31 -0700

Quoted 20 lines · Dan Carpenter

Quoted 19 lines · Joe Perches
Quoted 17 lines · Dan Carpenter
Quoted 1 line
opt.__reserved isn't cleared so we leak a byte of stack information.
[]
net/sched/sch_cbq.c
Quoted 1 line
net/sched/sch_cbq.c +0 -0
[]
net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1475
Quoted 8 lines
net/sched/sch_cbq.c +1 -0
@@ -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.

Quoted 6 lines

Quoted 1 line
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.

Joe Perches

2013-07-29 21:50 UTC #5 lore raw

Quoted 25 lines · David Miller

From: Joe Perches <[email protected]>
Date: Mon, 29 Jul 2013 13:12:31 -0700
Quoted 22 lines · Dan Carpenter
Quoted 20 lines · Joe Perches
Quoted 5 lines · Dan Carpenter
Quoted 1 line
opt.__reserved isn't cleared so we leak a byte of stack information.
[]
net/sched/sch_cbq.c
Quoted 1 line
net/sched/sch_cbq.c +0 -0
[]
Quoted 12 lines
net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1475
Quoted 8 lines
net/sched/sch_cbq.c +1 -0
@@ -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?

Quoted 7 lines

Quoted 4 lines
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);
}

Dan Carpenter

author

2013-07-30 06:55 UTC #6 lore raw

In reply to Joe Perches

Quoted 20 lines · Joe Perches

Quoted 19 lines · Dan Carpenter
Quoted 17 lines · Joe Perches
Quoted 1 line · Dan Carpenter
opt.__reserved isn't cleared so we leak a byte of stack information.
[]
net/sched/sch_cbq.c
Quoted 1 line
net/sched/sch_cbq.c +0 -0
[]
net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1475
Quoted 8 lines
net/sched/sch_cbq.c +1 -0
@@ -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.

Quoted 7 lines

Quoted 1 line
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

Joe Perches

2013-07-30 07:12 UTC #7 lore raw

Quoted 25 lines · Dan Carpenter

Quoted 24 lines · Joe Perches
Quoted 19 lines · Dan Carpenter
Quoted 5 lines · Joe Perches
Quoted 1 line · Dan Carpenter
opt.__reserved isn't cleared so we leak a byte of stack information.
[]
net/sched/sch_cbq.c
Quoted 1 line
net/sched/sch_cbq.c +0 -0
[]
Quoted 12 lines
net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1475
Quoted 8 lines
net/sched/sch_cbq.c +1 -0
@@ -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.
Quoted 1 line
Hm... Which arches would align it differently?
Hey Dan.

Quoted 7 lines

Quoted 3 lines
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

David Miller

maintainer

2013-07-30 07:18 UTC #8 lore raw

From: Joe Perches <[email protected]>
Date: Tue, 30 Jul 2013 00:12:32 -0700

Quoted 39 lines · Dan Carpenter

Quoted 25 lines · Joe Perches
Quoted 24 lines · Dan Carpenter
Quoted 19 lines · Joe Perches
Quoted 5 lines · Dan Carpenter
Quoted 1 line
opt.__reserved isn't cleared so we leak a byte of stack information.
[]
net/sched/sch_cbq.c
Quoted 1 line
net/sched/sch_cbq.c +0 -0
[]
Quoted 12 lines
net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1475
Quoted 8 lines
net/sched/sch_cbq.c +1 -0
@@ -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.
Quoted 1 line
Hm... Which arches would align it differently?
Hey Dan.
Quoted 7 lines
Quoted 3 lines
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.

Quoted 6 lines

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" <[email protected]>
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 <[email protected]>

---
 net/sched/sch_cbq.c | 1 +
 1 file changed, 1 insertion(+)

net/sched/sch_cbq.c +1 -0

index 71a5688..7a42c81 100644
@@ -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;

Signature · 1 line

1.7.11.7

walter harms

2013-07-30 10:18 UTC #9 lore raw

Am 30.07.2013 09:18, schrieb David Miller:

Quoted 73 lines

From: Joe Perches <[email protected]>
Date: Tue, 30 Jul 2013 00:12:32 -0700
Quoted 39 lines · Dan Carpenter
Quoted 25 lines · Joe Perches
Quoted 24 lines · Dan Carpenter
Quoted 19 lines · Joe Perches
Quoted 5 lines · Dan Carpenter
Quoted 1 line
opt.__reserved isn't cleared so we leak a byte of stack information.
[]
net/sched/sch_cbq.c
Quoted 1 line
net/sched/sch_cbq.c +0 -0
[]
Quoted 12 lines
net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1475
Quoted 8 lines
net/sched/sch_cbq.c +1 -0
@@ -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.
Quoted 1 line
Hm... Which arches would align it differently?
Hey Dan.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
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.
--------------------
Quoted 1 line
From a0db856a95a29efb1c23db55c02d9f0ff4f0db48 Mon Sep 17 00:00:00 2001
From: "David S. Miller" <[email protected]>
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 <[email protected]>
---
 net/sched/sch_cbq.c | 1 +
 1 file changed, 1 insertion(+)
net/sched/sch_cbq.c +0 -0
index 71a5688..7a42c81 100644

net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1471

Quoted 7 lines

net/sched/sch_cbq.c +1 -0

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

Jiri Pirko

maintainer

2013-07-29 21:20 UTC #10 lore raw

In reply to Dan Carpenter

net/sched/sch_cbq.c static int cbq_dump_wrr(struct sk_buff *skb, struct cbq_class *cl) · line 1472

Quoted 13 lines · Mon, Jul 29, 2013 at 09:36:51PM CEST, [email protected] wrote:

opt.__reserved isn't cleared so we leak a byte of stack information.
Signed-off-by Dan Carpenter <[email protected]>
net/sched/sch_cbq.c +1 -0
index 71a5688..6398a61 100644
@@ -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.