netdev - Re: [Patch net] ipv4: restore rt->fi for reference counting

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: <1494296302.7796.61.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Mon, 08 May 2017 19:18:22 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     xiyou.wangcong@...il.com, netdev@...r.kernel.org,
        andreyknvl@...gle.com, edumazet@...gle.com
Subject: Re: [Patch net] ipv4: restore rt->fi for reference counting

On Mon, 2017-05-08 at 21:22 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@...il.com>
> Date: Mon, 08 May 2017 17:01:20 -0700
> 
> > On Mon, 2017-05-08 at 14:35 -0400, David Miller wrote:
> >> From: Cong Wang <xiyou.wangcong@...il.com>
> >> Date: Thu,  4 May 2017 14:54:17 -0700
> >> 
> >> > IPv4 dst could use fi->fib_metrics to store metrics but fib_info
> >> > itself is refcnt'ed, so without taking a refcnt fi and
> >> > fi->fib_metrics could be freed while dst metrics still points to
> >> > it. This triggers use-after-free as reported by Andrey twice.
> >> > 
> >> > This patch reverts commit 2860583fe840 ("ipv4: Kill rt->fi") to
> >> > restore this reference counting. It is a quick fix for -net and
> >> > -stable, for -net-next, as Eric suggested, we can consider doing
> >> > reference counting for metrics itself instead of relying on fib_info.
> >> > 
> >> > IPv6 is very different, it copies or steals the metrics from mx6_config
> >> > in fib6_commit_metrics() so probably doesn't need a refcnt.
> >> > 
> >> > Decnet has already done the refcnt'ing, see dn_fib_semantic_match().
> >> > 
> >> > Fixes: 2860583fe840 ("ipv4: Kill rt->fi")
> >> > Reported-by: Andrey Konovalov <andreyknvl@...gle.com>
> >> > Tested-by: Andrey Konovalov <andreyknvl@...gle.com>
> >> > Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> >> 
> >> Applied and queued up for -stable, thanks.
> > 
> > Although I now have on latest net tree these messages when I reboot my
> > test machine.
> > 
> > [  224.085873] unregister_netdevice: waiting for eth0 to become free. Usage count = 43
> 
> Strange, the refcounting looks quite OK in the patch you're quoting.
> I looked over it a few times and cannot figure out a possible cause
> there.
> 
> I am assuming you are quite confident it is this change?

At least, reverting the patch resolves the issue for me.

Keeping fib (and their reference to netdev) is apparently too much,
we probably need to implement a refcount on the metrics themselves,
being stand alone objects.