Linux Ethernet Bridging - Oct 2018 - [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries

Recently a check was added which prevents marking of routers with zero
source address, but for IPv6 that cannot happen as the relevant RFCs
actually forbid such packets:
RFC 2710 (MLDv1):
"To be valid, the Query message MUST
 come from a link-local IPv6 Source Address, be at least 24 octets
 long, and have a correct MLD checksum."

Same goes for RFC 3810.

And also it can be seen as a requirement in ipv6_mc_check_mld_query()
which is used by the bridge to validate the message before processing
it. Thus any queries with :: source address won't be processed anyway.
So just remove the check for zero IPv6 source address from the query
processing function.

Fixes: 5a2de63fd1a5 ("bridge: do not add port to router list when receives
query with source 0.0.0.0")
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
 net/bridge/br_multicast.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 41cdafbf2ebe..6bac0d6b7b94 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1428,8 +1428,7 @@ static void br_multicast_query_received(struct net_bridge
*br,
 	 * is 0.0.0.0 should not be added to router port list.
 	 */
 	if ((saddr->proto == htons(ETH_P_IP) && saddr->u.ip4) ||
-	    (saddr->proto == htons(ETH_P_IPV6) &&
-	     !ipv6_addr_any(&saddr->u.ip6)))
+	    saddr->proto == htons(ETH_P_IPV6))
 		br_multicast_mark_router(br, port);
 }
 
-- 
2.17.2

On Sat, 27 Oct 2018 12:07:47 +0300
Nikolay Aleksandrov <nikolay at cumulusnetworks.com> wrote:
> Recently a check was added which prevents marking of routers with zero
> source address, but for IPv6 that cannot happen as the relevant RFCs
> actually forbid such packets:
> RFC 2710 (MLDv1):
> "To be valid, the Query message MUST
>  come from a link-local IPv6 Source Address, be at least 24 octets
>  long, and have a correct MLD checksum."
> 
> Same goes for RFC 3810.
> 
> And also it can be seen as a requirement in ipv6_mc_check_mld_query()
> which is used by the bridge to validate the message before processing
> it. Thus any queries with :: source address won't be processed anyway.
> So just remove the check for zero IPv6 source address from the query
> processing function.
> 
> Fixes: 5a2de63fd1a5 ("bridge: do not add port to router list when
receives query with source 0.0.0.0")
> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
What about a broken/malicious sender? Could an all zero source be used
to poison the multicast table?

On Sat, Oct 27, 2018 at 12:07:47PM +0300, Nikolay Aleksandrov
wrote:
> Recently a check was added which prevents marking of routers with zero
> source address, but for IPv6 that cannot happen as the relevant RFCs
> actually forbid such packets:
> RFC 2710 (MLDv1):
> "To be valid, the Query message MUST
>  come from a link-local IPv6 Source Address, be at least 24 octets
>  long, and have a correct MLD checksum."
> 
> Same goes for RFC 3810.
> 
> And also it can be seen as a requirement in ipv6_mc_check_mld_query()
> which is used by the bridge to validate the message before processing
> it. Thus any queries with :: source address won't be processed anyway.
> So just remove the check for zero IPv6 source address from the query
> processing function.
> 
> Fixes: 5a2de63fd1a5 ("bridge: do not add port to router list when
receives query with source 0.0.0.0")
> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Opps.. Sorry for the mistake and thank you for your fix.

Regards
Hangbin
> ---
>  net/bridge/br_multicast.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 41cdafbf2ebe..6bac0d6b7b94 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1428,8 +1428,7 @@ static void br_multicast_query_received(struct
net_bridge *br,
>  	 * is 0.0.0.0 should not be added to router port list.
>  	 */
>  	if ((saddr->proto == htons(ETH_P_IP) && saddr->u.ip4) ||
> -	    (saddr->proto == htons(ETH_P_IPV6) &&
> -	     !ipv6_addr_any(&saddr->u.ip6)))
> +	    saddr->proto == htons(ETH_P_IPV6))
>  		br_multicast_mark_router(br, port);
>  }
>  
> -- 
> 2.17.2
>

From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Date: Sat, 27 Oct 2018 12:07:47 +0300
> Recently a check was added which prevents marking of routers with zero
> source address, but for IPv6 that cannot happen as the relevant RFCs
> actually forbid such packets:
> RFC 2710 (MLDv1):
> "To be valid, the Query message MUST
>  come from a link-local IPv6 Source Address, be at least 24 octets
>  long, and have a correct MLD checksum."
> 
> Same goes for RFC 3810.
> 
> And also it can be seen as a requirement in ipv6_mc_check_mld_query()
> which is used by the bridge to validate the message before processing
> it. Thus any queries with :: source address won't be processed anyway.
> So just remove the check for zero IPv6 source address from the query
> processing function.
> 
> Fixes: 5a2de63fd1a5 ("bridge: do not add port to router list when
receives query with source 0.0.0.0")
> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Applied.

Even though RFC4541 recommends this, I'm not quite sure whether
this works... even for IGMP.

I think this would lead to multicast packet loss in a scenario
like this:

----------

     [Switch A] -------------- [Switch B]
        /                          /
       /                          /
      /                          /
 (Host A)                   (Host B)


- Snooping Switches: Switch A + Switch B
- Selected Querier: Switch A, with 0.0.0.0 query source
- Multicast Listener: Host A
- Multicast Data Sender: Host B

1) Host A sends IGMP report to Switch A
2) Switch A refrains from forwarding it to Switch B
   (reports are only forwarded to multicast routers according to
    RFC4541)
   => Switch B does not learn about listeners on Host A

Now, with this patch and recommendation in RFC4541 to not add queries
with a 0.0.0.0 source address to the multicast router port list:

3) Host B sends multicast data to Switch B
   => Switch B does not forward it to Switch A as it neither
      detected a multicast listener nor multicast router on
      the according port.
   => Host A does not receive the multicast data it signed up for

(Or with colors:
https://metameute.de/~tux/linux/bridge/query-zero-source-no-mcrouter-port.png)

----------

Alternatively we would need to ignore 0.0.0.0 for the querier
election and "querier present" detection. And by that disable
multicast snooping if there are no queries from a non-zero source
address.

But I'm a little hesitant whether ignoring is a reliable way as
IGMPv3 (RFC3376) and IGMPv2 (RFC2236) make no such restrictions
regarding the query source address.

With no such restrictions according to RFC3376/RFC2236 a 0.0.0.0
would always win the querier election. Meaning any potential
querier with a non-zero source address would remain silent.
Meaning we would always disable multicast snooping then?


Adding queriers with a 0.0.0.0 source address to the multicast
router list, too, seems like a less harmful way then disabling multicast
snooping completely?

----------

However, one of the two options seems to be necessary. Either
reverting the patch for the IGMP part, too. Or Ignoring 0.0.0.0
sources for querier eletcion and presence detection.

The current state seems broken to me unless I'm missing something.