Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S934416AbdDGRKv (ORCPT <rfc822;w@1wt.eu>);
        Fri, 7 Apr 2017 13:10:51 -0400
Received: from mail-wr0-f176.google.com ([209.85.128.176]:33120 "EHLO
        mail-wr0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S932537AbdDGRKk (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 7 Apr 2017 13:10:40 -0400
MIME-Version: 1.0
In-Reply-To: <1491475750.10124.72.camel@edumazet-glaptop3.roam.corp.google.com>
References: <CAAeHK+zJ20r8O=MhPY69UPK7sObODp2ByK5136JXgw5U9SMofw@mail.gmail.com>
 <CANn89iKuT+VBZqbJ4=hTAgUbnhuZ0berg0PK+imd1CKuWLy1eQ@mail.gmail.com>
 <CAM_iQpUittpMt-8BpNePWv7iTDasCZuNYTEPnu=K-4sFK5qiBg@mail.gmail.com>
 <1491360338.10124.39.camel@edumazet-glaptop3.roam.corp.google.com>
 <CAM_iQpUk_=oeGoYLb4DGc2+mNw+3Z08mw4wVNKAaDZgsbB1i+g@mail.gmail.com> <1491475750.10124.72.camel@edumazet-glaptop3.roam.corp.google.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 7 Apr 2017 10:10:18 -0700
Message-ID: <CAM_iQpWaHvjUAhCWQ9v6FarfuuRGdtkuB1mY4MziX0ixfc9mrw@mail.gmail.com>
Subject: Re: net/ipv4: use-after-free in ipv4_mtu
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>,
        Andrey Konovalov <andreyknvl@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        netdev <netdev@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Dmitry Vyukov <dvyukov@google.com>, Kostya Serebryany <kcc@google.com>,
        syzkaller <syzkaller@googlegroups.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1562
Lines: 54

On Thu, Apr 6, 2017 at 3:49 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2017-04-05 at 15:33 -0700, Cong Wang wrote:
>
>> Good find! I missed the refcnt in rt_set_nexthop() before that commit.
>>
>> We need to revert that commit to restore the refcnt for fib_info.
>
> Well, there are other spots , in decnet and IPv6.

IPv6 is very different, it copies or steals the metrics from mx6_config:

static int fib6_commit_metrics(struct dst_entry *dst, struct mx6_config *mxc)
{
        if (!mxc->mx)
                return 0;

        if (dst->flags & DST_HOST) {
                u32 *mp = dst_metrics_write_ptr(dst);

                if (unlikely(!mp))
                        return -ENOMEM;

                fib6_copy_metrics(mp, mxc);
        } else {
                dst_init_metrics(dst, mxc->mx, false);

                /* We've stolen mx now. */
                mxc->mx = NULL;
        }

        return 0;
}

so probably doesn't need a refcnt.

Decnet has already done the refcnt'ing, see dn_fib_semantic_match().


>
> This is why my original mail stated the problem was in the calls to :
>
> dst_init_metrics(&rt->dst, fi->fib_metrics, true);
>
> Lets do not think in "reverting" spirit, but adding the missing bits.
>
> The problem here is that the metrics should not be freed until last user
> is gone.
>
> So maybe a refcount should be added to metrics, and we do not have to
> add a fib pointer again in all dsts.
>

Good point, but it is harder than just revert given that fact that dst metrics
is a magic pointer to an array and COW.