From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eric Dumazet <eric.dumazet@gmail.com>
Subject: Re: [PATCH net 0/8] net: fix uninit-values in networking stack
Date: Sun, 8 Apr 2018 09:38:13 -0700
Message-ID: <3748e283-5172-199c-660d-6814afe51823@gmail.com>
References: <20180407204243.176626-1-edumazet@google.com>
 <20180407.224025.1923207101579714958.davem@davemloft.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org
To: David Miller <davem@davemloft.net>, edumazet@google.com
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-qk0-f193.google.com ([209.85.220.193]:46151 "EHLO
        mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751886AbeDHQiR (ORCPT
        <rfc822;netdev@vger.kernel.org>); Sun, 8 Apr 2018 12:38:17 -0400
Received: by mail-qk0-f193.google.com with SMTP id p67so6697112qke.13
        for <netdev@vger.kernel.org>; Sun, 08 Apr 2018 09:38:16 -0700 (PDT)
In-Reply-To: <20180407.224025.1923207101579714958.davem@davemloft.net>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>



On 04/07/2018 07:40 PM, David Miller wrote:
> From: Eric Dumazet <edumazet@google.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.