Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1403530 > unrolled thread

[PATCH 3.12 69/76] net: fix infoleak in rtnetlink

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3.12 69/76] net: fix infoleak in rtnetlink Jiri Slaby <jslaby@suse.cz> - 2016-05-19 11:20 +0200
    Re: [PATCH 3.12 69/76] net: fix infoleak in rtnetlink Vegard Nossum <vegard.nossum@gmail.com> - 2016-05-20 14:10 +0200
      Re: [PATCH 3.12 69/76] net: fix infoleak in rtnetlink Vegard Nossum <vegard.nossum@gmail.com> - 2016-05-20 16:30 +0200
      Re: [PATCH 3.12 69/76] net: fix infoleak in rtnetlink David Miller <davem@davemloft.net> - 2016-05-20 18:50 +0200
        Re: [PATCH 3.12 69/76] net: fix infoleak in rtnetlink Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-05-21 02:50 +0200

#1403530 — [PATCH 3.12 69/76] net: fix infoleak in rtnetlink

From: Kangjie Lu <kangjielu@gmail.com>

3.12-stable review patch.  If anyone has any objections, please let me know.

===============

[ Upstream commit 5f8e44741f9f216e33736ea4ec65ca9ac03036e6 ]

The stack object “map” has a total size of 32 bytes. Its last 4
bytes are padding generated by compiler. These padding bytes are
not initialized and sent out via “nla_put”.

Signed-off-by: Kangjie Lu <kjlu@gatech.edu>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 net/core/rtnetlink.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index fd3a16e45dd9..5093f42d7afc 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -950,14 +950,16 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 		goto nla_put_failure;
 
 	if (1) {
-		struct rtnl_link_ifmap map = {
-			.mem_start   = dev->mem_start,
-			.mem_end     = dev->mem_end,
-			.base_addr   = dev->base_addr,
-			.irq         = dev->irq,
-			.dma         = dev->dma,
-			.port        = dev->if_port,
-		};
+		struct rtnl_link_ifmap map;
+
+		memset(&map, 0, sizeof(map));
+		map.mem_start   = dev->mem_start;
+		map.mem_end     = dev->mem_end;
+		map.base_addr   = dev->base_addr;
+		map.irq         = dev->irq;
+		map.dma         = dev->dma;
+		map.port        = dev->if_port;
+
 		if (nla_put(skb, IFLA_MAP, sizeof(map), &map))
 			goto nla_put_failure;
 	}
-- 
2.8.2

[toc] | [next] | [standalone]

#1404370

On 19 May 2016 at 11:08, Jiri Slaby <jslaby@suse.cz> wrote:
> From: Kangjie Lu <kangjielu@gmail.com>
>
> 3.12-stable review patch.  If anyone has any objections, please let me know.
>
> ===============
>
> [ Upstream commit 5f8e44741f9f216e33736ea4ec65ca9ac03036e6 ]
>
> The stack object “map” has a total size of 32 bytes. Its last 4
> bytes are padding generated by compiler. These padding bytes are
> not initialized and sent out via “nla_put”.
>
> Signed-off-by: Kangjie Lu <kjlu@gatech.edu>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  net/core/rtnetlink.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index fd3a16e45dd9..5093f42d7afc 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -950,14 +950,16 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>                 goto nla_put_failure;
>
>         if (1) {
> -               struct rtnl_link_ifmap map = {
> -                       .mem_start   = dev->mem_start,
> -                       .mem_end     = dev->mem_end,
> -                       .base_addr   = dev->base_addr,
> -                       .irq         = dev->irq,
> -                       .dma         = dev->dma,
> -                       .port        = dev->if_port,
> -               };
> +               struct rtnl_link_ifmap map;
> +
> +               memset(&map, 0, sizeof(map));
> +               map.mem_start   = dev->mem_start;
> +               map.mem_end     = dev->mem_end;
> +               map.base_addr   = dev->base_addr;
> +               map.irq         = dev->irq;
> +               map.dma         = dev->dma;
> +               map.port        = dev->if_port;
> +
>                 if (nla_put(skb, IFLA_MAP, sizeof(map), &map))
>                         goto nla_put_failure;
>         }
> --
> 2.8.2
>

Just out of curiosity, was this observed in practice? I could be
wrong, but I was under the impression that using designated
initializers would zero the rest of the struct, including padding.
This seems to back that up:

http://stackoverflow.com/a/3374468/1697183

If this is indeed a real info leak, then I would assume we have much
bigger problems around the kernel.


Vegard

[toc] | [prev] | [next] | [standalone]

#1404493

On 20 May 2016 at 15:43, Kangjie Lu <kangjielu@gmail.com> wrote:
>
>
> On Friday, May 20, 2016, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>>
>> On 19 May 2016 at 11:08, Jiri Slaby <jslaby@suse.cz> wrote:
>> > From: Kangjie Lu <kangjielu@gmail.com>
>> >
>> > 3.12-stable review patch.  If anyone has any objections, please let me
>> > know.
>> >
>> > ===============
>> >
>> > [ Upstream commit 5f8e44741f9f216e33736ea4ec65ca9ac03036e6 ]
>> >
>> > The stack object “map” has a total size of 32 bytes. Its last 4
>> > bytes are padding generated by compiler. These padding bytes are
>> > not initialized and sent out via “nla_put”.
>> >
>> > Signed-off-by: Kangjie Lu <kjlu@gatech.edu>
>> > Signed-off-by: David S. Miller <davem@davemloft.net>
>> > Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> > ---
>> >  net/core/rtnetlink.c | 18 ++++++++++--------
>> >  1 file changed, 10 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> > index fd3a16e45dd9..5093f42d7afc 100644
>> > --- a/net/core/rtnetlink.c
>> > +++ b/net/core/rtnetlink.c
>> > @@ -950,14 +950,16 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>> > struct net_device *dev,
>> >                 goto nla_put_failure;
>> >
>> >         if (1) {
>> > -               struct rtnl_link_ifmap map = {
>> > -                       .mem_start   = dev->mem_start,
>> > -                       .mem_end     = dev->mem_end,
>> > -                       .base_addr   = dev->base_addr,
>> > -                       .irq         = dev->irq,
>> > -                       .dma         = dev->dma,
>> > -                       .port        = dev->if_port,
>> > -               };
>> > +               struct rtnl_link_ifmap map;
>> > +
>> > +               memset(&map, 0, sizeof(map));
>> > +               map.mem_start   = dev->mem_start;
>> > +               map.mem_end     = dev->mem_end;
>> > +               map.base_addr   = dev->base_addr;
>> > +               map.irq         = dev->irq;
>> > +               map.dma         = dev->dma;
>> > +               map.port        = dev->if_port;
>> > +
>> >                 if (nla_put(skb, IFLA_MAP, sizeof(map), &map))
>> >                         goto nla_put_failure;
>> >         }
>> > --
>> > 2.8.2
>> >
>>
>> Just out of curiosity, was this observed in practice? I could be
>> wrong, but I was under the impression that using designated
>> initializers would zero the rest of the struct, including padding.
>
>
> Yes or no.  According to my experiences, it depends on how
> it is initialized:
> if there are no variables but all constants in the bracket,
> a global initializer will be generated, which will zero the remaining bytes
> including padding; otherwise, no global initializer
> will be used, hence the remaining bytes are not initialized.
> In this case, dev is not a constant, so no global initializer
> will be used to initialize the padding bytes

I did some experiements with gcc and my observations are:

1. it doesn't depend on whether the initializer is constant or variable, but...

2. whether or not padding gets initialized depends on *which fields*
you're initializing (I assume this has to do with what instructions it
ends up using, as it might be faster to do a 32-bit mov on x86 instead
of an 8-bit one if you're initializing an 8-bit field which is
followed by 24 bits of padding, for example).

>> This seems to back that up:
>>
>> http://stackoverflow.com/a/3374468/1697183
>>
>> If this is indeed a real info leak, then I would assume we have much
>> bigger problems around the kernel.
>
>
> Could be.  We've found many such bugs.

That is pretty sad. Anyway, thanks for fixing them.


Vegard

[toc] | [prev] | [next] | [standalone]

#1404602

From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Fri, 20 May 2016 14:04:54 +0200

> Just out of curiosity, was this observed in practice? I could be
> wrong, but I was under the impression that using designated
> initializers would zero the rest of the struct, including padding.

I compiled testcases and found that the compiler does not zero out
padding when using designated initializers.

You can do the same.

For example, on sparc 32-bit, this code:

struct foo {
	int a;
	short b;
	int c;
};

extern void foo(struct foo *);

void bar(void)
{
	struct foo f = { .a = 1, .b = 2, .c = 3 };

	foo(&f);
}

gives:

	mov	1, %g1
	st	%g1, [%fp-12]
	mov	2, %g1
	sth	%g1, [%fp-8]
	mov	3, %g1
	st	%g1, [%fp-4]

It does not initialize the padding between 'b' and 'c'.

[toc] | [prev] | [next] | [standalone]

#1404780

On 20.05.2016 18:45, David Miller wrote:
> From: Vegard Nossum <vegard.nossum@gmail.com>
> Date: Fri, 20 May 2016 14:04:54 +0200
> 
>> Just out of curiosity, was this observed in practice? I could be
>> wrong, but I was under the impression that using designated
>> initializers would zero the rest of the struct, including padding.
> 
> I compiled testcases and found that the compiler does not zero out
> padding when using designated initializers.
> 
> You can do the same.
> 
> For example, on sparc 32-bit, this code:
> 
> struct foo {
> 	int a;
> 	short b;
> 	int c;
> };
> 
> extern void foo(struct foo *);
> 
> void bar(void)
> {
> 	struct foo f = { .a = 1, .b = 2, .c = 3 };
> 
> 	foo(&f);
> }
> 
> gives:
> 
> 	mov	1, %g1
> 	st	%g1, [%fp-12]
> 	mov	2, %g1
> 	sth	%g1, [%fp-8]
> 	mov	3, %g1
> 	st	%g1, [%fp-4]
> 
> It does not initialize the padding between 'b' and 'c'.

Interesting side note here is question 1 of the survey "What is C in
practice?", here:

<https://www.cl.cam.ac.uk/~pes20/cerberus/notes50-survey-discussion.html>

It seems safe right now from my understanding but we need to be careful
with future compiler optimizations, e.g. for memset, as Joseph Myers
commented on the question for future possible optimizations.

This report is also going to be presented in the C2X standard meetings,
hopefully they come up with something sensible for that.

Bye,
Hannes

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web