netdev - Re: [PATCH net] ipv4: Fix use-after-free when flushing FIB tables

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: <20171219184913.GA18980@splinter>
Date:   Tue, 19 Dec 2017 20:49:13 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Ido Schimmel <idosch@...lanox.com>,
        Netdev <netdev@...r.kernel.org>,
        "Duyck, Alexander H" <alexander.h.duyck@...el.com>,
        Fengguang Wu <fengguang.wu@...el.com>,
        David Ahern <dsahern@...il.com>, mlxsw@...lanox.com
Subject: Re: [PATCH net] ipv4: Fix use-after-free when flushing FIB tables

On Tue, Dec 19, 2017 at 09:34:16AM -0800, Alexander Duyck wrote:
> That seems like unneeded complexity when the issue is just the order
> that these were created in versus the order they are freed in. As long
> as we always destroy the one containing the alias before the one that
> has the actual data we don't need to have a reference count. Basically
> the issue is the bring-up and the tear-down order. It isn't something
> that really needs a reference count since it would always be either 1
> or 2. My preference would be to just add a comment explaining that
> local must always be destroyed before the main trie in order to
> guarantee that there are no external references to the data contained
> in main when it is freed.
> 
> The one question I have in all this is if I did the bring-up in the
> right order in the first place. I'm wondering if local should be where
> the combined trie lives instead of main. Local is currently destroyed
> after main anyway so I wonder if it wouldn't have been better if
> everything lived in local since from what I can tell it looks like we
> add rules for local first before we do so in main. The complexity of
> that patch would be higher though since the patch would need to be
> much larger and touch multiple files.

I decided to go with the original patch because it resulted in a very
small diff (patch is needed in -stable as well), but I agree with Dave
about it not being explicit enough.

How about I'll send v2 with a comment and then we can try Alex's
suggestion in net-next?

Thanks