[PATCH] fs: select: fix information leak to userspace

[PATCH] fs: select: fix information leak to userspace

[Vasiliy Kulikov]

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 <segooon@gmail.com>
---
 Compile tested.

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

diff --git a/fs/select.c b/fs/select.c
index b7b10aa..32cf018 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -306,6 +306,7 @@ static int poll_select_copy_remaining(struct timespec *end_time, void __user *p,
 		rts.tv_sec = rts.tv_nsec = 0;
 
 	if (timeval) {
+		memset(&rtv, 0, sizeof(rtv));
 		rtv.tv_sec = rts.tv_sec;
 		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;
 
-- 
1.7.0.4

Re: [PATCH] fs: select: fix information leak to userspace

[Andrew Morton]

On Wed, 10 Nov 2010 23:38:02 +0300
Vasiliy Kulikov <segooon@gmail.com> wrote:

> On some architectures __kernel_suseconds_t is int.

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

>  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 <segooon@gmail.com>
> ---
>  Compile tested.
> 
>  fs/select.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/select.c b/fs/select.c
> index b7b10aa..32cf018 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -306,6 +306,7 @@ static int poll_select_copy_remaining(struct timespec *end_time, void __user *p,
>  		rts.tv_sec = rts.tv_nsec = 0;
>  
>  	if (timeval) {
> +		memset(&rtv, 0, sizeof(rtv));
>  		rtv.tv_sec = rts.tv_sec;
>  		rtv.tv_usec = rts.tv_nsec / NSEC_PER_USEC;

How about this?

--- a/fs/select.c~fs-select-fix-information-leak-to-userspace-fix
+++ a/fs/select.c
@@ -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.

Re: [PATCH] fs: select: fix information leak to userspace

[Andreas Dilger]

On 2010-11-12, at 13:08, Andrew Morton wrote:
> On Wed, 10 Nov 2010 23:38:02 +0300
> Vasiliy Kulikov <segooon@gmail.com> wrote:
>> On some architectures __kernel_suseconds_t is int.
> 
> On sparc and parisc.  On all other architectures this patch is a waste
> of cycles.
> 
> --- a/fs/select.c~fs-select-fix-information-leak-to-userspace-fix
> +++ a/fs/select.c
> @@ -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.

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

Cheers, Andreas

[PATCH v2] fs: select: fix information leak to userspace

[Vasiliy Kulikov]

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 <segoon@openwall.com>
---
 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(-)

diff --git a/fs/select.c b/fs/select.c
index b7b10aa..43d4805 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -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
+		};
 
 		if (!copy_to_user(p, &rtv, sizeof(rtv)))
 			return ret;
-- 
1.7.0.4

Re: [PATCH v2] fs: select: fix information leak to userspace

[Andrew Morton]

On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <segoon@openwall.com> 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 <segoon@openwall.com>
> ---
>  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(-)
> 
> diff --git a/fs/select.c b/fs/select.c
> index b7b10aa..43d4805 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -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
> +		};
>  
>  		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.

Re: [PATCH v2] fs: select: fix information leak to userspace

[Eric Dumazet]

Le dimanche 14 novembre 2010 à 18:06 -0800, Andrew Morton a écrit : 
> On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <segoon@openwall.com> wrote:
> >  
> >  	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;
> 
> 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 !

Re: [PATCH v2] fs: select: fix information leak to userspace

[Boaz Harrosh]

On 11/15/2010 09:12 PM, Eric Dumazet wrote:
> Le dimanche 14 novembre 2010 à 18:06 -0800, Andrew Morton a écrit : 
>> On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <segoon@openwall.com> wrote:
>>>  
>>>  	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;
>>
>> 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 !
> 

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

Thanks
Boaz

Re: [PATCH v2] fs: select: fix information leak to userspace

[Andrew Morton]

On Tue, 16 Nov 2010 13:19:36 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 11/15/2010 09:12 PM, Eric Dumazet wrote:
> > Le dimanche 14 novembre 2010 __ 18:06 -0800, Andrew Morton a __crit : 
> >> On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <segoon@openwall.com> wrote:
> >>>  
> >>>  	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;
> >>
> >> 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 !
> > 
> 
> 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.

Re: [PATCH v2] fs: select: fix information leak to userspace

[Eric Dumazet]

Le lundi 22 novembre 2010 à 15:50 -0800, Andrew Morton a écrit :

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

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

main:
	pushl	%ebp
	movl	%esp, %ebp
	andl	$-16, %esp
	subl	$32, %esp
	leal	24(%esp), %eax
	movl	$0, 24(%esp)
	movl	%eax, (%esp)
	movl	$0, 28(%esp)
	movb	$1, 24(%esp)
	call	bar
	xorl	%eax, %eax
	leave
	ret

Re: [PATCH v2] fs: select: fix information leak to userspace

[Andrew Morton]

On Tue, 23 Nov 2010 01:20:48 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le lundi 22 novembre 2010 __ 15:50 -0800, Andrew Morton a __crit :
> 
> > 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.
> > 
> 
> 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

OK, thanks.  That rather settles it then.  memset() it is.

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

Maybe a glitch, maybe a small optimisation?  That's the sort of thing
which will change over gcc versions too..

Re: [PATCH v2] fs: select: fix information leak to userspace

[Dan Carpenter]

On Mon, Nov 22, 2010 at 04:32:34PM -0800, Andrew Morton wrote:
> On Tue, 23 Nov 2010 01:20:48 +0100
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > Le lundi 22 novembre 2010 __ 15:50 -0800, Andrew Morton a __crit :
> > 
> > > 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.
> > > 
> > 
> > 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?

regards,
dan carpenter

Re: [PATCH v2] fs: select: fix information leak to userspace

[Eric Dumazet]

Le mardi 23 novembre 2010 à 08:12 +0300, Dan Carpenter a écrit :

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

Apparently not.

At least, commits 0f04cfd098fb81fded74e78ea1a1b86cc6c6c31e and
1c40be12f7d8ca1d387510d39787b12e512a7ce8 were safe, as structures that
were touched dont have holes.

Re: [PATCH v2] fs: select: fix information leak to userspace

[Américo Wang]

On Mon, Nov 15, 2010 at 08:12:21PM +0100, Eric Dumazet wrote:
>Le dimanche 14 novembre 2010 à 18:06 -0800, Andrew Morton a écrit : 
>> On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <segoon@openwall.com> wrote:
>> >  
>> >  	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;
>> 
>> 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.
>

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.

-- 
Live like a child, think like the god.
 

Re: [PATCH v2] fs: select: fix information leak to userspace

[walter harms]

Am 23.11.2010 15:01, schrieb Américo Wang:
> On Mon, Nov 15, 2010 at 08:12:21PM +0100, Eric Dumazet wrote:
>> Le dimanche 14 novembre 2010 à 18:06 -0800, Andrew Morton a écrit : 
>>> On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <segoon@openwall.com> wrote:
>>>>  
>>>>  	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;
>>>
>>> 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.
>>
> 
> 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.
> 
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 ?

re,
 wh

Re: [PATCH v2] fs: select: fix information leak to userspace

[Américo Wang]

On Tue, Nov 23, 2010 at 03:45:18PM +0100, walter harms wrote:
>Am 23.11.2010 15:01, schrieb Américo Wang:
>> On Mon, Nov 15, 2010 at 08:12:21PM +0100, Eric Dumazet wrote:
>>>
>>> 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.
>> 
>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).


Well, IMHO, the default behavior should be "undefined", thus
"-fzero-padding" is needed. But, you know, I am not a compiler
people at all. :)

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

gcc@gcc.gnu.org ?

-- 
Live like a child, think like the god.
 

Re: [PATCH v2] fs: select: fix information leak to userspace

[Andreas Dilger]

On 2010-11-23, at 07:45, walter harms wrote:
> 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.

Cheers, Andreas

Re: [PATCH v2] fs: select: fix information leak to userspace

[Andrew Morton]

On Tue, 23 Nov 2010 11:02:44 -0700
Andreas Dilger <adilger.kernel@dilger.ca> wrote:

> On 2010-11-23, at 07:45, walter harms wrote:
> > 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.

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

Re: [PATCH v2] fs: select: fix information leak to userspace

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 23 Nov 2010 12:18:40 -0800

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

Completely agreed.

Re: [PATCH v2] fs: select: fix information leak to userspace

[Andreas Dilger]

On 2010-11-23, at 13:18, Andrew Morton wrote:
> On Tue, 23 Nov 2010 11:02:44 -0700
> Andreas Dilger <adilger.kernel@dilger.ca> wrote:
> 
>> On 2010-11-23, at 07:45, walter harms wrote:
>>> 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?

At the same time, we've had these same issues for 15+ years already, and IMHO I don't think anyone can extract a great deal of "information" from 4 bytes of random data interspersed in the middle of some structure that is otherwise initialized or zeroed out by the compiler.

On the other hand, calling memset() on these structures imposes a (virtually) permanent overhead on all processing of those structures.


If people are genuinely concerned about this kind of "information leak", a far better solution would be to add explicit padding fields to the structures (that are otherwise never going to be used) and then GCC will automatically initialize them without the overhead of initializing the fields twice.

Cheers, Andreas

Re: [PATCH v2] fs: select: fix information leak to userspace

[walter harms]

Am 23.11.2010 21:18, schrieb Andrew Morton:
> On Tue, 23 Nov 2010 11:02:44 -0700
> Andreas Dilger <adilger.kernel@dilger.ca> wrote:
> 
>> On 2010-11-23, at 07:45, walter harms wrote:
>>> 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.
> 
> (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?
> 
> 
agreed, but it does not mean not to talk to the gcc people that there is
an interesse to change.

re,
 wh

Re: [PATCH v2] fs: select: fix information leak to userspace

[Pádraig Brady]

On 23/11/10 18:02, Andreas Dilger wrote:
> On 2010-11-23, at 07:45, walter harms wrote:
>> 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.

So perhaps just:

-        struct timespec rts;
-        struct timeval rtv;
+        struct timespec rts = {0,};
+        struct timeval rtv = {0,};

One could also move the rtv declaration
to the scope where it's used.

cheers,
Pádraig.

Re: [PATCH v2] fs: select: fix information leak to userspace

[Américo Wang]

On Wed, Nov 24, 2010 at 10:44:50AM +0000, Pádraig Brady wrote:
>On 23/11/10 18:02, Andreas Dilger wrote:
>> On 2010-11-23, at 07:45, walter harms wrote:
>>> 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.

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

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

Re: [PATCH v2] fs: select: fix information leak to userspace

[Pádraig Brady]

On 24/11/10 11:05, Américo Wang wrote:
> On Wed, Nov 24, 2010 at 10:44:50AM +0000, Pádraig Brady wrote:
>> On 23/11/10 18:02, Andreas Dilger wrote:
>>> On 2010-11-23, at 07:45, walter harms wrote:
>>>> 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.
> 
>>
>> 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.

cheers,
Pádraig.

Re: [PATCH v2] fs: select: fix information leak to userspace

[Américo Wang]

On Wed, Nov 24, 2010 at 11:46:33AM +0000, Pádraig Brady wrote:
>On 24/11/10 11:05, Américo Wang wrote:
>> On Wed, Nov 24, 2010 at 10:44:50AM +0000, Pádraig Brady wrote:
>>> On 23/11/10 18:02, Andreas Dilger wrote:
>>>> On 2010-11-23, at 07:45, walter harms wrote:
>>>>> 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.
>> 
>>>
>>> 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.
>

Depends on if "the remainder of the aggregate" includes padding bytes
or not.

Re: [PATCH v2] fs: select: fix information leak to userspace

[Al Viro]

On Wed, Nov 24, 2010 at 11:46:33AM +0000, P??draig Brady wrote:

> 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 text you are quoting is about handling of missing initializers - it
essentially refers you back to the rules in 6.7.8p{9,10} (for arithmetical
types use zero, for pointers - null pointer, for arrays - apply recursively
to each array member, for structs - apply recursively for each named
member, for unions - apply recursively for the first named member).  That's
it - nothing in the standard says how to initialize padding.  Note that
these rules are _not_ guaranteed to be "fill everything with zero bits";
it often will be true, but it's implementation-dependent.  Implementation
may decide to fill padding with all-zeroes; it's not required to do so.

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.

Re: [PATCH v2] fs: select: fix information leak to userspace

[Andreas Dilger]

On 2010-12-15, at 02:49, Al Viro wrote:
> 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.

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

Re: [PATCH v2] fs: select: fix information leak to userspace

[Julia Lawall]

On Wed, 15 Dec 2010, Andreas Dilger wrote:

> On 2010-12-15, at 02:49, Al Viro wrote:
> > 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.
> 
> 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?

julia

> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2] fs: select: fix information leak to userspace

[Eric Dumazet]

Le mercredi 15 décembre 2010 à 21:33 +0100, Julia Lawall a écrit :
> On Wed, 15 Dec 2010, Andreas Dilger wrote:
> 
> > On 2010-12-15, at 02:49, Al Viro wrote:
> > > 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.
> > 
> > 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.

Re: [PATCH v2] fs: select: fix information leak to userspace

[Andreas Dilger]

On 2010-12-15, at 13:52, Eric Dumazet wrote:
> Le mercredi 15 décembre 2010 à 21:33 +0100, Julia Lawall a écrit :
>> On Wed, 15 Dec 2010, Andreas Dilger wrote:
>>> 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.

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

Re: [PATCH v2] fs: select: fix information leak to userspace

[Boaz Harrosh]

On 12/16/2010 12:19 AM, Andreas Dilger wrote:
> On 2010-12-15, at 13:52, Eric Dumazet wrote:
>> Le mercredi 15 décembre 2010 à 21:33 +0100, Julia Lawall a écrit :
>>> On Wed, 15 Dec 2010, Andreas Dilger wrote:
>>>> 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?
>>

If you let the compiler have the driving sit. But if you take matter
to your own hand, and one should when dealing with external interfaces
that might get accessed from different compilers/languages.

so:
struct export_foo {
	u32 m1;
	u32 padding1;
	u64 m2
	....
} __attribute__((aligned(8),packed))

will make sure that even other compilers/versions will do the same
thing. Also the same rule for on the wire structures.

Just choose the biggest type you have in the structure and
specify that, which will make good code for most cases.

At the end this things depend on if sizeof(long) is 8 or 4

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

I agree. It is always good practice for public structures to be
considered more delicately. All padding should be spelled out.
Not only for security but for future compatibility and maintenance.

> Cheers, Andreas
> 
> 

Thanks
Boaz

Re: [PATCH v2] fs: select: fix information leak to userspace

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1474 bytes --]

On Tue, 23 Nov 2010 15:45:18 +0100, walter harms said:

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

Note that zero padding by default has a price - the code has to include zeroing
instructions for each structure that needs it.  In the case of a function that
gets inlined, the zeroing instructions could easily cost almost as much as the
actual function.  So your code ends up bigger and slower. Let's look at that example
again:

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

Quick - is the optimizer able to eliminate the zero padding?  If so, how does
it know that?  And if the optimizer *can't* eliminate the zero padding, what
does that to do the overall generated code quality (especially on CPUs like the
x86 in 32-bit mode, where there's significant register pressure)?

You might be able to get them to add an *option* to force zero-padding, but
there's no way that's going to become the default.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH v2] fs: select: fix information leak to userspace

[Vasiliy Kulikov]

On Sun, Nov 14, 2010 at 18:06 -0800, Andrew Morton wrote:
> On Sun, 14 Nov 2010 12:25:33 +0300 Vasiliy Kulikov <segoon@openwall.com> 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 <segoon@openwall.com>
> > ---
> >  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(-)
> > 
> > diff --git a/fs/select.c b/fs/select.c
> > index b7b10aa..43d4805 100644
> > --- a/fs/select.c
> > +++ b/fs/select.c
> > @@ -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
> > +		};
> >  
> >  		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.

This concrete c code generates movl + movq, movl would zero unnamed
4 bytes.  However, I cannot find whether this behavior is guaranteed...

-- 
Vasiliy Kulikov

Re: [PATCH] fs: select: fix information leak to userspace

[Andrew Morton]

On Sat, 13 Nov 2010 14:38:19 -0700 Andreas Dilger <adilger.kernel@dilger.ca> wrote:

> On 2010-11-12, at 13:08, Andrew Morton wrote:
> > On Wed, 10 Nov 2010 23:38:02 +0300
> > Vasiliy Kulikov <segooon@gmail.com> wrote:
> >> On some architectures __kernel_suseconds_t is int.
> > 
> > On sparc and parisc.  On all other architectures this patch is a waste
> > of cycles.
> > 
> > --- a/fs/select.c~fs-select-fix-information-leak-to-userspace-fix
> > +++ a/fs/select.c
> > @@ -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.
> 
> 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). 
> 

Well, I'm the resident comment fanatic, but I thought this was all
sufficiently obvious to not need one.  But I'll add one ;)