netdev - Re: [PATCH net 0/8] net: fix uninit-values in networking stack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3748e283-5172-199c-660d-6814afe51823@gmail.com>
Date:   Sun, 8 Apr 2018 09:38:13 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     David Miller <davem@...emloft.net>, edumazet@...gle.com
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net 0/8] net: fix uninit-values in networking stack



On 04/07/2018 07:40 PM, David Miller wrote:
> From: Eric Dumazet <edumazet@...gle.com>
> Date: Sat,  7 Apr 2018 13:42:35 -0700
> 
>> It seems syzbot got new features enabled, and fired some interesting
>> reports. Oh well.
> 
> Series applied, however in patch #7 the condition syzbot detects
> cannot happen.
> 
> In all code paths that lead to __mkroute_output() with res->type
> uninitialized, __mkroute_output() will reassign the local variable
> 'type' before reading it.

Well, we have :

u16 type = res->type;
...

       if (ipv4_is_lbcast(fl4->daddr))
                type = RTN_BROADCAST;
        else if (ipv4_is_multicast(fl4->daddr))
                type = RTN_MULTICAST;
        else if (ipv4_is_zeronet(fl4->daddr))
                return ERR_PTR(-EINVAL);

...

if (type == RTN_BROADCAST) {  /* This is where KMSAN complained */

So it looks like type could have been random at this point.

> 
> Furthermore, by doing a full structure initialization lots of
> unrelated things will be initialized now as well.

fib_result is 40 bytes on 64bit arches.

> 
> We explicitly are only setting up the "inputs" of the fib_result
> object before we call fib_lookup().  The prefixlen and other members
> have no business being initialized there.
> 

Yep

We might put all inputs at the beginning of the structure,
and output at the end. then replace sizeof() by offsetof(),
but this looks a bit convoluted and maybe risky.