net: bridge: mcast: fix broken length + header check for MRDv6 Adv.

The IPv6 Multicast Router Advertisements parsing has the following two
issues:

For one thing, ICMPv6 MRD Advertisements are smaller than ICMPv6 MLD
messages (ICMPv6 MRD Adv.: 8 bytes vs. ICMPv6 MLDv1/2: >= 24 bytes,
assuming MLDv2 Reports with at least one multicast address entry).
When ipv6_mc_check_mld_msg() tries to parse an Multicast Router
Advertisement its MLD length check will fail - and it will wrongly
return -EINVAL, even if we have a valid MRD Advertisement. With the
returned -EINVAL the bridge code will assume a broken packet and will
wrongly discard it, potentially leading to multicast packet loss towards
multicast routers.

The second issue is the MRD header parsing in
br_ip6_multicast_mrd_rcv(): It wrongly checks for an ICMPv6 header
immediately after the IPv6 header (IPv6 next header type). However
according to RFC4286, section 2 all MRD messages contain a Router Alert
option (just like MLD). So instead there is an IPv6 Hop-by-Hop option
for the Router Alert between the IPv6 and ICMPv6 header, again leading
to the bridge wrongly discarding Multicast Router Advertisements.

To fix these two issues, introduce a new return value -ENODATA to
ipv6_mc_check_mld() to indicate a valid ICMPv6 packet with a hop-by-hop
option which is not an MLD but potentially an MRD packet. This also
simplifies further parsing in the bridge code, as ipv6_mc_check_mld()
already fully checks the ICMPv6 header and hop-by-hop option.

These issues were found and fixed with the help of the mrdisc tool
(https://github.com/troglobit/mrdisc).

Fixes: 4b3087c7e3 ("bridge: Snoop Multicast Router Advertisements")
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
Signed-off-by: David S. Miller <davem@davemloft.net>
This commit is contained in:
Linus Lüssing 2021-04-25 17:27:35 +02:00 committed by David S. Miller
parent 6d72e7c767
commit 9901408815
3 changed files with 15 additions and 31 deletions

View File

@ -233,7 +233,6 @@ void ipv6_mc_unmap(struct inet6_dev *idev);
void ipv6_mc_remap(struct inet6_dev *idev); void ipv6_mc_remap(struct inet6_dev *idev);
void ipv6_mc_init_dev(struct inet6_dev *idev); void ipv6_mc_init_dev(struct inet6_dev *idev);
void ipv6_mc_destroy_dev(struct inet6_dev *idev); void ipv6_mc_destroy_dev(struct inet6_dev *idev);
int ipv6_mc_check_icmpv6(struct sk_buff *skb);
int ipv6_mc_check_mld(struct sk_buff *skb); int ipv6_mc_check_mld(struct sk_buff *skb);
void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp); void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp);

View File

@ -3159,25 +3159,14 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
} }
#if IS_ENABLED(CONFIG_IPV6) #if IS_ENABLED(CONFIG_IPV6)
static int br_ip6_multicast_mrd_rcv(struct net_bridge *br, static void br_ip6_multicast_mrd_rcv(struct net_bridge *br,
struct net_bridge_port *port, struct net_bridge_port *port,
struct sk_buff *skb) struct sk_buff *skb)
{ {
int ret;
if (ipv6_hdr(skb)->nexthdr != IPPROTO_ICMPV6)
return -ENOMSG;
ret = ipv6_mc_check_icmpv6(skb);
if (ret < 0)
return ret;
if (icmp6_hdr(skb)->icmp6_type != ICMPV6_MRDISC_ADV) if (icmp6_hdr(skb)->icmp6_type != ICMPV6_MRDISC_ADV)
return -ENOMSG; return;
br_multicast_mark_router(br, port); br_multicast_mark_router(br, port);
return 0;
} }
static int br_multicast_ipv6_rcv(struct net_bridge *br, static int br_multicast_ipv6_rcv(struct net_bridge *br,
@ -3191,18 +3180,12 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
err = ipv6_mc_check_mld(skb); err = ipv6_mc_check_mld(skb);
if (err == -ENOMSG) { if (err == -ENOMSG || err == -ENODATA) {
if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr)) if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
BR_INPUT_SKB_CB(skb)->mrouters_only = 1; BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
if (err == -ENODATA &&
if (ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr)) { ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr))
err = br_ip6_multicast_mrd_rcv(br, port, skb); br_ip6_multicast_mrd_rcv(br, port, skb);
if (err < 0 && err != -ENOMSG) {
br_multicast_err_count(br, port, skb->protocol);
return err;
}
}
return 0; return 0;
} else if (err < 0) { } else if (err < 0) {

View File

@ -109,7 +109,7 @@ static int ipv6_mc_check_mld_msg(struct sk_buff *skb)
struct mld_msg *mld; struct mld_msg *mld;
if (!ipv6_mc_may_pull(skb, len)) if (!ipv6_mc_may_pull(skb, len))
return -EINVAL; return -ENODATA;
mld = (struct mld_msg *)skb_transport_header(skb); mld = (struct mld_msg *)skb_transport_header(skb);
@ -122,7 +122,7 @@ static int ipv6_mc_check_mld_msg(struct sk_buff *skb)
case ICMPV6_MGM_QUERY: case ICMPV6_MGM_QUERY:
return ipv6_mc_check_mld_query(skb); return ipv6_mc_check_mld_query(skb);
default: default:
return -ENOMSG; return -ENODATA;
} }
} }
@ -131,7 +131,7 @@ static inline __sum16 ipv6_mc_validate_checksum(struct sk_buff *skb)
return skb_checksum_validate(skb, IPPROTO_ICMPV6, ip6_compute_pseudo); return skb_checksum_validate(skb, IPPROTO_ICMPV6, ip6_compute_pseudo);
} }
int ipv6_mc_check_icmpv6(struct sk_buff *skb) static int ipv6_mc_check_icmpv6(struct sk_buff *skb)
{ {
unsigned int len = skb_transport_offset(skb) + sizeof(struct icmp6hdr); unsigned int len = skb_transport_offset(skb) + sizeof(struct icmp6hdr);
unsigned int transport_len = ipv6_transport_len(skb); unsigned int transport_len = ipv6_transport_len(skb);
@ -150,7 +150,6 @@ int ipv6_mc_check_icmpv6(struct sk_buff *skb)
return 0; return 0;
} }
EXPORT_SYMBOL(ipv6_mc_check_icmpv6);
/** /**
* ipv6_mc_check_mld - checks whether this is a sane MLD packet * ipv6_mc_check_mld - checks whether this is a sane MLD packet
@ -161,7 +160,10 @@ EXPORT_SYMBOL(ipv6_mc_check_icmpv6);
* *
* -EINVAL: A broken packet was detected, i.e. it violates some internet * -EINVAL: A broken packet was detected, i.e. it violates some internet
* standard * standard
* -ENOMSG: IP header validation succeeded but it is not an MLD packet. * -ENOMSG: IP header validation succeeded but it is not an ICMPv6 packet
* with a hop-by-hop option.
* -ENODATA: IP+ICMPv6 header with hop-by-hop option validation succeeded
* but it is not an MLD packet.
* -ENOMEM: A memory allocation failure happened. * -ENOMEM: A memory allocation failure happened.
* *
* Caller needs to set the skb network header and free any returned skb if it * Caller needs to set the skb network header and free any returned skb if it