netdev - Re: [PATCH v3 net] net: ipv4/ipv6 addrconf: call igmp{,6}_group

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]

Message-ID: <20230412135337.hp6jxne6lkl2r2ud@skbuf>
Date:   Wed, 12 Apr 2023 16:53:37 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Ido Schimmel <idosch@...dia.com>
Cc:     netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH v3 net] net: ipv4/ipv6 addrconf: call
 igmp{,6}_group_dropped() while dev is still up

On Wed, Apr 12, 2023 at 04:34:00PM +0300, Ido Schimmel wrote:
> Even with the proposed fix, wouldn't you get the same leak with the
> following reproducer?
> 
> ip link set dev swp0 up
> bridge fdb add 01:02:03:04:05:06 dev swp0 self local
> ip link set dev swp0 down
> echo 0000:00:00.5 > /sys/bus/pci/drivers/mscc_felix/unbind
> 
> If so, I wonder how other drivers that allocate memory in their
> ndo_set_rx_mode() deal with this problem. I would imagine that they
> flush the addresses in their ndo_stop() or as part of device dismantle.

Hmm, yeah, fair. I was operating on the premise that callers of
dev_mc_add()/dev_mc_del() would be sane enough to keep track of their
calls, and at least aspire to not leave something behind. Clearly not the
case here, if user space can add a unicast/multicast filter and run...

So I guess I'm left to clean up the stale addresses at DSA removal time,
and remove the WARN_ON().

Actually, looking at ndo_dflt_fdb_add() and ndo_dflt_fdb_del() again,
what concerns me more is this:

	if (is_unicast_ether_addr(addr) || is_link_local_ether_addr(addr))
		err = dev_uc_add_excl(dev, addr);

I.O.W. this:

$ bridge fdb add 01:80:c2:00:00:0e dev swp0 self local

generates a filtering call for a unicast address.
And I don't know about other drivers, but in DSA, we assume that unicast
addresses are unicast, and we have separate API for that vs multicast...