linux-kernel - Re: [RFC PATCH] net: bridge: call br_multicast_del

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] [day] [month] [year] [list]

Date:   Fri, 16 Oct 2020 16:50:22 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Nikolay Aleksandrov <nikolay@...dia.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bridge@...ts.linux-foundation.org" 
        <bridge@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Roopa Prabhu <roopa@...dia.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>
Subject: Re: [RFC PATCH] net: bridge: call br_multicast_del_port before the
 port leaves

On Fri, Oct 16, 2020 at 01:43:06PM +0000, Nikolay Aleksandrov wrote:
> It can potentially use after free, multicast resources (per-cpu stats) are freed
> in br_multicast_del_port() and can be used due to a race with port state
> sync on other CPUs since the handler can still process packets. That has a
> chance of happening if vlans are not used.

Interesting, thanks for pointing this out, I haven't observed
use-after-free in my limited testing of this patch.

> Interesting that br_stp_disable_port() calls br_multicast_disable_port() which
> flushes all non-permanent mdb entries, so I'm guessing you have problem only
> with permanent ones?

Indeed, I'm testing out your L2 multicast patch.

> Perhaps we can flush them all before. Either by passing an argument to
> br_stp_disable_port() that we're deleting the port which will be
> passed down to br_multicast_disable_port() or by calling an additional
> helper to flush all which can be re-used by both disable_port() and
> stop_multicast() calls. Adding an argument to br_stp_disable_port() to
> be passed down sounds cleaner to me. What do you think?

That sounds a bit complicated, to be honest.
In fact, the reason why I submitted this as RFC only is because it isn't
solving all my problems. You know that saying "- it hurts when I do that
- then don't do that"? I think I can just change the ocelot driver to
stop remapping the untagged MDB entries to its pvid, and then I can drop
all my charges to the bridge driver.