Re: CVE-2014-9900 fix is not upstream

[One Thousand Gnomes]

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

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

Re: CVE-2014-9900 fix is not upstream

[One Thousand Gnomes]

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

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

Re: CVE-2014-9900 fix is not upstream

[Johannes Berg]

> struct ethtool_wolinfo {
> __u32   cmd;
> __u32   supported;
> __u32   wolopts;
> __u8sopass[SOPASS_MAX]; // 6, actually
> };
> 
> we could do
> 
> struct ethtool_wolinfo {
> __u32   cmd;
> __u32   supported;
> __u32   wolopts;
> __u8sopass[SOPASS_MAX]; // 6, actually
>   __u8reserved[2];
> };
> 
> and then the compiler has to properly treat it, since it's no longer
> unnamed padding.
> 

Although, on some architectures, that could actually break the ABI by
changing the size, oh well.

johannes

Re: CVE-2014-9900 fix is not upstream

[Johannes Berg]

> struct ethtool_wolinfo {
> __u32   cmd;
> __u32   supported;
> __u32   wolopts;
> __u8sopass[SOPASS_MAX]; // 6, actually
> };
> 
> we could do
> 
> struct ethtool_wolinfo {
> __u32   cmd;
> __u32   supported;
> __u32   wolopts;
> __u8sopass[SOPASS_MAX]; // 6, actually
>   __u8reserved[2];
> };
> 
> and then the compiler has to properly treat it, since it's no longer
> unnamed padding.
> 

Although, on some architectures, that could actually break the ABI by
changing the size, oh well.

johannes

Re: CVE-2014-9900 fix is not upstream

[Johannes Berg]

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

We could also spell out the padding bytes as reserved, i.e. instead of

struct ethtool_wolinfo {
__u32   cmd;
__u32   supported;
__u32   wolopts;
__u8sopass[SOPASS_MAX]; // 6, actually
};

we could do

struct ethtool_wolinfo {
__u32   cmd;
__u32   supported;
__u32   wolopts;
__u8sopass[SOPASS_MAX]; // 6, actually
__u8reserved[2];
};

and then the compiler has to properly treat it, since it's no longer
unnamed padding.

Maybe somebody can come up with a smart BUILD_BUG_ON() to ensure such
structs have no padding.

That would allow us to keep the C99 initializers (which is nice) and
not have to worry about this.

johannes

Re: CVE-2014-9900 fix is not upstream

[Johannes Berg]

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

We could also spell out the padding bytes as reserved, i.e. instead of

struct ethtool_wolinfo {
__u32   cmd;
__u32   supported;
__u32   wolopts;
__u8sopass[SOPASS_MAX]; // 6, actually
};

we could do

struct ethtool_wolinfo {
__u32   cmd;
__u32   supported;
__u32   wolopts;
__u8sopass[SOPASS_MAX]; // 6, actually
__u8reserved[2];
};

and then the compiler has to properly treat it, since it's no longer
unnamed padding.

Maybe somebody can come up with a smart BUILD_BUG_ON() to ensure such
structs have no padding.

That would allow us to keep the C99 initializers (which is nice) and
not have to worry about this.

johannes

Re: CVE-2014-9900 fix is not upstream

[Hannes Frederic Sowa]

On 24.08.2016 16:03, Lennart Sorensen wrote:
> 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

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

Re: CVE-2014-9900 fix is not upstream

[Hannes Frederic Sowa]

On 24.08.2016 16:03, Lennart Sorensen wrote:
> 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

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

Re: CVE-2014-9900 fix is not upstream

[Lennart Sorensen]

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

Re: CVE-2014-9900 fix is not upstream

[Lennart Sorensen]

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

Re: CVE-2014-9900 fix is not upstream

[Al Viro]

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.

Re: CVE-2014-9900 fix is not upstream

[Al Viro]

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.

Re: CVE-2014-9900 fix is not upstream

[Lennart Sorensen]

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:
> > 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;
> > __u8sopass[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.
> 
> try this on x86-64
> 
> $ pahole -C ethtool_wolinfo vmlinux
> struct ethtool_wolinfo {
>   __u32  cmd;  /* 0 4 */
>   __u32  supported;/* 4 4 */
>   __u32  wolopts;  /* 8 4 */
>   __u8   sopass[6];/*12 6 */
> 
>   /* size: 20, cachelines: 1, members: 4 */
>   /* padding: 2 */
>   /* last cacheline: 20 bytes */
> };

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

Re: CVE-2014-9900 fix is not upstream

[Lennart Sorensen]

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:
> > 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;
> > __u8sopass[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.
> 
> try this on x86-64
> 
> $ pahole -C ethtool_wolinfo vmlinux
> struct ethtool_wolinfo {
>   __u32  cmd;  /* 0 4 */
>   __u32  supported;/* 4 4 */
>   __u32  wolopts;  /* 8 4 */
>   __u8   sopass[6];/*12 6 */
> 
>   /* size: 20, cachelines: 1, members: 4 */
>   /* padding: 2 */
>   /* last cacheline: 20 bytes */
> };

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

Re: CVE-2014-9900 fix is not upstream

[Joe Perches]

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:
> > > 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;
> __u8sopass[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.

try this on x86-64

$ pahole -C ethtool_wolinfo vmlinux
struct ethtool_wolinfo {
__u32  cmd;  /* 0 4 */
__u32  supported;/* 4 4 */
__u32  wolopts;  /* 8 4 */
__u8   sopass[6];/*12 6 */

/* size: 20, cachelines: 1, members: 4 */
/* padding: 2 */
/* last cacheline: 20 bytes */
};

Re: CVE-2014-9900 fix is not upstream

[Joe Perches]

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:
> > > 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;
> __u8sopass[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.

try this on x86-64

$ pahole -C ethtool_wolinfo vmlinux
struct ethtool_wolinfo {
__u32  cmd;  /* 0 4 */
__u32  supported;/* 4 4 */
__u32  wolopts;  /* 8 4 */
__u8   sopass[6];/*12 6 */

/* size: 20, cachelines: 1, members: 4 */
/* padding: 2 */
/* last cacheline: 20 bytes */
};

Re: CVE-2014-9900 fix is not upstream

[Al Viro]

On Tue, Aug 23, 2016 at 11:24:06AM -0700, David Miller wrote:

> > 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;
__u8sopass[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.

Re: CVE-2014-9900 fix is not upstream

[Al Viro]

On Tue, Aug 23, 2016 at 11:24:06AM -0700, David Miller wrote:

> > 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;
__u8sopass[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.

Re: CVE-2014-9900 fix is not upstream

[David Miller]

From: Ben Hutchings 
Date: Tue, 23 Aug 2016 18:35:27 +0100

> On Tue, 2016-08-23 at 09:40 -0700, David Miller wrote:
>> From: Luis Henriques 
>> 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.
> 
> 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.

Re: CVE-2014-9900 fix is not upstream

[David Miller]

From: Ben Hutchings 
Date: Tue, 23 Aug 2016 18:35:27 +0100

> On Tue, 2016-08-23 at 09:40 -0700, David Miller wrote:
>> From: Luis Henriques 
>> 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.
> 
> 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.

Re: CVE-2014-9900 fix is not upstream

[Ben Hutchings]

On Tue, 2016-08-23 at 09:40 -0700, David Miller wrote:
> From: Luis Henriques 
> 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.

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.


signature.asc
Description: This is a digitally signed message part

Re: CVE-2014-9900 fix is not upstream

[Ben Hutchings]

On Tue, 2016-08-23 at 09:40 -0700, David Miller wrote:
> From: Luis Henriques 
> 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.

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.


signature.asc
Description: This is a digitally signed message part

Re: CVE-2014-9900 fix is not upstream

[David Miller]

From: Luis Henriques 
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.

Re: CVE-2014-9900 fix is not upstream

[David Miller]

From: Luis Henriques 
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.

CVE-2014-9900 fix is not upstream

[Luis Henriques]

Hi!

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.

References:
 https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-9900
 http://source.android.com/security/bulletin/2016-08-01.html
 
https://source.codeaurora.org/quic/la/kernel/msm-3.10/commit/?id=63c317dbee97983004dffdd9f742a20d17150071

Cheers,
--
Luís

CVE-2014-9900 fix is not upstream

[Luis Henriques]

Hi!

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.

References:
 https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-9900
 http://source.android.com/security/bulletin/2016-08-01.html
 
https://source.codeaurora.org/quic/la/kernel/msm-3.10/commit/?id=63c317dbee97983004dffdd9f742a20d17150071

Cheers,
--
Luís