Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote:
> From: Avijit Kanti Das
>
> 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
> ---
> 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 00movq $0x0,0x84(%rsp)
3c23: 00 00 00 00 00
3c28: 48 c7 84 24 8c 00 00movq $0x0,0x8c(%rsp)
3c2f: 00 00 00 00 00
3c34: c7 84 24 94 00 00 00movl $0x0,0x94(%rsp)
3c3b: 00 00 00 00
3c3f: c7 84 24 84 00 00 00movl $0x5,0x84(%rsp)
3c46: 05 00 00 00
3c4a: 4d 8b b5 18 02 00 00mov0x218(%r13),%r14
3c51: 49 8b 46 28 mov0x28(%r14),%rax
3c55: 48 85 c0test %rax,%rax
3c58: 0f 84 9f 0d 00 00 je 49fd
3c5e: 48 8d b4 24 84 00 00lea0x84(%rsp),%rsi
3c65: 00
3c66: 4c 89 efmov%r13,%rdi
3c69: 41 bc f2 ff ff ff mov$0xfff2,%r12d
3c6f: ff d0 callq *%rax
3c71: be 0e 03 00 00 mov$0x30e,%esi
3c76: 48 c7 c7 00 00 00 00mov$0x0,%rdi
3c79: R_X86_64_32S .rodata.str1.8
3c7d: e8 00 00 00 00 callq 3c82
3c7e: R_X86_64_PC32 __might_fault-0x4
3c82: 48 8d b4 24 84 00 00lea0x84(%rsp),%rsi
3c89: 00
3c8a: ba 14 00 00 00 mov$0x14,%edx
3c8f: 48 89 dfmov%rbx,%rdi
3c92: e8 00 00 00 00 callq 3c97
3c93: R_X86_64_PC32 _copy_to_user-0x4
3c97: 48 85 c0test %rax,%rax
Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
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
> >
> > 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
> > ---
> > 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=kangji...@gmail.com
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: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
On 23 August 2016 at 17:05, Joe Perches 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
>> >
>> > 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
Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
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
Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
2016-08-23 18:36 GMT+03:00 Eric Dumazet : > 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.
Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
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)
Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
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.
Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
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
> >
> > 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?
Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote:
> From: Avijit Kanti Das
>
> 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)))
