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

[Vegard Nossum]

On 20 May 2016 at 15:43, Kangjie Lu <kangjielu@xxxxxxxxx> wrote:
>
>
> On Friday, May 20, 2016, Vegard Nossum <vegard.nossum@xxxxxxxxx> wrote:
>>
>> On 19 May 2016 at 11:08, Jiri Slaby <jslaby@xxxxxxx> wrote:
>> > From: Kangjie Lu <kangjielu@xxxxxxxxx>
>> >
>> > 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@xxxxxxxxxx>
>> > Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
>> > Signed-off-by: Jiri Slaby <jslaby@xxxxxxx>
>> > ---
>> >  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