diff mbox series

[RFC,v2,5/5] seg6: Leverage ip6_parse_tlv

Message ID 1559933708-13947-6-git-send-email-tom@quantonium.net
State RFC
Delegated to: David Miller
Headers show
Series seg6: Segment routing fixes | expand

Commit Message

Tom Herbert June 7, 2019, 6:55 p.m. UTC 
Call ip6_parse_tlv from segment routing receive function to properly
parse TLVs and to leverage to existing implementation that already
parses Destination and Hop-by-Hop Options. This includes applying
the denial of service mitigations and limits to processing segment
routing TLVs.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/net/seg6.h      |  5 +++++
 include/net/seg6_hmac.h |  2 +-
 net/ipv6/exthdrs.c      | 51 ++++++++++++++++++++++++++++++++++++++++++++++---
 net/ipv6/seg6_hmac.c    | 16 +++-------------
 net/ipv6/seg6_local.c   | 21 ++++++++++++++------
 5 files changed, 72 insertions(+), 23 deletions(-)

Comments

David Lebrun June 8, 2019, 12:45 p.m. UTC | #1 
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;
>
diff mbox series

Patch

diff --git a/include/net/seg6.h b/include/net/seg6.h
index 8b2dc68..b7d8a94 100644
--- a/include/net/seg6.h
+++ b/include/net/seg6.h
@@ -38,6 +38,11 @@  static inline void update_csum_diff16(struct sk_buff *skb, __be32 *from,
 	skb->csum = ~csum_partial((char *)diff, sizeof(diff), ~skb->csum);
 }
 
+static inline unsigned int seg6_tlv_offset(struct ipv6_sr_hdr *srh)
+{
+	return sizeof(*srh) + ((srh->first_segment + 1) << 4);
+}
+
 struct seg6_pernet_data {
 	struct mutex lock;
 	struct in6_addr __rcu *tun_src;
diff --git a/include/net/seg6_hmac.h b/include/net/seg6_hmac.h
index 7fda469..6ad33e2 100644
--- a/include/net/seg6_hmac.h
+++ b/include/net/seg6_hmac.h
@@ -53,7 +53,7 @@  extern int seg6_hmac_info_add(struct net *net, u32 key,
 extern int seg6_hmac_info_del(struct net *net, u32 key);
 extern int seg6_push_hmac(struct net *net, struct in6_addr *saddr,
 			  struct ipv6_sr_hdr *srh);
-extern bool seg6_hmac_validate_skb(struct sk_buff *skb);
+extern bool seg6_hmac_validate_skb(struct sk_buff *skb, int optoff);
 extern int seg6_hmac_init(void);
 extern void seg6_hmac_exit(void);
 extern int seg6_hmac_net_init(struct net *net);
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index a394d20..7d14c0d 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -112,7 +112,8 @@  static bool ip6_tlvopt_unknown(struct sk_buff *skb, int optoff,
 	return false;
 }
 
-/* Parse tlv encoded option header (hop-by-hop or destination)
+/* Parse tlv encoded option header (hop-by-hop, destination, or
+ * segment routing header)
  *
  * Arguments:
  *   procs - TLV proc structure
@@ -365,14 +366,42 @@  static void seg6_update_csum(struct sk_buff *skb)
 			   (__be32 *)addr);
 }
 
+static bool seg6_recv_hmac(struct sk_buff *skb, int optoff)
+{
+	if (!seg6_hmac_validate_skb(skb, optoff)) {
+		kfree_skb(skb);
+		return false;
+	}
+
+	return true;
+}
+
+static const struct tlvtype_proc tlvprocsrhopt_lst[] = {
+#ifdef CONFIG_IPV6_SEG6_HMAC
+	{
+		.type	= SR6_TLV_HMAC,
+		.func	= seg6_recv_hmac,
+	},
+#endif
+	{-1,			NULL}
+};
+
+static bool seg6_srhopt_unknown(struct sk_buff *skb, int optoff,
+				bool disallow_unknowns)
+{
+	/* Unknown segment routing header options are ignored */
+
+	return !disallow_unknowns;
+}
+
 static int ipv6_srh_rcv(struct sk_buff *skb)
 {
 	struct inet6_skb_parm *opt = IP6CB(skb);
 	struct net *net = dev_net(skb->dev);
+	int accept_seg6, tlvoff, tlvlen;
 	struct ipv6_sr_hdr *hdr;
 	struct inet6_dev *idev;
 	struct in6_addr *addr;
-	int accept_seg6;
 
 	hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);
 
@@ -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,
+				   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);
+#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;