netdev - Re: [RFC v2 PATCH 5/5] seg6: Leverage ip6_parse

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

Date:   Sat, 8 Jun 2019 13:45:59 +0100
From:   David Lebrun <dav.lebrun@...il.com>
To:     Tom Herbert <tom@...bertland.com>, davem@...emloft.net,
        netdev@...r.kernel.org, dlebrun@...gle.com
Cc:     Tom Herbert <tom@...ntonium.net>
Subject: Re: [RFC v2 PATCH 5/5] seg6: Leverage ip6_parse_tlv

On 07/06/2019 19:55, Tom Herbert wrote:
>   

...

> @@ -387,8 +416,24 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
>   		return -1;
>   	}
>   
> +	tlvoff = seg6_tlv_offset(hdr);
> +	tlvlen = ipv6_optlen((struct ipv6_opt_hdr *)hdr) - tlvoff;
> +
> +	if (tlvlen) {
> +		if (tlvlen > net->ipv6.sysctl.max_srh_opts_len) {
> +			kfree_skb(skb);
> +			return -1;
> +		}
> +
> +		if (!ip6_parse_tlv(tlvprocsrhopt_lst, skb,
> +				   init_net.ipv6.sysctl.max_srh_opts_cnt,

Why init_net ? I assume you mean 'net->ipv6.sysctl' instead.

> +				   tlvoff, tlvlen, seg6_srhopt_unknown))
> +			return -1;
> +	}
> +
>   #ifdef CONFIG_IPV6_SEG6_HMAC
> -	if (!seg6_hmac_validate_skb(skb)) {
> +	if (idev->cnf.seg6_require_hmac > 0 && !sr_has_hmac(hdr)) {
> +		/* mandatory check but no HMAC tlv */
>   		kfree_skb(skb);
>   		return -1;
>   	}
> diff --git a/net/ipv6/seg6_hmac.c b/net/ipv6/seg6_hmac.c
> index 8546f94..18f82f2 100644
> --- a/net/ipv6/seg6_hmac.c
> +++ b/net/ipv6/seg6_hmac.c
> @@ -240,7 +240,7 @@ EXPORT_SYMBOL(seg6_hmac_compute);
>    *
>    * called with rcu_read_lock()
>    */
> -bool seg6_hmac_validate_skb(struct sk_buff *skb)
> +bool seg6_hmac_validate_skb(struct sk_buff *skb, int optoff)
>   {
>   	u8 hmac_output[SEG6_HMAC_FIELD_LEN];
>   	struct net *net = dev_net(skb->dev);
> @@ -251,23 +251,13 @@ bool seg6_hmac_validate_skb(struct sk_buff *skb)
>   
>   	idev = __in6_dev_get(skb->dev);
>   
> -	srh = (struct ipv6_sr_hdr *)skb_transport_header(skb);
> -
> -	tlv = seg6_get_tlv_hmac(srh);
> -
> -	/* mandatory check but no tlv */
> -	if (idev->cnf.seg6_require_hmac > 0 && !tlv)
> -		return false;
> -
>   	/* no check */
>   	if (idev->cnf.seg6_require_hmac < 0)
>   		return true;
>   
> -	/* check only if present */
> -	if (idev->cnf.seg6_require_hmac == 0 && !tlv)
> -		return true;
> +	srh = (struct ipv6_sr_hdr *)skb_transport_header(skb);
>   
> -	/* now, seg6_require_hmac >= 0 && tlv */
> +	tlv = (struct sr6_tlv_hmac *)(skb_network_header(skb) + optoff);
>   
>   	hinfo = seg6_hmac_info_lookup(net, be32_to_cpu(tlv->hmackeyid));
>   	if (!hinfo)
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index 78155fd..d486ed8 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -92,6 +92,19 @@ static struct ipv6_sr_hdr *get_srh(struct sk_buff *skb)
>   	return srh;
>   }
>   
> +static bool seg6_local_hmac_validate_skb(struct sk_buff *skb,
> +					 struct ipv6_sr_hdr *srh)
> +{
> +#ifdef CONFIG_IPV6_SEG6_HMAC
> +	int off = sr_hmac_offset(srh);
> +
> +	return off ? seg6_hmac_validate_skb(skb, off) :
> +		     (__in6_dev_get(skb->dev)->cnf.seg6_require_hmac <= 0);

If I read sr_hmac_offset() correctly, it returns an offset relative to 
the start of the SRH, while seg_hmac_validate_skb() now expects an 
offset relative to the network header (and rightly so as it receives 
such an offset from ip6_parse_tlv). A solution might be:

seg6_hmac_validate_skb(skb, skb_network_header_len(skb) + off)

But this also assumes that the SRH is present immediately after the IPv6 
header, which might not be true when HBH or Destination Options are also 
present. So I'd suggest something like:

int nhoff = (unsigned char *)srh - skb_network_header(skb);
int off = sr_hmac_offset(srh);

return off ? seg6_hmac_validate_skb(skb, off + nhoff) ...

> +#else
> +	return true;
> +#endif
> +}
> +
>   static struct ipv6_sr_hdr *get_and_validate_srh(struct sk_buff *skb)
>   {
>   	struct ipv6_sr_hdr *srh;
> @@ -103,10 +116,8 @@ static struct ipv6_sr_hdr *get_and_validate_srh(struct sk_buff *skb)
>   	if (srh->segments_left == 0)
>   		return NULL;
>   
> -#ifdef CONFIG_IPV6_SEG6_HMAC
> -	if (!seg6_hmac_validate_skb(skb))
> +	if (!seg6_local_hmac_validate_skb(skb, srh))
>   		return NULL;
> -#endif
>   
>   	return srh;
>   }
> @@ -120,10 +131,8 @@ static bool decap_and_validate(struct sk_buff *skb, int proto)
>   	if (srh && srh->segments_left > 0)
>   		return false;
>   
> -#ifdef CONFIG_IPV6_SEG6_HMAC
> -	if (srh && !seg6_hmac_validate_skb(skb))
> +	if (srh && !seg6_local_hmac_validate_skb(skb, srh))
>   		return false;
> -#endif
>   
>   	if (ipv6_find_hdr(skb, &off, proto, NULL, NULL) < 0)
>   		return false;
>