From: Luis Henriques <hidden> Date: 2016-08-23 13:41:15
From: Avijit Kanti Das <redacted>
memset() the structure ethtool_wolinfo that has padded bytes
but the padded bytes have not been zeroed out.
Change-Id: If3fd2d872a1b1ab9521d937b86a29fc468a8bbfe
Signed-off-by: Avijit Kanti Das <redacted>
---
net/core/ethtool.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Joe Perches <joe@perches.com> Date: 2016-08-23 14:10:09
On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote:
From: Avijit Kanti Das <redacted>
memset() the structure ethtool_wolinfo that has padded bytes
but the padded bytes have not been zeroed out.
I expect there are more of these in the kernel tree.
While this patch is strictly true and the behavior is not
guaranteed by spec, what compilers do not memset then set
the specified member? Every time I've looked, gcc does.
From: Eric Dumazet <hidden> Date: 2016-08-23 14:41:49
On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote:
quoted hunk
From: Avijit Kanti Das <redacted>
memset() the structure ethtool_wolinfo that has padded bytes
but the padded bytes have not been zeroed out.
Change-Id: If3fd2d872a1b1ab9521d937b86a29fc468a8bbfe
Signed-off-by: Avijit Kanti Das <redacted>
---
net/core/ethtool.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
A compiler does not have a standards based requirement to
initialize arbitrary padding bytes.
I believe gcc always does zero all padding anyway.
I checked that my compiler does properly put zeros there, even in the
padding area.
If we can not rely on such constructs, we have hundreds of similar
patches to submit.
True.
From a practical point of view, does any compiler used for
kernel compilation (gcc/icc/llvm/any others?) not always
perform zero padding of alignment bytes?
From: Eric Dumazet <hidden> Date: 2016-08-23 15:38:30
On Tue, 2016-08-23 at 08:05 -0700, Joe Perches wrote:
A compiler does not have a standards based requirement to
initialize arbitrary padding bytes.
I believe gcc always does zero all padding anyway.
I would not worry for kernel code, because the amount of scrutiny there
will be enough to 'fix potential bugs' [1], but a lot of user land code
would suffer from various bugs as well that might sit there forever.
[1] Also, most call sites in the kernel are using same call stack, so
the offset of '1-7 leaked bytes' vs kernel stack is constant, making
exploits quite challenging.
Even if the current standards are lazy (are they, I did not check ?),
security needs would call for a sane compiler behavior and changing the
standards accordingly.
2016-08-23 18:36 GMT+03:00 Eric Dumazet [off-list ref]:
On Tue, 2016-08-23 at 08:05 -0700, Joe Perches wrote:
quoted
A compiler does not have a standards based requirement to
initialize arbitrary padding bytes.
I believe gcc always does zero all padding anyway.
I would not worry for kernel code, because the amount of scrutiny there
will be enough to 'fix potential bugs' [1], but a lot of user land code
would suffer from various bugs as well that might sit there forever.
[1] Also, most call sites in the kernel are using same call stack, so
the offset of '1-7 leaked bytes' vs kernel stack is constant, making
exploits quite challenging.
Even if the current standards are lazy (are they, I did not check ?),
security needs would call for a sane compiler behavior and changing the
standards accordingly.
From: David Miller <davem@davemloft.net> Date: 2016-08-23 16:40:56
From: Luis Henriques <redacted>
Date: Tue, 23 Aug 2016 14:41:07 +0100
Digging through some old CVEs I came across this one that doesn't seem be
in mainline. Was there a good reason for not being sent upstream? Maybe it was
rejected for some reason and I failed to find the discussion.
Because the patch is completely bogus, and thus so is the CVE.
The variable initializer clears out the entire structure.
Until you can show compiler output from gcc that shows it not
initializing the structure I will not apply this patch because I know
that it faithfully does.
From: Edward Cree <hidden> Date: 2016-08-23 16:50:36
On 23/08/16 16:36, Eric Dumazet wrote:
On Tue, 2016-08-23 at 08:05 -0700, Joe Perches wrote:
quoted
A compiler does not have a standards based requirement to
initialize arbitrary padding bytes.
I believe gcc always does zero all padding anyway.
Even if the current standards are lazy (are they, I did not check ?),
security needs would call for a sane compiler behavior and changing the
standards accordingly.
Sadly C99 is: section 6.2.6.1.6 (in draft N1256) says
"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."
with a footnote (42) reading
"Thus, for example, structure assignment need not copy any padding bits."
In C11 (or, at least, draft N1570), the corresponding text is identical,
only the footnote number has changed (51).
HOWEVER, section 6.7.9.10 has changed, and now (for static objects) reads:
"if it is an aggregate, every member is initialized (recursively)
according to these rules, _and_any_padding_is_initialized_to_zero_bits_"
(emphasis added).
On the other hand, a sufficiently pedantic (and perverse) language lawyer
could argue that the initialiser only "specifies the initial value stored
in an object" (6.7.9.8), and that when a value is 'stored in an object'
section 6.2.6.1.6 applies to the storing process after the initial value
has been constructed.
On the gripping hand, such an interpretation would cause the new 6.7.9.10
language to have no observable effect, and thus clearly cannot be what was
intended by the C11 committee.
Aren't you glad you asked?
In any case, there is more to life than standards lawyering, compilers
that don't zero the padding bytes are as a practical matter broken even
though not in violation of the (C99) standard. Besides, it's not as if
Linux doesn't already have requirements on its compilers that go beyond
the standards...
-Ed
@@ -1435,11 +1435,13 @@ static int ethtool_reset(struct net_device *dev, char __user *useraddr) static int ethtool_get_wol(struct net_device *dev, char __user *useraddr) {- struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };+ struct ethtool_wolinfo wol; if (!dev->ethtool_ops->get_wol) return -EOPNOTSUPP;+ memset(&wol, 0, sizeof(struct ethtool_wolinfo));+ wol.cmd = ETHTOOL_GWOL; dev->ethtool_ops->get_wol(dev, &wol); if (copy_to_user(useraddr, &wol, sizeof(wol)))
This would suggest a compiler bug to me.
A compiler does not have a standards based requirement to
initialize arbitrary padding bytes.
I believe gcc always does zero all padding anyway.
quoted
I checked that my compiler does properly put zeros there, even in the
padding area.
If we can not rely on such constructs, we have hundreds of similar
patches to submit.
True.
From a practical point of view, does any compiler used for
kernel compilation (gcc/icc/llvm/any others?) not always
perform zero padding of alignment bytes?
From: Ben Hutchings <hidden> Date: 2016-08-23 17:33:23
On Tue, 2016-08-23 at 07:21 -0700, Eric Dumazet wrote:
On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote:
quoted
quoted
quoted
From: Avijit Kanti Das <redacted>
memset() the structure ethtool_wolinfo that has padded bytes
but the padded bytes have not been zeroed out.
Change-Id: If3fd2d872a1b1ab9521d937b86a29fc468a8bbfe
Unfortunately the C standard does not guarantee that padding bytes are
initialised (at least not for automatic storage).
[...]
If we can not rely on such constructs, we have hundreds of similar
patches to submit.
[...]
Many such patches have been applied and can be found with:
git log --author=kangjielu@gmail.com
Ben.
--
Ben Hutchings
The program is absolutely right; therefore, the computer must be wrong.
From: Ben Hutchings <hidden> Date: 2016-08-23 17:35:33
On Tue, 2016-08-23 at 09:40 -0700, David Miller wrote:
From: Luis Henriques <redacted>
Date: Tue, 23 Aug 2016 14:41:07 +0100
quoted
Digging through some old CVEs I came across this one that doesn't
seem be
quoted
in mainline. Was there a good reason for not being sent upstream?
Maybe it was
quoted
rejected for some reason and I failed to find the discussion.
Because the patch is completely bogus, and thus so is the CVE.
The variable initializer clears out the entire structure.
Until you can show compiler output from gcc that shows it not
initializing the structure I will not apply this patch because I know
that it faithfully does.
On some versions and architectures. Can you guarantee that you will
notice when an exception appears?
Ben.
--
Ben Hutchings
The program is absolutely right; therefore, the computer must be wrong.
From: David Miller <davem@davemloft.net> Date: 2016-08-23 18:24:13
From: Ben Hutchings <redacted>
Date: Tue, 23 Aug 2016 18:35:27 +0100
On Tue, 2016-08-23 at 09:40 -0700, David Miller wrote:
quoted
From: Luis Henriques <redacted>
Date: Tue, 23 Aug 2016 14:41:07 +0100
quoted
Digging through some old CVEs I came across this one that doesn't
seem be
quoted
in mainline. Was there a good reason for not being sent upstream?
Maybe it was
quoted
rejected for some reason and I failed to find the discussion.
Because the patch is completely bogus, and thus so is the CVE.
The variable initializer clears out the entire structure.
Until you can show compiler output from gcc that shows it not
initializing the structure I will not apply this patch because I know
that it faithfully does.
On some versions and architectures. Can you guarantee that you will
notice when an exception appears?
Again, show me the assembler output exhibiting the lack of
initialization, for this specific structure and situation.
That's all that I'm asking.
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2016-08-23 20:10:03
On Tue, Aug 23, 2016 at 11:24:06AM -0700, David Miller wrote:
quoted
On some versions and architectures. Can you guarantee that you will
notice when an exception appears?
Again, show me the assembler output exhibiting the lack of
initialization, for this specific structure and situation.
That's all that I'm asking.
... and then we can file a bug report against the sodding compiler. Note
that
struct ethtool_wolinfo {
__u32 cmd;
__u32 supported;
__u32 wolopts;
__u8 sopass[SOPASS_MAX]; // 6, actually
};
is not going to *have* padding. Not on anything even remotely sane.
If array of 6 char as member of a struct requires 64bit alignment on some
architecture, I would really like some of what the designers of that ABI
must have been smoking.
Initializer might be allowed to leave padding uninitialized. But all fields
_must_ be initialized, the missing initializers treated exactly as they
would've been for a static-duration object (C99 6.7.8p19). And that is
going to cover everything in that sucker. It's not a function of compiler -
only of C ABI on given target.
From: Joe Perches <joe@perches.com> Date: 2016-08-23 20:36:35
On Tue, 2016-08-23 at 21:09 +0100, Al Viro wrote:
On Tue, Aug 23, 2016 at 11:24:06AM -0700, David Miller wrote:
quoted
quoted
On some versions and architectures. Can you guarantee that you will
notice when an exception appears?
Again, show me the assembler output exhibiting the lack of
initialization, for this specific structure and situation.
That's all that I'm asking.
... and then we can file a bug report against the sodding compiler. Note
that
struct ethtool_wolinfo {
__u32 cmd;
__u32 supported;
__u32 wolopts;
__u8 sopass[SOPASS_MAX]; // 6, actually
};
is not going to *have* padding. Not on anything even remotely sane.
If array of 6 char as member of a struct requires 64bit alignment on some
architecture, I would really like some of what the designers of that ABI
must have been smoking.
From: Lennart Sorensen <hidden> Date: 2016-08-23 20:59:41
On Tue, Aug 23, 2016 at 01:34:05PM -0700, Joe Perches wrote:
On Tue, 2016-08-23 at 21:09 +0100, Al Viro wrote:
quoted
On Tue, Aug 23, 2016 at 11:24:06AM -0700, David Miller wrote:
... and then we can file a bug report against the sodding compiler. Note
that
struct ethtool_wolinfo {
__u32 cmd;
__u32 supported;
__u32 wolopts;
__u8 sopass[SOPASS_MAX]; // 6, actually
};
is not going to *have* padding. Not on anything even remotely sane.
If array of 6 char as member of a struct requires 64bit alignment on some
architecture, I would really like some of what the designers of that ABI
must have been smoking.
That would be padding after the structure elements.
I think what was meant is that it won't add padding in the middle of the
structure due to alignment, ie it isn't doing:
struct ethtool_wolinfo {
__u32 cmd; /* 0 4 */
__u32 supported; /* 4 4 */
__u32 wolopts; /* 8 4 */
<4 bytes padding here>
__u8 sopass[6]; /* 16 6 */
};
which would have 4 bytes of padding in the middle between wolopts
and sopass.
I would not think it is the compilers job to worry about what is after
your structure elements, since you shouldn't be going there.
--
Len Sorensen
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2016-08-23 21:26:49
On Tue, Aug 23, 2016 at 04:49:33PM -0400, Lennart Sorensen wrote:
That would be padding after the structure elements.
I think what was meant is that it won't add padding in the middle of the
structure due to alignment, ie it isn't doing:
struct ethtool_wolinfo {
__u32 cmd; /* 0 4 */
__u32 supported; /* 4 4 */
__u32 wolopts; /* 8 4 */
<4 bytes padding here>
__u8 sopass[6]; /* 16 6 */
};
which would have 4 bytes of padding in the middle between wolopts
and sopass.
I would not think it is the compilers job to worry about what is after
your structure elements, since you shouldn't be going there.
Sadly, sizeof is what we use when copying that sucker to userland. So these
padding bits in the end would've leaked, true enough, and the case is somewhat
weaker. And any normal architecture will have those, but then any such
architecture will have no more trouble zeroing a 32bit value than 16bit one.
From: Lennart Sorensen <hidden> Date: 2016-08-24 14:03:23
On Tue, Aug 23, 2016 at 10:25:45PM +0100, Al Viro wrote:
Sadly, sizeof is what we use when copying that sucker to userland. So these
padding bits in the end would've leaked, true enough, and the case is somewhat
weaker. And any normal architecture will have those, but then any such
architecture will have no more trouble zeroing a 32bit value than 16bit one.
Hmm, good point. Too bad I don't see a compiler option of "zero all
padding in structs". Certainly generating the code should not really
be that different.
I see someone did request it 2 years ago:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63479
--
Len Sorensen
From: Hannes Frederic Sowa <hidden> Date: 2016-08-24 20:36:18
On 24.08.2016 16:03, Lennart Sorensen wrote:
On Tue, Aug 23, 2016 at 10:25:45PM +0100, Al Viro wrote:
quoted
Sadly, sizeof is what we use when copying that sucker to userland. So these
padding bits in the end would've leaked, true enough, and the case is somewhat
weaker. And any normal architecture will have those, but then any such
architecture will have no more trouble zeroing a 32bit value than 16bit one.
Hmm, good point. Too bad I don't see a compiler option of "zero all
padding in structs". Certainly generating the code should not really
be that different.
I see someone did request it 2 years ago:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63479
I don't think this is sufficient. Basically if you write one field in a
struct after a memset again, the compiler is allowed by the standard to
write padding bytes again, causing them to be undefined.
If we want to go down this route, probably the only option is to add
__attribute__((pack)) those structs to just have no padding at all, thus
breaking uapi.
E.g. the x11 protocol implementation specifies padding bytes in their
binary representation of the wire protocol to limit the leaking:
https://cgit.freedesktop.org/xorg/proto/xproto/tree/Xproto.h
... which would be another option.
Bye,
Hannes
I don't think this is sufficient. Basically if you write one field in a
struct after a memset again, the compiler is allowed by the standard to
write padding bytes again, causing them to be undefined.
The question is simply what gcc actually does. The rest is C language
lawyering and since the kernel isn't written to the C language spec but
to gcc only gcc matters.
Alan