* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-23 14:45 ` walter harms
@ 2010-11-23 15:23 ` Américo Wang
2010-11-23 18:02 ` Andreas Dilger
2010-11-24 17:54 ` Valdis.Kletnieks
2 siblings, 0 replies; 33+ messages in thread
From: Américo Wang @ 2010-11-23 15:23 UTC (permalink / raw)
To: walter harms
Cc: Américo Wang, Eric Dumazet, Andrew Morton, Vasiliy Kulikov,
Andreas Dilger, kernel-janitors, Alexander Viro, linux-fsdevel,
linux-kernel, Jakub Jelinek
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-23 14:45 ` walter harms
2010-11-23 15:23 ` Américo Wang
@ 2010-11-23 18:02 ` Andreas Dilger
2010-11-23 20:18 ` Andrew Morton
2010-11-24 10:44 ` Pádraig Brady
2010-11-24 17:54 ` Valdis.Kletnieks
2 siblings, 2 replies; 33+ messages in thread
From: Andreas Dilger @ 2010-11-23 18:02 UTC (permalink / raw)
To: wharms
Cc: Américo Wang, Eric Dumazet, Andrew Morton, Vasiliy Kulikov,
kernel-janitors, Alexander Viro, linux-fsdevel, linux-kernel,
Jakub Jelinek
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-23 18:02 ` Andreas Dilger
@ 2010-11-23 20:18 ` Andrew Morton
2010-11-23 20:22 ` David Miller
` (2 more replies)
2010-11-24 10:44 ` Pádraig Brady
1 sibling, 3 replies; 33+ messages in thread
From: Andrew Morton @ 2010-11-23 20:18 UTC (permalink / raw)
To: Andreas Dilger
Cc: wharms, Américo Wang, Eric Dumazet, Vasiliy Kulikov,
kernel-janitors, Alexander Viro, linux-fsdevel, linux-kernel,
Jakub Jelinek
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?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-23 20:18 ` Andrew Morton
@ 2010-11-23 20:22 ` David Miller
2010-11-24 0:24 ` Andreas Dilger
2010-11-24 16:06 ` walter harms
2 siblings, 0 replies; 33+ messages in thread
From: David Miller @ 2010-11-23 20:22 UTC (permalink / raw)
To: akpm
Cc: adilger.kernel, wharms, xiyou.wangcong, eric.dumazet, segoon,
kernel-janitors, viro, linux-fsdevel, linux-kernel, jakub
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-23 20:18 ` Andrew Morton
2010-11-23 20:22 ` David Miller
@ 2010-11-24 0:24 ` Andreas Dilger
2010-11-24 16:06 ` walter harms
2 siblings, 0 replies; 33+ messages in thread
From: Andreas Dilger @ 2010-11-24 0:24 UTC (permalink / raw)
To: Andrew Morton
Cc: wharms, Américo Wang, Eric Dumazet, Vasiliy Kulikov,
kernel-janitors, Alexander Viro, linux-fsdevel, linux-kernel,
Jakub Jelinek
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-23 20:18 ` Andrew Morton
2010-11-23 20:22 ` David Miller
2010-11-24 0:24 ` Andreas Dilger
@ 2010-11-24 16:06 ` walter harms
2 siblings, 0 replies; 33+ messages in thread
From: walter harms @ 2010-11-24 16:06 UTC (permalink / raw)
To: Andrew Morton
Cc: Andreas Dilger, Américo Wang, Eric Dumazet, Vasiliy Kulikov,
kernel-janitors, Alexander Viro, linux-fsdevel, linux-kernel,
Jakub Jelinek
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-23 18:02 ` Andreas Dilger
2010-11-23 20:18 ` Andrew Morton
@ 2010-11-24 10:44 ` Pádraig Brady
2010-11-24 11:05 ` Américo Wang
1 sibling, 1 reply; 33+ messages in thread
From: Pádraig Brady @ 2010-11-24 10:44 UTC (permalink / raw)
To: Andreas Dilger
Cc: wharms, Américo Wang, Eric Dumazet, Andrew Morton,
Vasiliy Kulikov, kernel-janitors, Alexander Viro, linux-fsdevel,
linux-kernel, Jakub Jelinek
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-24 10:44 ` Pádraig Brady
@ 2010-11-24 11:05 ` Américo Wang
2010-11-24 11:46 ` Pádraig Brady
0 siblings, 1 reply; 33+ messages in thread
From: Américo Wang @ 2010-11-24 11:05 UTC (permalink / raw)
To: Pádraig Brady
Cc: Andreas Dilger, wharms, Américo Wang, Eric Dumazet,
Andrew Morton, Vasiliy Kulikov, kernel-janitors, Alexander Viro,
linux-fsdevel, linux-kernel, Jakub Jelinek
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-24 11:05 ` Américo Wang
@ 2010-11-24 11:46 ` Pádraig Brady
2010-11-24 12:32 ` Américo Wang
2010-12-15 9:49 ` Al Viro
0 siblings, 2 replies; 33+ messages in thread
From: Pádraig Brady @ 2010-11-24 11:46 UTC (permalink / raw)
To: Américo Wang
Cc: Andreas Dilger, wharms, Eric Dumazet, Andrew Morton,
Vasiliy Kulikov, kernel-janitors, Alexander Viro, linux-fsdevel,
linux-kernel, Jakub Jelinek
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-24 11:46 ` Pádraig Brady
@ 2010-11-24 12:32 ` Américo Wang
2010-12-15 9:49 ` Al Viro
1 sibling, 0 replies; 33+ messages in thread
From: Américo Wang @ 2010-11-24 12:32 UTC (permalink / raw)
To: Pádraig Brady
Cc: Américo Wang, Andreas Dilger, wharms, Eric Dumazet,
Andrew Morton, Vasiliy Kulikov, kernel-janitors, Alexander Viro,
linux-fsdevel, linux-kernel, Jakub Jelinek
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-24 11:46 ` Pádraig Brady
2010-11-24 12:32 ` Américo Wang
@ 2010-12-15 9:49 ` Al Viro
2010-12-15 20:30 ` Andreas Dilger
1 sibling, 1 reply; 33+ messages in thread
From: Al Viro @ 2010-12-15 9:49 UTC (permalink / raw)
To: P??draig Brady
Cc: Am??rico Wang, Andreas Dilger, wharms, Eric Dumazet,
Andrew Morton, Vasiliy Kulikov, kernel-janitors, linux-fsdevel,
linux-kernel, Jakub Jelinek
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-12-15 9:49 ` Al Viro
@ 2010-12-15 20:30 ` Andreas Dilger
2010-12-15 20:33 ` Julia Lawall
0 siblings, 1 reply; 33+ messages in thread
From: Andreas Dilger @ 2010-12-15 20:30 UTC (permalink / raw)
To: Al Viro
Cc: P??draig Brady, Am??rico Wang, wharms, Eric Dumazet,
Andrew Morton, Vasiliy Kulikov, kernel-janitors, linux-fsdevel,
linux-kernel, Jakub Jelinek
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-12-15 20:30 ` Andreas Dilger
@ 2010-12-15 20:33 ` Julia Lawall
2010-12-15 20:52 ` Eric Dumazet
0 siblings, 1 reply; 33+ messages in thread
From: Julia Lawall @ 2010-12-15 20:33 UTC (permalink / raw)
To: Andreas Dilger
Cc: Al Viro, P??draig Brady, Am??rico Wang, wharms, Eric Dumazet,
Andrew Morton, Vasiliy Kulikov, kernel-janitors, linux-fsdevel,
linux-kernel, Jakub Jelinek
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-12-15 20:33 ` Julia Lawall
@ 2010-12-15 20:52 ` Eric Dumazet
2010-12-15 22:19 ` Andreas Dilger
0 siblings, 1 reply; 33+ messages in thread
From: Eric Dumazet @ 2010-12-15 20:52 UTC (permalink / raw)
To: Julia Lawall
Cc: Andreas Dilger, Al Viro, P??draig Brady, Am??rico Wang, wharms,
Andrew Morton, Vasiliy Kulikov, kernel-janitors, linux-fsdevel,
linux-kernel, Jakub Jelinek
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-12-15 20:52 ` Eric Dumazet
@ 2010-12-15 22:19 ` Andreas Dilger
2010-12-16 9:39 ` Boaz Harrosh
0 siblings, 1 reply; 33+ messages in thread
From: Andreas Dilger @ 2010-12-15 22:19 UTC (permalink / raw)
To: Eric Dumazet
Cc: Julia Lawall, Al Viro, P??draig Brady, Am??rico Wang, wharms,
Andrew Morton, Vasiliy Kulikov, kernel-janitors, linux-fsdevel,
linux-kernel, Jakub Jelinek
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-12-15 22:19 ` Andreas Dilger
@ 2010-12-16 9:39 ` Boaz Harrosh
0 siblings, 0 replies; 33+ messages in thread
From: Boaz Harrosh @ 2010-12-16 9:39 UTC (permalink / raw)
To: Andreas Dilger
Cc: Eric Dumazet, Julia Lawall, Al Viro, P??draig Brady,
Am??rico Wang, wharms, Andrew Morton, Vasiliy Kulikov,
kernel-janitors, linux-fsdevel, linux-kernel, Jakub Jelinek
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2] fs: select: fix information leak to userspace
2010-11-23 14:45 ` walter harms
2010-11-23 15:23 ` Américo Wang
2010-11-23 18:02 ` Andreas Dilger
@ 2010-11-24 17:54 ` Valdis.Kletnieks
2 siblings, 0 replies; 33+ messages in thread
From: Valdis.Kletnieks @ 2010-11-24 17:54 UTC (permalink / raw)
To: wharms
Cc: Américo Wang, Eric Dumazet, Andrew Morton, Vasiliy Kulikov,
Andreas Dilger, kernel-janitors, Alexander Viro, linux-fsdevel,
linux-kernel, Jakub Jelinek
[-- 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 --]
^ permalink raw reply [flat|nested] 33+ messages in thread