[v2,net] ipv6: Fix MLD Query message check

Based on RFC3810 6.2, we also need to check the hop limit and router alert
option besides source address.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/mcast.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Wed, 25 Jun 2014 09:31:07 +0800

> @@ -1301,8 +1301,18 @@ int igmp6_event_query(struct sk_buff *skb)
>  	len = ntohs(ipv6_hdr(skb)->payload_len) + sizeof(struct ipv6hdr);
>  	len -= skb_network_header_len(skb);
>  
> -	/* Drop queries with not link local source */
> -	if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL))
> +	/* RFC3810 6.2
> +	 * Upon reception of an MLD message that contains a Query, the node
> +	 * checks if the source address of the message is a valid link-local
> +	 * address, if the Hop Limit is set to 1, and if the Router Alert
> +	 * option is present in the Hop-By-Hop Options header of the IPv6
> +	 * packet.  If any of these checks fails, the packet is dropped.
> +	 */
> +	if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL) ||
> +	    ipv6_hdr(skb)->hop_limit != 1 ||
> +	    ipv6_hdr(skb)->nexthdr != NEXTHDR_HOP ||
> +	    !(IP6CB(skb)->flags & IP6SKB_ROUTERALERT) ||
> +	    IP6CB(skb)->ra != htons(IPV6_OPT_ROUTERALERT_MLD))
>  		return -EINVAL;
>  

Does the NEXTHDR_HOP have the be exactly the next extension header
present?  I think you need to allow for more flexibility here.

In any event, the only way the IP6SKB_ROUTERALERT bit can be set
is if there were hop-by-hop options present, so testing only the
bit itself is sufficient.

The forwarding path simplifies the check in this way too.

So please adjust this to only check the bit and remove the NEXTHDR_HOP
test.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

2014-06-27 8:33 GMT+08:00 David Miller <davem@davemloft.net>:
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Wed, 25 Jun 2014 09:31:07 +0800
>
>> @@ -1301,8 +1301,18 @@ int igmp6_event_query(struct sk_buff *skb)
>>       len = ntohs(ipv6_hdr(skb)->payload_len) + sizeof(struct ipv6hdr);
>>       len -= skb_network_header_len(skb);
>>
>> -     /* Drop queries with not link local source */
>> -     if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL))
>> +     /* RFC3810 6.2
>> +      * Upon reception of an MLD message that contains a Query, the node
>> +      * checks if the source address of the message is a valid link-local
>> +      * address, if the Hop Limit is set to 1, and if the Router Alert
>> +      * option is present in the Hop-By-Hop Options header of the IPv6
>> +      * packet.  If any of these checks fails, the packet is dropped.
>> +      */
>> +     if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL) ||
>> +         ipv6_hdr(skb)->hop_limit != 1 ||
>> +         ipv6_hdr(skb)->nexthdr != NEXTHDR_HOP ||
>> +         !(IP6CB(skb)->flags & IP6SKB_ROUTERALERT) ||
>> +         IP6CB(skb)->ra != htons(IPV6_OPT_ROUTERALERT_MLD))
>>               return -EINVAL;
>>
>
> Does the NEXTHDR_HOP have the be exactly the next extension header
> present?  I think you need to allow for more flexibility here.
>
> In any event, the only way the IP6SKB_ROUTERALERT bit can be set
> is if there were hop-by-hop options present, so testing only the
> bit itself is sufficient.
>
> The forwarding path simplifies the check in this way too.
>
> So please adjust this to only check the bit and remove the NEXTHDR_HOP
> test.
>
Thanks for this advise, will send a v3 patch

Best regards
Hangbin Liu
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html