* CVE-2014-9900 fix is not upstream @ 2016-08-23 13:41 Luis Henriques 2016-08-23 13:41 ` net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() Luis Henriques 2016-08-23 16:40 ` CVE-2014-9900 fix is not upstream David Miller 0 siblings, 2 replies; 23+ messages in thread From: Luis Henriques @ 2016-08-23 13:41 UTC (permalink / raw) To: Avijit Kanti Das; +Cc: David S . Miller, Ben Hutchings, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() 2016-08-23 13:41 CVE-2014-9900 fix is not upstream Luis Henriques @ 2016-08-23 13:41 ` Luis Henriques 2016-08-23 14:06 ` Joe Perches 2016-08-23 14:21 ` Eric Dumazet 2016-08-23 16:40 ` CVE-2014-9900 fix is not upstream David Miller 1 sibling, 2 replies; 23+ messages in thread From: Luis Henriques @ 2016-08-23 13:41 UTC (permalink / raw) To: Avijit Kanti Das; +Cc: David S . Miller, Ben Hutchings, netdev, linux-kernel From: Avijit Kanti Das <avijitnsec@codeaurora.org> 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 <avijitnsec@codeaurora.org> --- net/core/ethtool.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 977489820eb9..6bf6362e8114 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -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))) ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() 2016-08-23 13:41 ` net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() Luis Henriques @ 2016-08-23 14:06 ` Joe Perches [not found] ` <CAOp4FwRxfE61azV78TZ7EKESQZzRU2Pfkc2GJ9j3MV7pr80qew@mail.gmail.com> 2016-08-23 14:21 ` Eric Dumazet 1 sibling, 1 reply; 23+ messages in thread From: Joe Perches @ 2016-08-23 14:06 UTC (permalink / raw) To: Luis Henriques, Avijit Kanti Das Cc: David S . Miller, Ben Hutchings, netdev, linux-kernel On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote: > From: Avijit Kanti Das <avijitnsec@codeaurora.org> > > 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. > diff --git a/net/core/ethtool.c b/net/core/ethtool.c [] > @@ -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))) ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <CAOp4FwRxfE61azV78TZ7EKESQZzRU2Pfkc2GJ9j3MV7pr80qew@mail.gmail.com>]
* Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() [not found] ` <CAOp4FwRxfE61azV78TZ7EKESQZzRU2Pfkc2GJ9j3MV7pr80qew@mail.gmail.com> @ 2016-08-23 15:40 ` Joe Perches 0 siblings, 0 replies; 23+ messages in thread From: Joe Perches @ 2016-08-23 15:40 UTC (permalink / raw) To: Loganaden Velvindron Cc: Luis Henriques, Avijit Kanti Das, David S . Miller, Ben Hutchings, netdev, linux-kernel On Tue, 2016-08-23 at 19:27 +0400, Loganaden Velvindron wrote: > Better be safe than sorry. Better still would be to create a tool via something like coccinelle that could be run on the entire kernel than submit a single patch for a construct that likely occurs dozens of times in the kernel source tree. (and please do not send html formatted emails to lkml) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() 2016-08-23 13:41 ` net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() Luis Henriques 2016-08-23 14:06 ` Joe Perches @ 2016-08-23 14:21 ` Eric Dumazet 2016-08-23 15:05 ` Joe Perches 2016-08-23 17:33 ` Ben Hutchings 1 sibling, 2 replies; 23+ messages in thread From: Eric Dumazet @ 2016-08-23 14:21 UTC (permalink / raw) To: Luis Henriques Cc: Avijit Kanti Das, David S . Miller, Ben Hutchings, netdev, linux-kernel On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote: > From: Avijit Kanti Das <avijitnsec@codeaurora.org> > > 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 <avijitnsec@codeaurora.org> > --- > net/core/ethtool.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 977489820eb9..6bf6362e8114 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -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. 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. 3c1c: 48 c7 84 24 84 00 00 movq $0x0,0x84(%rsp) 3c23: 00 00 00 00 00 3c28: 48 c7 84 24 8c 00 00 movq $0x0,0x8c(%rsp) 3c2f: 00 00 00 00 00 3c34: c7 84 24 94 00 00 00 movl $0x0,0x94(%rsp) 3c3b: 00 00 00 00 3c3f: c7 84 24 84 00 00 00 movl $0x5,0x84(%rsp) 3c46: 05 00 00 00 3c4a: 4d 8b b5 18 02 00 00 mov 0x218(%r13),%r14 3c51: 49 8b 46 28 mov 0x28(%r14),%rax 3c55: 48 85 c0 test %rax,%rax 3c58: 0f 84 9f 0d 00 00 je 49fd <dev_ethtool+0x1d3d> 3c5e: 48 8d b4 24 84 00 00 lea 0x84(%rsp),%rsi 3c65: 00 3c66: 4c 89 ef mov %r13,%rdi 3c69: 41 bc f2 ff ff ff mov $0xfffffff2,%r12d 3c6f: ff d0 callq *%rax 3c71: be 0e 03 00 00 mov $0x30e,%esi 3c76: 48 c7 c7 00 00 00 00 mov $0x0,%rdi 3c79: R_X86_64_32S .rodata.str1.8 3c7d: e8 00 00 00 00 callq 3c82 <dev_ethtool+0xfc2> 3c7e: R_X86_64_PC32 __might_fault-0x4 3c82: 48 8d b4 24 84 00 00 lea 0x84(%rsp),%rsi 3c89: 00 3c8a: ba 14 00 00 00 mov $0x14,%edx 3c8f: 48 89 df mov %rbx,%rdi 3c92: e8 00 00 00 00 callq 3c97 <dev_ethtool+0xfd7> 3c93: R_X86_64_PC32 _copy_to_user-0x4 3c97: 48 85 c0 test %rax,%rax ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() 2016-08-23 14:21 ` Eric Dumazet @ 2016-08-23 15:05 ` Joe Perches 2016-08-23 15:36 ` Eric Dumazet 2016-08-23 17:15 ` Vegard Nossum 2016-08-23 17:33 ` Ben Hutchings 1 sibling, 2 replies; 23+ messages in thread From: Joe Perches @ 2016-08-23 15:05 UTC (permalink / raw) To: Eric Dumazet, Luis Henriques Cc: Avijit Kanti Das, David S . Miller, Ben Hutchings, netdev, linux-kernel On Tue, 2016-08-23 at 07:21 -0700, Eric Dumazet wrote: > On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote: > > From: Avijit Kanti Das <avijitnsec@codeaurora.org> > > > > memset() the structure ethtool_wolinfo that has padded bytes > > but the padded bytes have not been zeroed out. [] > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c [] > > @@ -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. > 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? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() 2016-08-23 15:05 ` Joe Perches @ 2016-08-23 15:36 ` Eric Dumazet 2016-08-23 16:38 ` Andrey Ryabinin 2016-08-23 16:46 ` Edward Cree 2016-08-23 17:15 ` Vegard Nossum 1 sibling, 2 replies; 23+ messages in thread From: Eric Dumazet @ 2016-08-23 15:36 UTC (permalink / raw) To: Joe Perches Cc: Luis Henriques, Avijit Kanti Das, David S . Miller, Ben Hutchings, netdev, linux-kernel 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() 2016-08-23 15:36 ` Eric Dumazet @ 2016-08-23 16:38 ` Andrey Ryabinin 2016-08-23 16:46 ` Edward Cree 1 sibling, 0 replies; 23+ messages in thread From: Andrey Ryabinin @ 2016-08-23 16:38 UTC (permalink / raw) To: Eric Dumazet Cc: Joe Perches, Luis Henriques, Avijit Kanti Das, David S . Miller, Ben Hutchings, netdev, LKML 2016-08-23 18:36 GMT+03:00 Eric Dumazet <eric.dumazet@gmail.com>: > 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. > C11 guarantees zeroed padding. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() 2016-08-23 15:36 ` Eric Dumazet 2016-08-23 16:38 ` Andrey Ryabinin @ 2016-08-23 16:46 ` Edward Cree 1 sibling, 0 replies; 23+ messages in thread From: Edward Cree @ 2016-08-23 16:46 UTC (permalink / raw) To: Eric Dumazet, Joe Perches Cc: Luis Henriques, Avijit Kanti Das, David S . Miller, Ben Hutchings, netdev, linux-kernel On 23/08/16 16:36, Eric Dumazet wrote: > 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. > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() 2016-08-23 15:05 ` Joe Perches 2016-08-23 15:36 ` Eric Dumazet @ 2016-08-23 17:15 ` Vegard Nossum 1 sibling, 0 replies; 23+ messages in thread From: Vegard Nossum @ 2016-08-23 17:15 UTC (permalink / raw) To: Joe Perches Cc: Eric Dumazet, Luis Henriques, Avijit Kanti Das, David S . Miller, Ben Hutchings, Linux Netdev List, LKML On 23 August 2016 at 17:05, Joe Perches <joe@perches.com> wrote: > On Tue, 2016-08-23 at 07:21 -0700, Eric Dumazet wrote: >> On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote: >> > From: Avijit Kanti Das <avijitnsec@codeaurora.org> >> > >> > memset() the structure ethtool_wolinfo that has padded bytes >> > but the padded bytes have not been zeroed out. > [] >> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > [] >> > @@ -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. > >> 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? > gcc often does not do it, depends on a few factors though: https://lkml.org/lkml/2016/5/20/389 Vegard ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() 2016-08-23 14:21 ` Eric Dumazet 2016-08-23 15:05 ` Joe Perches @ 2016-08-23 17:33 ` Ben Hutchings 1 sibling, 0 replies; 23+ messages in thread From: Ben Hutchings @ 2016-08-23 17:33 UTC (permalink / raw) To: Eric Dumazet, Luis Henriques Cc: Avijit Kanti Das, David S . Miller, netdev, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1772 bytes --] On Tue, 2016-08-23 at 07:21 -0700, Eric Dumazet wrote: > On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote: > > > > > > From: Avijit Kanti Das <avijitnsec@codeaurora.org> > > > > 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 <avijitnsec@codeaurora.org> > > --- > > net/core/ethtool.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > > index 977489820eb9..6bf6362e8114 100644 > > --- a/net/core/ethtool.c > > +++ b/net/core/ethtool.c > > @@ -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. 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. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-23 13:41 CVE-2014-9900 fix is not upstream Luis Henriques 2016-08-23 13:41 ` net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() Luis Henriques @ 2016-08-23 16:40 ` David Miller 2016-08-23 17:35 ` Ben Hutchings 1 sibling, 1 reply; 23+ messages in thread From: David Miller @ 2016-08-23 16:40 UTC (permalink / raw) To: luis.henriques; +Cc: avijitnsec, ben, netdev, linux-kernel From: Luis Henriques <luis.henriques@canonical.com> 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-23 16:40 ` CVE-2014-9900 fix is not upstream David Miller @ 2016-08-23 17:35 ` Ben Hutchings 2016-08-23 18:24 ` David Miller 0 siblings, 1 reply; 23+ messages in thread From: Ben Hutchings @ 2016-08-23 17:35 UTC (permalink / raw) To: David Miller, luis.henriques; +Cc: avijitnsec, netdev, linux-kernel [-- Attachment #1: Type: text/plain, Size: 912 bytes --] On Tue, 2016-08-23 at 09:40 -0700, David Miller wrote: > From: Luis Henriques <luis.henriques@canonical.com> > 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. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-23 17:35 ` Ben Hutchings @ 2016-08-23 18:24 ` David Miller 2016-08-23 20:09 ` Al Viro 0 siblings, 1 reply; 23+ messages in thread From: David Miller @ 2016-08-23 18:24 UTC (permalink / raw) To: ben; +Cc: luis.henriques, avijitnsec, netdev, linux-kernel From: Ben Hutchings <ben@decadent.org.uk> Date: Tue, 23 Aug 2016 18:35:27 +0100 > On Tue, 2016-08-23 at 09:40 -0700, David Miller wrote: >> From: Luis Henriques <luis.henriques@canonical.com> >> 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-23 18:24 ` David Miller @ 2016-08-23 20:09 ` Al Viro 2016-08-23 20:34 ` Joe Perches 0 siblings, 1 reply; 23+ messages in thread From: Al Viro @ 2016-08-23 20:09 UTC (permalink / raw) To: David Miller; +Cc: ben, luis.henriques, avijitnsec, netdev, linux-kernel 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; __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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-23 20:09 ` Al Viro @ 2016-08-23 20:34 ` Joe Perches 2016-08-23 20:49 ` Lennart Sorensen 0 siblings, 1 reply; 23+ messages in thread From: Joe Perches @ 2016-08-23 20:34 UTC (permalink / raw) To: Al Viro, David Miller Cc: ben, luis.henriques, avijitnsec, netdev, linux-kernel 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; > __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. 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 */ }; ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-23 20:34 ` Joe Perches @ 2016-08-23 20:49 ` Lennart Sorensen 2016-08-23 21:25 ` Al Viro 0 siblings, 1 reply; 23+ messages in thread From: Lennart Sorensen @ 2016-08-23 20:49 UTC (permalink / raw) To: Joe Perches Cc: Al Viro, David Miller, ben, luis.henriques, avijitnsec, netdev, linux-kernel 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; > > __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. > > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-23 20:49 ` Lennart Sorensen @ 2016-08-23 21:25 ` Al Viro 2016-08-24 14:03 ` Lennart Sorensen 0 siblings, 1 reply; 23+ messages in thread From: Al Viro @ 2016-08-23 21:25 UTC (permalink / raw) To: Lennart Sorensen Cc: Joe Perches, David Miller, ben, luis.henriques, avijitnsec, netdev, linux-kernel 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-23 21:25 ` Al Viro @ 2016-08-24 14:03 ` Lennart Sorensen 2016-08-24 20:36 ` Hannes Frederic Sowa 0 siblings, 1 reply; 23+ messages in thread From: Lennart Sorensen @ 2016-08-24 14:03 UTC (permalink / raw) To: Al Viro Cc: Joe Perches, David Miller, ben, luis.henriques, avijitnsec, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-24 14:03 ` Lennart Sorensen @ 2016-08-24 20:36 ` Hannes Frederic Sowa 2016-08-25 12:40 ` Johannes Berg 2016-08-25 15:14 ` One Thousand Gnomes 0 siblings, 2 replies; 23+ messages in thread From: Hannes Frederic Sowa @ 2016-08-24 20:36 UTC (permalink / raw) To: Lennart Sorensen, Al Viro Cc: Joe Perches, David Miller, ben, luis.henriques, avijitnsec, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-24 20:36 ` Hannes Frederic Sowa @ 2016-08-25 12:40 ` Johannes Berg 2016-08-25 12:41 ` Johannes Berg 2016-08-25 15:14 ` One Thousand Gnomes 1 sibling, 1 reply; 23+ messages in thread From: Johannes Berg @ 2016-08-25 12:40 UTC (permalink / raw) To: Hannes Frederic Sowa, Lennart Sorensen, Al Viro Cc: Joe Perches, David Miller, ben, luis.henriques, avijitnsec, netdev, linux-kernel > 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; __u8 sopass[SOPASS_MAX]; // 6, actually }; we could do struct ethtool_wolinfo { __u32 cmd; __u32 supported; __u32 wolopts; __u8 sopass[SOPASS_MAX]; // 6, actually __u8 reserved[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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-25 12:40 ` Johannes Berg @ 2016-08-25 12:41 ` Johannes Berg 0 siblings, 0 replies; 23+ messages in thread From: Johannes Berg @ 2016-08-25 12:41 UTC (permalink / raw) To: Hannes Frederic Sowa, Lennart Sorensen, Al Viro Cc: Joe Perches, David Miller, ben, luis.henriques, avijitnsec, netdev, linux-kernel > struct ethtool_wolinfo { > __u32 cmd; > __u32 supported; > __u32 wolopts; > __u8 sopass[SOPASS_MAX]; // 6, actually > }; > > we could do > > struct ethtool_wolinfo { > __u32 cmd; > __u32 supported; > __u32 wolopts; > __u8 sopass[SOPASS_MAX]; // 6, actually > __u8 reserved[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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: CVE-2014-9900 fix is not upstream 2016-08-24 20:36 ` Hannes Frederic Sowa 2016-08-25 12:40 ` Johannes Berg @ 2016-08-25 15:14 ` One Thousand Gnomes 1 sibling, 0 replies; 23+ messages in thread From: One Thousand Gnomes @ 2016-08-25 15:14 UTC (permalink / raw) To: Hannes Frederic Sowa Cc: Lennart Sorensen, Al Viro, Joe Perches, David Miller, ben, luis.henriques, avijitnsec, netdev, linux-kernel > > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-08-25 16:10 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-23 13:41 CVE-2014-9900 fix is not upstream Luis Henriques 2016-08-23 13:41 ` net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() Luis Henriques 2016-08-23 14:06 ` Joe Perches [not found] ` <CAOp4FwRxfE61azV78TZ7EKESQZzRU2Pfkc2GJ9j3MV7pr80qew@mail.gmail.com> 2016-08-23 15:40 ` Joe Perches 2016-08-23 14:21 ` Eric Dumazet 2016-08-23 15:05 ` Joe Perches 2016-08-23 15:36 ` Eric Dumazet 2016-08-23 16:38 ` Andrey Ryabinin 2016-08-23 16:46 ` Edward Cree 2016-08-23 17:15 ` Vegard Nossum 2016-08-23 17:33 ` Ben Hutchings 2016-08-23 16:40 ` CVE-2014-9900 fix is not upstream David Miller 2016-08-23 17:35 ` Ben Hutchings 2016-08-23 18:24 ` David Miller 2016-08-23 20:09 ` Al Viro 2016-08-23 20:34 ` Joe Perches 2016-08-23 20:49 ` Lennart Sorensen 2016-08-23 21:25 ` Al Viro 2016-08-24 14:03 ` Lennart Sorensen 2016-08-24 20:36 ` Hannes Frederic Sowa 2016-08-25 12:40 ` Johannes Berg 2016-08-25 12:41 ` Johannes Berg 2016-08-25 15:14 ` One Thousand Gnomes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).