fs: select: fix information leak to userspace

On some architectures __kernel_suseconds_t is int.

On these archs
struct timeval has padding bytes at the end. This struct is copied to userspace with these padding bytes uninitialized. This leads to leaking of contents of kernel stack memory.

This bug was added with v2.6.27-rc5-286-gb773ad4.

Signed-off-by Vasiliy Kulikov <[email protected]>

---
Compile tested.

 fs/select.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

fs/select.c +0 -0

index b7b10aa..32cf018 100644

Quoted 1 line · On Wed, 10 Nov 2010 23:38:02 +0300 Vasiliy Kulikov <[email protected]> wrote:

On some architectures __kernel_suseconds_t is int.

On sparc and parisc. On all other architectures this patch is a waste of cycles.

fs/select.c renamed from fs/select.c~fs-select-fix-information-leak-to-userspace-fix +2 -1

@@ -306,7 +306,8 @@ static int poll_select_copy_remaining(st
 		rts.tv_sec = rts.tv_nsec = 0;
 
 	if (timeval) {
-		memset(&rtv, 0, sizeof(rtv));
+		if (sizeof(rtv) > sizeof(rtv.tv_sec) + sizeof(rtv.tv_usec))
+			memset(&rtv, 0, sizeof(rtv));
 		rtv.tv_sec = rts.tv_sec;
 		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
 

_

The `if' gets eliminated at compile time. With this approach we add four bytes of text to the sparc64 build and zero bytes of text to the x86_64 build.

On some architectures __kernel_suseconds_t is int. On these archs struct timeval has padding bytes at the end. This struct is copied to userspace with these padding bytes uninitialized. This leads to leaking of contents of kernel stack memory.

Signed-off-by Vasiliy Kulikov <[email protected]>

---
Patch v1 used memset(), it was waste of cycles on almost all archs.

Compile tested.

 fs/select.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

fs/select.c +0 -1

index b7b10aa..43d4805 100644
@@ -288,7 +288,6 @@ static int poll_select_copy_remaining(struct timespec *end_time, void __user *p,
 				      int timeval, int ret)
 {
 	struct timespec rts;
-	struct timeval rtv;
 
 	if (!p)
 		return ret;

@@ -306,8 +305,10 @@ static int poll_select_copy_remaining(struct timespec *end_time, void __user *p,
 		rts.tv_sec = rts.tv_nsec = 0;
 

Quoted 9 lines · On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <[email protected]> wrote:

fs/select.c +4 -2

 	if (timeval) {
-		rtv.tv_sec = rts.tv_sec;
-		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
+		struct timeval rtv = {
+			.tv_sec = rts.tv_sec,
+			.tv_usec = rts.tv_nsec / NSEC_PER_USEC
+		};

fs/select.c static int poll_select_copy_remaining(struct timespec *end_time, void __user *p, · line 314

Quoted 2 lines

fs/select.c +0 -0

 		if (!copy_to_user(p, &rtv, sizeof(rtv)))
 			return ret;

Please check the assembly code - this will still leave four bytes of uninitalised stack data in 'rtv', surely.

Quoted 10 lines · Andrew Morton

Quoted 9 lines · On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <[email protected]> wrote:

fs/select.c +4 -2

 	if (timeval) {
-		rtv.tv_sec = rts.tv_sec;
-		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
+		struct timeval rtv = {
+			.tv_sec = rts.tv_sec,
+			.tv_usec = rts.tv_nsec / NSEC_PER_USEC
+		};

Quoted 5 lines

fs/select.c static int poll_select_copy_remaining(struct timespec *end_time, void __user *p, · line 314

Quoted 2 lines

fs/select.c +0 -0

 		if (!copy_to_user(p, &rtv, sizeof(rtv)))
 			return ret;

Please check the assembly code - this will still leave four bytes of uninitalised stack data in 'rtv', surely.

Thats a good question.

In my understanding, gcc should initialize all holes (and other not mentioned fields) with 0, even for automatic storage [C99 only mandates this on static storage]

I tested on x86_64 and this is the case, but could not find a definitive answer in gcc documentation.

This kind of construct is widely used in networking tree.

Maybe we should ask to gcc experts if this behavior is guaranteed by gcc, or if we must review our code ;(

CC Jakub

Thanks !

Quoted 33 lines · Eric Dumazet

Le dimanche 14 novembre 2010 __ 18:06 -0800, Andrew Morton a __crit :

Quoted 10 lines · On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <[email protected]> wrote:

Quoted 9 lines

fs/select.c +4 -2

 	if (timeval) {
-		rtv.tv_sec = rts.tv_sec;
-		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
+		struct timeval rtv = {
+			.tv_sec = rts.tv_sec,
+			.tv_usec = rts.tv_nsec / NSEC_PER_USEC
+		};

Quoted 5 lines

fs/select.c static int poll_select_copy_remaining(struct timespec *end_time, void __user *p, · line 314

Quoted 2 lines

fs/select.c +0 -0

 		if (!copy_to_user(p, &rtv, sizeof(rtv)))
 			return ret;

Please check the assembly code - this will still leave four bytes of uninitalised stack data in 'rtv', surely.

Thats a good question.

In my understanding, gcc should initialize all holes (and other not mentioned fields) with 0, even for automatic storage [C99 only mandates this on static storage]

I tested on x86_64 and this is the case, but could not find a definitive answer in gcc documentation.

This kind of construct is widely used in networking tree.

Maybe we should ask to gcc experts if this behavior is guaranteed by gcc, or if we must review our code ;(

CC Jakub

Quoted 2 lines

Thanks !

This is what I thought too. If it is not there are tones of bugs I wrote of code that relays on this behaviour.

It would be interesting to know for sure

Well. We certainly assume in many places that

	struct foo {
		int a;
		int b;
	} f = {
		.a = 1,
	};

will initialise b to zero. But I doubt if much code at all assumes that this initialisation patterm will reliably zero out *holes* in the struct.

Le lundi 22 novembre 2010 __ 15:50 -0800, Andrew Morton a __crit :

Quoted 9 lines

Well. We certainly assume in many places that

	struct foo {
		int a;
		int b;
	} f = {
		.a = 1,
	};

Quoted 4 lines

will initialise b to zero. But I doubt if much code at all assumes that this initialisation patterm will reliably zero out *holes* in the struct.

We did such assertions in the past, we were wrong.

Check commit 1c40be12f7d8ca1d387510d39787b12e512a7ce8 for an example (net sched: fix some kernel memory leaks)

I guess we must make a full audit of all C99 initializers or structures copied to userspace, giving a name to hidden holes, to force gcc to init them to 0.

# cat try.c
struct s {
	char c;
	long l;
	};

void bar(void *v)

{
	unsigned long *p = v;

	printf("%lx %lx\n", p[0], p[1]);
}

int main()

{
	struct s s1 = {
		.c = 1,
		.l = 2,
	};

	bar(&s1);
	return 0;
}

# gcc -O2 -o try try.c
# ./try
8049401 2

Strangely, if we remove ".l = 2," line, gcc emits code to clear al the fields

Quoted 11 lines · On Tue, 23 Nov 2010 01:20:48 +0100 Eric Dumazet <[email protected]> wrote:

Le lundi 22 novembre 2010 __ 15:50 -0800, Andrew Morton a __crit :

Quoted 9 lines

Well. We certainly assume in many places that

	struct foo {
		int a;
		int b;
	} f = {
		.a = 1,
	};

Quoted 6 lines

Quoted 4 lines

will initialise b to zero. But I doubt if much code at all assumes that this initialisation patterm will reliably zero out *holes* in the struct.

We did such assertions in the past, we were wrong.

Well, that sucks... I know I wrote some code that relied on holes getting zeroed as well. Is there no option to GCC to make this work?

Quoted 10 lines · Andrew Morton

Quoted 9 lines · On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <[email protected]> wrote:

fs/select.c +4 -2

 	if (timeval) {
-		rtv.tv_sec = rts.tv_sec;
-		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
+		struct timeval rtv = {
+			.tv_sec = rts.tv_sec,
+			.tv_usec = rts.tv_nsec / NSEC_PER_USEC
+		};

Quoted 5 lines

fs/select.c static int poll_select_copy_remaining(struct timespec *end_time, void __user *p, · line 314

Quoted 2 lines

fs/select.c +0 -0

 		if (!copy_to_user(p, &rtv, sizeof(rtv)))
 			return ret;

Please check the assembly code - this will still leave four bytes of uninitalised stack data in 'rtv', surely.

Thats a good question.

In my understanding, gcc should initialize all holes (and other not mentioned fields) with 0, even for automatic storage [C99 only mandates this on static storage]

I tested on x86_64 and this is the case, but could not find a definitive answer in gcc documentation.

Quoted 17 lines · Andrew Morton

Quoted 10 lines · On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <[email protected]> wrote:

Quoted 9 lines

fs/select.c +4 -2

 	if (timeval) {
-		rtv.tv_sec = rts.tv_sec;
-		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
+		struct timeval rtv = {
+			.tv_sec = rts.tv_sec,
+			.tv_usec = rts.tv_nsec / NSEC_PER_USEC
+		};

Quoted 5 lines

fs/select.c static int poll_select_copy_remaining(struct timespec *end_time, void __user *p, · line 314

Quoted 2 lines

fs/select.c +0 -0

 		if (!copy_to_user(p, &rtv, sizeof(rtv)))
 			return ret;

Please check the assembly code - this will still leave four bytes of uninitalised stack data in 'rtv', surely.

Quoted 9 lines

Thats a good question.

In my understanding, gcc should initialize all holes (and other not mentioned fields) with 0, even for automatic storage [C99 only mandates this on static storage]

I tested on x86_64 and this is the case, but could not find a definitive answer in gcc documentation.

Yeah, this is not clearly defined by C99 I think, but we can still find some clues in 6.2.6.1, Paragraph 6,

"
When a value is stored in an object of structure or union type, including in a member object, the bytes of the object representation that correspond to any padding bytes take unspecified values. "

So we can't rely on the compiler to initialize the padding bytes too.

Am 23.11.2010 15:01, schrieb Américo Wang:

Quoted 13 lines · Eric Dumazet

Quoted 8 lines

In my understanding, gcc should initialize all holes (and other not mentioned fields) with 0, even for automatic storage [C99 only mandates this on static storage]

I tested on x86_64 and this is the case, but could not find a definitive answer in gcc documentation.

Yeah, this is not clearly defined by C99 I think, but we can still find some clues in 6.2.6.1, Paragraph 6,

Quoted 9 lines

"
When a value is stored in an object of structure or union type, including in a member object, the bytes of the object representation that correspond to any padding bytes take unspecified values. "

So we can't rely on the compiler to initialize the padding bytes too.

hi all,
as we see this is not a question of c99.
Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

People that still want a "undefined" (for what ever reason) can use an option to enable it again (e.g. --no-zero-padding).

do anyone have a contact so we can forward that request ?

Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

Quoted 1 line · walter harms

Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

It makes the most sense to tackle this at the GCC level, since the added overhead of doing memset(0) on the whole struct may be non-trivial for commonly-used and/or large structures.

We can't reasonably address this with gcc changes. If gcc starts doing what we want in the next release, how long will it be until that release is the *oldest* version of gcc which the kernel may be compiled with? Five years?

Quoted 1 line · On Tue, 23 Nov 2010 11:02:44 -0700 Andreas Dilger <[email protected]>…

On 2010-11-23, at 07:45, walter harms wrote:

Quoted 3 lines

Quoted 1 line

Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

It makes the most sense to tackle this at the GCC level, since the added overhead of doing memset(0) on the whole struct may be non-trivial for commonly-used and/or large structures.

We can't reasonably address this with gcc changes. If gcc starts doing what we want in the next release, how long will it be until that release is the *oldest* version of gcc which the kernel may be compiled with? Five years?

Quoted 3 lines · On Tue, 23 Nov 2010 11:02:44 -0700 Andreas Dilger <[email protected]>…

Quoted 1 line · walter harms

Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

Quoted 1 line

It makes the most sense to tackle this at the GCC level, since the added overhead of doing memset(0) on the whole struct may be non-trivial for commonly-used and/or large structures.

(My, what long lines you have!)

We can't reasonably address this with gcc changes. If gcc starts doing what we want in the next release, how long will it be until that release is the *oldest* version of gcc which the kernel may be compiled with? Five years?

Quoted 1 line · walter harms

Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

It makes the most sense to tackle this at the GCC level, since the added overhead of doing memset(0) on the whole struct may be non-trivial for commonly-used and/or large structures. Since GCC is already explicitly zeroing the _used_ fields in the struct, it can much more easily determine whether there is padding in the structure, and zero those few bytes as needed.

Quoted 4 lines · Andreas Dilger

Quoted 1 line · walter harms

Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

It makes the most sense to tackle this at the GCC level, since the added overhead of doing memset(0) on the whole struct may be non-trivial for commonly-used and/or large structures. Since GCC is already explicitly zeroing the _used_ fields in the struct, it can much more easily determine whether there is padding in the structure, and zero those few bytes as needed.

Zero padding structs is part of C90. Details here:
http://www.pixelbeat.org/programming/gcc/auto_init.html

gcc doesn't zero pad when _all_ elements are specified.

Quoted 8 lines · Pádraig Brady

Quoted 4 lines · Andreas Dilger

Quoted 1 line · walter harms

Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

It makes the most sense to tackle this at the GCC level, since the added overhead of doing memset(0) on the whole struct may be non-trivial for commonly-used and/or large structures. Since GCC is already explicitly zeroing the _used_ fields in the struct, it can much more easily determine whether there is padding in the structure, and zero those few bytes as needed.

Zero padding structs is part of C90. Details here:
http://www.pixelbeat.org/programming/gcc/auto_init.html

Nope.

Quoted 3 lines

gcc doesn't zero pad when _all_ elements are specified.

That is what gcc does, not what C standard specifies.

Quoted 10 lines · Américo Wang

Quoted 8 lines · Pádraig Brady

Quoted 4 lines · Andreas Dilger

Quoted 1 line · walter harms

Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

It makes the most sense to tackle this at the GCC level, since the added overhead of doing memset(0) on the whole struct may be non-trivial for commonly-used and/or large structures. Since GCC is already explicitly zeroing the _used_ fields in the struct, it can much more easily determine whether there is padding in the structure, and zero those few bytes as needed.

Zero padding structs is part of C90. Details here:
http://www.pixelbeat.org/programming/gcc/auto_init.html

Quoted 7 lines

Nope.

Quoted 3 lines

gcc doesn't zero pad when _all_ elements are specified.

That is what gcc does, not what C standard specifies.

Looks like gcc is following the standard exactly.

C90 - 6.5.7
C99 - 6.7.8

If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate ... the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.

Looks like gcc is following the standard exactly.

C90 - 6.5.7
C99 - 6.7.8

If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate ... the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.

Incorrect. See 6.2.6.1 in C99; basically, padding bytes have unspecified contents. Implementation is allowed to leave them in any state (including not bothering to copy them when doing struct assignments, etc.). See Appendix J (portability issues) as well.

The bottom line: if you rely on that, you are relying on non-portable details of compiler behaviour. Moreover, the authors are not even required to document what they are doing or to keep that behaviour consistent.

Quoted 5 lines · Al Viro

Incorrect. See 6.2.6.1 in C99; basically, padding bytes have unspecified contents. Implementation is allowed to leave them in any state (including not bothering to copy them when doing struct assignments, etc.). See Appendix J (portability issues) as well.

Quoted 4 lines

The bottom line: if you rely on that, you are relying on non-portable details of compiler behaviour. Moreover, the authors are not even required to document what they are doing or to keep that behaviour consistent.

I thought my proposed solution was reasonable - add explicit padding fields where there are holes in the structure, which would be unused by the kernel, but since they are defined fields the compiler is obligated to initialize them.

This wouldn't add any overhead in cases where the compiler is already initializing the fields, and is still going to be less overhead than doing a memset(0) on the whole structure, and then initializing the other fields explicitly.

That has the added bonus that it becomes instantly clear where there are padding fields in the structure, and possibly they can be put to use in the future.

Cheers, Andreas

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 11 lines · Andreas Dilger

Quoted 9 lines · Al Viro

Incorrect. See 6.2.6.1 in C99; basically, padding bytes have unspecified contents. Implementation is allowed to leave them in any state (including not bothering to copy them when doing struct assignments, etc.). See Appendix J (portability issues) as well.

The bottom line: if you rely on that, you are relying on non-portable details of compiler behaviour. Moreover, the authors are not even required to document what they are doing or to keep that behaviour consistent.

Quoted 1 line

I thought my proposed solution was reasonable - add explicit padding fields where there are holes in the structure, which would be unused by the kernel, but since they are defined fields the compiler is obligated to initialize them.

Is the presence of holes always apparent at the source code level? Or is it dependent on the compiler or target architecture?

Quoted 5 lines · Julia Lawall

Quoted 1 line · Andreas Dilger

I thought my proposed solution was reasonable - add explicit padding fields where there are holes in the structure, which would be unused by the kernel, but since they are defined fields the compiler is obligated to initialize them.

Is the presence of holes always apparent at the source code level? Or is it dependent on the compiler or target architecture?

It depends on target architecture.

This means doing a full review to add a named padding only for arches that need it.

Quoted 7 lines · Eric Dumazet

Quoted 5 lines · Julia Lawall

Quoted 1 line · Andreas Dilger

I thought my proposed solution was reasonable - add explicit padding fields where there are holes in the structure, which would be unused by the kernel, but since they are defined fields the compiler is obligated to initialize them.

Is the presence of holes always apparent at the source code level? Or is it dependent on the compiler or target architecture?

Quoted 4 lines

It depends on target architecture.

This means doing a full review to add a named padding only for arches that need it.

There are automated tools like "pahole" (IIRC) that will report the presence of these structure holes. However, the memset(0) won't add itself to the code either (i.e. it needs an audit to determine if it is needed).

Cheers, Andreas

hi all,
as we see this is not a question of c99.
Maybe we can convince the gcc people to make 0 padding default. That will not solve the problems for other compilers but when they claim "works like gcc" we can press then to support this also. I can imagine that this will close some other subtle leaks also.

Quoted 10 lines

Quoted 10 lines

fs/select.c static int poll_select_copy_remaining(struct timespec *end_time, void __user *p, · line 314

Quoted 10 lines

fs/select.c +4 -2

 	if (timeval) {
-		rtv.tv_sec = rts.tv_sec;
-		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
+		struct timeval rtv = {
+			.tv_sec = rts.tv_sec,
+			.tv_usec = rts.tv_nsec / NSEC_PER_USEC
+		};
 
 		if (!copy_to_user(p, &rtv, sizeof(rtv)))
 			return ret;

Quoted 37 lines · On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <[email protected]> wrote:

On some architectures __kernel_suseconds_t is int. On these archs struct timeval has padding bytes at the end. This struct is copied to userspace with these padding bytes uninitialized. This leads to leaking of contents of kernel stack memory.

Signed-off-by Vasiliy Kulikov <[email protected]>

---
Patch v1 used memset(), it was waste of cycles on almost all archs.

Compile tested.

 fs/select.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

fs/select.c +4 -3

index b7b10aa..43d4805 100644
@@ -288,7 +288,6 @@ static int poll_select_copy_remaining(struct timespec *end_time, void __user *p,
 				      int timeval, int ret)
 {
 	struct timespec rts;
-	struct timeval rtv;
 
 	if (!p)
 		return ret;

@@ -306,8 +305,10 @@ static int poll_select_copy_remaining(struct timespec *end_time, void __user *p,
 		rts.tv_sec = rts.tv_nsec = 0;
 
 	if (timeval) {
-		rtv.tv_sec = rts.tv_sec;
-		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
+		struct timeval rtv = {
+			.tv_sec = rts.tv_sec,
+			.tv_usec = rts.tv_nsec / NSEC_PER_USEC
+		};
 

fs/select.c static int poll_select_copy_remaining(struct timespec *end_time, void __user *p, · line 314

Quoted 2 lines

fs/select.c +0 -0

 		if (!copy_to_user(p, &rtv, sizeof(rtv)))
 			return ret;

Please check the assembly code - this will still leave four bytes of uninitalised stack data in 'rtv', surely.

Quoted 19 lines · Andrew Morton

Quoted 1 line · On Wed, 10 Nov 2010 23:38:02 +0300 Vasiliy Kulikov <[email protected]> wrote:

On some architectures __kernel_suseconds_t is int.

On sparc and parisc. On all other architectures this patch is a waste of cycles.

fs/select.c renamed from fs/select.c~fs-select-fix-information-leak-to-userspace-fix +2 -1

@@ -306,7 +306,8 @@ static int poll_select_copy_remaining(st
 		rts.tv_sec = rts.tv_nsec = 0;
 
 	if (timeval) {
-		memset(&rtv, 0, sizeof(rtv));
+		if (sizeof(rtv) > sizeof(rtv.tv_sec) + sizeof(rtv.tv_usec))
+			memset(&rtv, 0, sizeof(rtv));
 		rtv.tv_sec = rts.tv_sec;
 		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
 

Quoted 6 lines

_

The `if' gets eliminated at compile time. With this approach we add four bytes of text to the sparc64 build and zero bytes of text to the x86_64 build.

It's nice to have comments (or at least a good commit message) for unusual code like this, so that in the future it is clear when this kind of workaround can be removed (e.g. if the time_t is changed to always be a 64-bit value for Y2038 issues, even on 32-bit arches).