[RFC PATCH 0/6] seg6: Segment routing fixes

[RFC PATCH 0/6] seg6: Segment routing fixes

[Tom Herbert]

A few bug fixes, code clean up, and minor enhancements to segment
routing.

Changes include:
  - Fix SRH TLV and flag definitions (some of them are now obsolete)
  - Implement a TLV parsing loop
  - Add function to parse and return HMAC TLV in an SRH
  - Support to rearrange segment routing header for AH ICV
    calculation

Set as RFC to let experts on segment routing implemenation evaluate
which of the patches are needed.

Tom Herbert (6):
  seg6: Fix TLV definitions
  seg6: Implement a TLV parsing loop
  seg6: Obsolete unused SRH flags
  ah6: Create function __zero_out_mutable_opts
  ah6: Be explicit about which routing types are processed.
  seg6: Add support to rearrange SRH for AH ICV calculation

 include/net/seg6.h        | 16 +++++++++
 include/uapi/linux/seg6.h | 60 +++++++++++++++++++++++++------
 net/ipv6/ah6.c            | 90 +++++++++++++++++++++++++++++++++++------------
 net/ipv6/exthdrs.c        |  2 +-
 net/ipv6/seg6.c           | 68 +++++++++++++++++++++--------------
 net/ipv6/seg6_hmac.c      |  8 ++---
 net/ipv6/seg6_iptunnel.c  |  4 +--
 7 files changed, 181 insertions(+), 67 deletions(-)

-- 
2.7.4

[RFC PATCH 1/6] seg6: Fix TLV definitions

[Tom Herbert]

The definitions of TLVs in uapi/linux/seg6.h are incorrect and
incomplete. Fix this.

TLV constants are defined for PAD1, PADN, and HMAC (the three defined in
draft-ietf-6man-segment-routing-header-19). The other TLV are unused and
and are marked as obsoleted.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/uapi/linux/seg6.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/seg6.h b/include/uapi/linux/seg6.h
index 286e8d6..9117113 100644
--- a/include/uapi/linux/seg6.h
+++ b/include/uapi/linux/seg6.h
@@ -38,10 +38,13 @@ struct ipv6_sr_hdr {
 #define SR6_FLAG1_ALERT		(1 << 4)
 #define SR6_FLAG1_HMAC		(1 << 3)
 
-#define SR6_TLV_INGRESS		1
-#define SR6_TLV_EGRESS		2
-#define SR6_TLV_OPAQUE		3
-#define SR6_TLV_PADDING		4
+
+#define SR6_TLV_INGRESS		1	/* obsoleted */
+#define SR6_TLV_EGRESS		2	/* obsoleted */
+#define SR6_TLV_OPAQUE		3	/* obsoleted */
+
+#define SR6_TLV_PAD1		0
+#define SR6_TLV_PADDING		1
 #define SR6_TLV_HMAC		5
 
 #define sr_has_hmac(srh) ((srh)->flags & SR6_FLAG1_HMAC)
-- 
2.7.4

[RFC PATCH 2/6] seg6: Implement a TLV parsing loop

[Tom Herbert]

Implement a TLV parsing loop for segment routing. The code is uniform
with other instances of TLV parsing loops in the stack (e.g. parsing of
Hop-by-Hop and Destination Options).

seg_validate_srh calls this function. Note, this fixes a bug in the
original parsing code that PAD1 was not supported.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/net/seg6.h |  6 ++++++
 net/ipv6/seg6.c    | 60 +++++++++++++++++++++++++++++++-----------------------
 2 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/include/net/seg6.h b/include/net/seg6.h
index 8b2dc68..563d4a6 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;
@@ -62,6 +67,7 @@ extern void seg6_iptunnel_exit(void);
 extern int seg6_local_init(void);
 extern void seg6_local_exit(void);
 
+extern bool __seg6_parse_srh(struct ipv6_sr_hdr *srh);
 extern bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len);
 extern int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
 			     int proto);
diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
index 0c5479e..e461357 100644
--- a/net/ipv6/seg6.c
+++ b/net/ipv6/seg6.c
@@ -30,44 +30,52 @@
 #include <net/seg6_hmac.h>
 #endif
 
-bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len)
+bool __seg6_parse_srh(struct ipv6_sr_hdr *srh)
 {
-	int trailing;
-	unsigned int tlv_offset;
+	int len = ipv6_optlen((struct ipv6_opt_hdr *)srh);
+	unsigned char *opt = (unsigned char *)srh;
+	unsigned int off;
 
-	if (srh->type != IPV6_SRCRT_TYPE_4)
-		return false;
+	off = seg6_tlv_offset(srh);
+	len -= off;
 
-	if (((srh->hdrlen + 1) << 3) != len)
-		return false;
+	while (len > 0) {
+		struct sr6_tlv *tlv;
+		unsigned int optlen;
 
-	if (srh->segments_left > srh->first_segment)
-		return false;
+		switch (opt[off]) {
+		case SR6_TLV_PAD1:
+			optlen = 1;
+			break;
+		default:
+			if (len < sizeof(*tlv))
+				return false;
 
-	tlv_offset = sizeof(*srh) + ((srh->first_segment + 1) << 4);
+			tlv = (struct sr6_tlv *)&opt[off];
+			optlen = sizeof(*tlv) + tlv->len;
 
-	trailing = len - tlv_offset;
-	if (trailing < 0)
-		return false;
+			break;
+		}
 
-	while (trailing) {
-		struct sr6_tlv *tlv;
-		unsigned int tlv_len;
+		off += optlen;
+		len -= optlen;
+	}
 
-		if (trailing < sizeof(*tlv))
-			return false;
+	return !len;
+}
 
-		tlv = (struct sr6_tlv *)((unsigned char *)srh + tlv_offset);
-		tlv_len = sizeof(*tlv) + tlv->len;
+bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len)
+{
+	if (srh->type != IPV6_SRCRT_TYPE_4)
+		return false;
 
-		trailing -= tlv_len;
-		if (trailing < 0)
-			return false;
+	if (ipv6_optlen((struct ipv6_opt_hdr *)srh) != len)
+		return false;
 
-		tlv_offset += tlv_len;
-	}
+	if (srh->segments_left > srh->first_segment)
+		return false;
 
-	return true;
+	return __seg6_parse_srh(srh);
 }
 
 static struct genl_family seg6_genl_family;
-- 
2.7.4

[RFC PATCH 3/6] seg6: Obsolete unused SRH flags

[Tom Herbert]

Currently no flags are defined for segment routing in
draft-ietf-6man-segment-routing-header-19. Mark them as being
obsolete.

The HMAC flag is the only one used by the stack. This needs
additional consideration.

Rewrite sr_has_hmac in uapi/linux/seg6.h to properly parse a
segment routing header as opposed to relying on the now obsolete
code.

Implement seg6_find_hmac_tlv for internal stack use. That function
parses (via __seg6_parse_srh) a TLV list and returns the pointer to
an HMAC TLV if one exists. The parsing function also eliminates the
assumption in seg6_get_tlv_hmac that the HMAC TLV must be the first TLV.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/net/seg6.h        | 12 +++++++++++-
 include/uapi/linux/seg6.h | 49 ++++++++++++++++++++++++++++++++++++++++-------
 net/ipv6/exthdrs.c        |  2 +-
 net/ipv6/seg6.c           | 12 ++++++++++--
 net/ipv6/seg6_hmac.c      |  8 +++-----
 net/ipv6/seg6_iptunnel.c  |  4 ++--
 6 files changed, 69 insertions(+), 18 deletions(-)

diff --git a/include/net/seg6.h b/include/net/seg6.h
index 563d4a6..47e7c90 100644
--- a/include/net/seg6.h
+++ b/include/net/seg6.h
@@ -17,6 +17,7 @@
 #include <linux/net.h>
 #include <linux/ipv6.h>
 #include <linux/seg6.h>
+#include <linux/seg6_hmac.h>
 #include <linux/rhashtable-types.h>
 
 static inline void update_csum_diff4(struct sk_buff *skb, __be32 from,
@@ -67,11 +68,20 @@ extern void seg6_iptunnel_exit(void);
 extern int seg6_local_init(void);
 extern void seg6_local_exit(void);
 
-extern bool __seg6_parse_srh(struct ipv6_sr_hdr *srh);
+extern bool __seg6_parse_srh(struct ipv6_sr_hdr *srh,
+			     struct sr6_tlv_hmac **hmacp);
 extern bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len);
 extern int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
 			     int proto);
 extern int seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh);
 extern int seg6_lookup_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
 			       u32 tbl_id);
+
+static inline struct sr6_tlv_hmac *seg6_find_hmac_tlv(struct ipv6_sr_hdr *srh)
+{
+	struct sr6_tlv_hmac *hmacp = NULL;
+
+	return __seg6_parse_srh(srh, &hmacp) ? hmacp : NULL;
+}
+
 #endif
diff --git a/include/uapi/linux/seg6.h b/include/uapi/linux/seg6.h
index 9117113..890420b0 100644
--- a/include/uapi/linux/seg6.h
+++ b/include/uapi/linux/seg6.h
@@ -33,11 +33,10 @@ struct ipv6_sr_hdr {
 	struct in6_addr segments[0];
 };
 
-#define SR6_FLAG1_PROTECTED	(1 << 6)
-#define SR6_FLAG1_OAM		(1 << 5)
-#define SR6_FLAG1_ALERT		(1 << 4)
-#define SR6_FLAG1_HMAC		(1 << 3)
-
+#define SR6_FLAG1_PROTECTED	(1 << 6)	/* obsoleted */
+#define SR6_FLAG1_OAM		(1 << 5)	/* obsoleted */
+#define SR6_FLAG1_ALERT		(1 << 4)	/* obsoleted */
+#define SR6_FLAG1_HMAC		(1 << 3)	/* obsoleted */
 
 #define SR6_TLV_INGRESS		1	/* obsoleted */
 #define SR6_TLV_EGRESS		2	/* obsoleted */
@@ -47,12 +46,48 @@ struct ipv6_sr_hdr {
 #define SR6_TLV_PADDING		1
 #define SR6_TLV_HMAC		5
 
-#define sr_has_hmac(srh) ((srh)->flags & SR6_FLAG1_HMAC)
-
 struct sr6_tlv {
 	__u8 type;
 	__u8 len;
 	__u8 data[0];
 };
 
+static inline bool __sr_has_hmac(struct ipv6_sr_hdr *srh)
+{
+	unsigned char *opt = (unsigned char *)srh;
+	int len = (srh->hdrlen + 1) << 8;
+	unsigned int off;
+
+	off = sizeof(*srh) + ((srh->first_segment + 1) << 4);
+	len -= off;
+
+	while (len > 0) {
+		struct sr6_tlv *tlv;
+		unsigned int optlen;
+
+		switch (opt[off]) {
+		case SR6_TLV_PAD1:
+			optlen = 1;
+			break;
+		case SR6_TLV_HMAC:
+			return true;
+		default:
+			if (len < sizeof(*tlv))
+				return false;
+
+			tlv = (struct sr6_tlv *)&opt[off];
+			optlen = sizeof(*tlv) + tlv->len;
+
+			break;
+		}
+
+		off += optlen;
+		len -= optlen;
+	}
+
+	return false;
+}
+
+#define sr_has_hmac(srh) __sr_has_hmac(srh)
+
 #endif
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 20291c2..112e2fd 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -922,7 +922,7 @@ static void ipv6_push_rthdr4(struct sk_buff *skb, u8 *proto,
 	}
 
 #ifdef CONFIG_IPV6_SEG6_HMAC
-	if (sr_has_hmac(sr_phdr)) {
+	if (seg6_find_hmac_tlv(sr_phdr)) {
 		struct net *net = NULL;
 
 		if (skb->dev)
diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
index e461357..1e782a6 100644
--- a/net/ipv6/seg6.c
+++ b/net/ipv6/seg6.c
@@ -30,7 +30,7 @@
 #include <net/seg6_hmac.h>
 #endif
 
-bool __seg6_parse_srh(struct ipv6_sr_hdr *srh)
+bool __seg6_parse_srh(struct ipv6_sr_hdr *srh, struct sr6_tlv_hmac **hmacp)
 {
 	int len = ipv6_optlen((struct ipv6_opt_hdr *)srh);
 	unsigned char *opt = (unsigned char *)srh;
@@ -39,6 +39,8 @@ bool __seg6_parse_srh(struct ipv6_sr_hdr *srh)
 	off = seg6_tlv_offset(srh);
 	len -= off;
 
+	*hmacp = NULL;
+
 	while (len > 0) {
 		struct sr6_tlv *tlv;
 		unsigned int optlen;
@@ -47,6 +49,10 @@ bool __seg6_parse_srh(struct ipv6_sr_hdr *srh)
 		case SR6_TLV_PAD1:
 			optlen = 1;
 			break;
+		case SR6_TLV_HMAC:
+			if (!*hmacp)
+				*hmacp = (struct sr6_tlv_hmac *)&opt[off];
+			/* Fall through */
 		default:
 			if (len < sizeof(*tlv))
 				return false;
@@ -66,6 +72,8 @@ bool __seg6_parse_srh(struct ipv6_sr_hdr *srh)
 
 bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len)
 {
+	struct sr6_tlv_hmac *hmacp;
+
 	if (srh->type != IPV6_SRCRT_TYPE_4)
 		return false;
 
@@ -75,7 +83,7 @@ bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len)
 	if (srh->segments_left > srh->first_segment)
 		return false;
 
-	return __seg6_parse_srh(srh);
+	return __seg6_parse_srh(srh, &hmacp);
 }
 
 static struct genl_family seg6_genl_family;
diff --git a/net/ipv6/seg6_hmac.c b/net/ipv6/seg6_hmac.c
index 8546f94..92b398c 100644
--- a/net/ipv6/seg6_hmac.c
+++ b/net/ipv6/seg6_hmac.c
@@ -95,13 +95,11 @@ static struct sr6_tlv_hmac *seg6_get_tlv_hmac(struct ipv6_sr_hdr *srh)
 	if (srh->hdrlen < (srh->first_segment + 1) * 2 + 5)
 		return NULL;
 
-	if (!sr_has_hmac(srh))
+	tlv = seg6_find_hmac_tlv(srh);
+	if (!tlv)
 		return NULL;
 
-	tlv = (struct sr6_tlv_hmac *)
-	      ((char *)srh + ((srh->hdrlen + 1) << 3) - 40);
-
-	if (tlv->tlvhdr.type != SR6_TLV_HMAC || tlv->tlvhdr.len != 38)
+	if (tlv->tlvhdr.len != sizeof(*tlv) - 2)
 		return NULL;
 
 	return tlv;
diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
index 7a525fd..5344bee 100644
--- a/net/ipv6/seg6_iptunnel.c
+++ b/net/ipv6/seg6_iptunnel.c
@@ -161,7 +161,7 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
 	set_tun_src(net, dst->dev, &hdr->daddr, &hdr->saddr);
 
 #ifdef CONFIG_IPV6_SEG6_HMAC
-	if (sr_has_hmac(isrh)) {
+	if (seg6_find_hmac_tlv(isrh)) {
 		err = seg6_push_hmac(net, &hdr->saddr, isrh);
 		if (unlikely(err))
 			return err;
@@ -211,7 +211,7 @@ int seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh)
 	hdr->daddr = isrh->segments[isrh->first_segment];
 
 #ifdef CONFIG_IPV6_SEG6_HMAC
-	if (sr_has_hmac(isrh)) {
+	if (seg6_find_hmac_tlv(isrh)) {
 		struct net *net = dev_net(skb_dst(skb)->dev);
 
 		err = seg6_push_hmac(net, &hdr->saddr, isrh);
-- 
2.7.4

[RFC PATCH 4/6] ah6: Create function __zero_out_mutable_opts

[Tom Herbert]

This is an adaptation of zero_out_mutable_opts that takes three
additional arguments: offset of the TLVs, a mask to locate the
mutable bit in the TLV type, and the type value for single byte
padding.

zero_out_mutable_opts calls the new function and sets the arguments
appropriate to Hop-by-Hop and Destination Options. The function will
be used to support zeroing out mutable SRH TLVs' data with the
appropriate arguments for SRH TLVs.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 net/ipv6/ah6.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 68b9e92..1e80157 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -102,32 +102,28 @@ static inline struct scatterlist *ah_req_sg(struct crypto_ahash *ahash,
 			     __alignof__(struct scatterlist));
 }
 
-static bool zero_out_mutable_opts(struct ipv6_opt_hdr *opthdr)
+static bool __zero_out_mutable_opts(struct ipv6_opt_hdr *opthdr, int off,
+				    unsigned char mut_bit, unsigned char pad1)
 {
 	u8 *opt = (u8 *)opthdr;
 	int len = ipv6_optlen(opthdr);
-	int off = 0;
 	int optlen = 0;
 
-	off += 2;
-	len -= 2;
+	len -= off;
 
 	while (len > 0) {
-
-		switch (opt[off]) {
-
-		case IPV6_TLV_PAD1:
+		if (opt[off] == pad1) {
 			optlen = 1;
-			break;
-		default:
+		} else {
 			if (len < 2)
 				goto bad;
-			optlen = opt[off+1]+2;
+
+			optlen = opt[off + 1] + 2;
 			if (len < optlen)
 				goto bad;
-			if (opt[off] & 0x20)
-				memset(&opt[off+2], 0, opt[off+1]);
-			break;
+
+			if (opt[off] & mut_bit)
+				memset(&opt[off + 2], 0, opt[off + 1]);
 		}
 
 		off += optlen;
@@ -140,6 +136,11 @@ static bool zero_out_mutable_opts(struct ipv6_opt_hdr *opthdr)
 	return false;
 }
 
+static bool zero_out_mutable_opts(struct ipv6_opt_hdr *opthdr)
+{
+	return __zero_out_mutable_opts(opthdr, 2, 0x20, IPV6_TLV_PAD1);
+}
+
 #if IS_ENABLED(CONFIG_IPV6_MIP6)
 /**
  *	ipv6_rearrange_destopt - rearrange IPv6 destination options header
-- 
2.7.4

[RFC PATCH 5/6] ah6: Be explicit about which routing types are processed.

[Tom Herbert]

The current code assumes that all routing headers can be processed
as type 0 when rearranging the routing header for AH verification.
Change this to be explicit. Type 0 and type 2 are supported and are
processed the same way with regards to AH.

Also check if rearranging routing header fails. Update reference
in comment to more current RFC.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 net/ipv6/ah6.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 1e80157..032491c 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -145,7 +145,7 @@ static bool zero_out_mutable_opts(struct ipv6_opt_hdr *opthdr)
 /**
  *	ipv6_rearrange_destopt - rearrange IPv6 destination options header
  *	@iph: IPv6 header
- *	@destopt: destionation options header
+ *	@destopt: destination options header
  */
 static void ipv6_rearrange_destopt(struct ipv6hdr *iph, struct ipv6_opt_hdr *destopt)
 {
@@ -204,15 +204,16 @@ static void ipv6_rearrange_destopt(struct ipv6hdr *iph, struct ipv6_opt_hdr *des
 #endif
 
 /**
- *	ipv6_rearrange_rthdr - rearrange IPv6 routing header
+ *	ipv6_rearrange_type0_rthdr - rearrange type 0 IPv6 routing header
  *	@iph: IPv6 header
  *	@rthdr: routing header
  *
  *	Rearrange the destination address in @iph and the addresses in @rthdr
  *	so that they appear in the order they will at the final destination.
- *	See Appendix A2 of RFC 2402 for details.
+ *	See Appendix A2 of RFC 4302 for details.
  */
-static void ipv6_rearrange_rthdr(struct ipv6hdr *iph, struct ipv6_rt_hdr *rthdr)
+static bool ipv6_rearrange_type0_rthdr(struct ipv6hdr *iph,
+				       struct ipv6_rt_hdr *rthdr)
 {
 	int segments, segments_left;
 	struct in6_addr *addrs;
@@ -220,15 +221,13 @@ static void ipv6_rearrange_rthdr(struct ipv6hdr *iph, struct ipv6_rt_hdr *rthdr)
 
 	segments_left = rthdr->segments_left;
 	if (segments_left == 0)
-		return;
+		return true;
 	rthdr->segments_left = 0;
 
 	/* The value of rthdr->hdrlen has been verified either by the system
 	 * call if it is locally generated, or by ipv6_rthdr_rcv() for incoming
 	 * packets.  So we can assume that it is even and that segments is
 	 * greater than or equal to segments_left.
-	 *
-	 * For the same reason we can assume that this option is of type 0.
 	 */
 	segments = rthdr->hdrlen >> 1;
 
@@ -240,6 +239,24 @@ static void ipv6_rearrange_rthdr(struct ipv6hdr *iph, struct ipv6_rt_hdr *rthdr)
 
 	addrs[0] = iph->daddr;
 	iph->daddr = final_addr;
+
+	return true;
+}
+
+static bool ipv6_rearrange_rthdr(struct ipv6hdr *iph, struct ipv6_rt_hdr *rthdr)
+{
+	switch (rthdr->type) {
+	case IPV6_SRCRT_TYPE_2:
+		/* Simplified format of type 0 so same processing */
+		/* fallthrough */
+	case IPV6_SRCRT_TYPE_0: /* Deprecated */
+		return ipv6_rearrange_type0_rthdr(iph, rthdr);
+	default:
+		/* Bad or unidentified routing header, we don't know how
+		 * to fix this header for security purposes. Return failure.
+		 */
+		return false;
+	}
 }
 
 static int ipv6_clear_mutable_options(struct ipv6hdr *iph, int len, int dir)
@@ -271,7 +288,11 @@ static int ipv6_clear_mutable_options(struct ipv6hdr *iph, int len, int dir)
 			break;
 
 		case NEXTHDR_ROUTING:
-			ipv6_rearrange_rthdr(iph, exthdr.rth);
+			if (!ipv6_rearrange_rthdr(iph, exthdr.rth)) {
+				net_dbg_ratelimited("bad routing header\n");
+				return -EINVAL;
+			}
+
 			break;
 
 		default:
-- 
2.7.4

[RFC PATCH 6/6] seg6: Add support to rearrange SRH for AH ICV calculation

[Tom Herbert]

Mutable fields related to segment routing are: destination address,
segments left, and modifiable TLVs (those whose high order bit is set).

Add support to rearrange a segment routing (type 4) routing header to
handle these mutability requirements. This is described in
draft-herbert-ipv6-srh-ah-00.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 net/ipv6/ah6.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 032491c..0c5ca29 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -27,6 +27,7 @@
 #include <net/icmp.h>
 #include <net/ipv6.h>
 #include <net/protocol.h>
+#include <net/seg6.h>
 #include <net/xfrm.h>
 
 #define IPV6HDR_BASELEN 8
@@ -141,6 +142,13 @@ static bool zero_out_mutable_opts(struct ipv6_opt_hdr *opthdr)
 	return __zero_out_mutable_opts(opthdr, 2, 0x20, IPV6_TLV_PAD1);
 }
 
+static bool zero_out_mutable_srh_opts(struct ipv6_sr_hdr *srh)
+{
+	return __zero_out_mutable_opts((struct ipv6_opt_hdr *)srh,
+				       seg6_tlv_offset(srh), 0x80,
+				       SR6_TLV_PAD1);
+}
+
 #if IS_ENABLED(CONFIG_IPV6_MIP6)
 /**
  *	ipv6_rearrange_destopt - rearrange IPv6 destination options header
@@ -243,6 +251,20 @@ static bool ipv6_rearrange_type0_rthdr(struct ipv6hdr *iph,
 	return true;
 }
 
+static bool ipv6_rearrange_type4_rthdr(struct ipv6hdr *iph,
+				       struct ipv6_rt_hdr *rthdr)
+{
+	struct ipv6_sr_hdr *srh = (struct ipv6_sr_hdr *)rthdr;
+
+	if (!zero_out_mutable_srh_opts(srh))
+		return false;
+
+	rthdr->segments_left = 0;
+	iph->daddr = srh->segments[0];
+
+	return true;
+}
+
 static bool ipv6_rearrange_rthdr(struct ipv6hdr *iph, struct ipv6_rt_hdr *rthdr)
 {
 	switch (rthdr->type) {
@@ -251,6 +273,8 @@ static bool ipv6_rearrange_rthdr(struct ipv6hdr *iph, struct ipv6_rt_hdr *rthdr)
 		/* fallthrough */
 	case IPV6_SRCRT_TYPE_0: /* Deprecated */
 		return ipv6_rearrange_type0_rthdr(iph, rthdr);
+	case IPV6_SRCRT_TYPE_4:
+		return ipv6_rearrange_type4_rthdr(iph, rthdr);
 	default:
 		/* Bad or unidentified routing header, we don't know how
 		 * to fix this header for security purposes. Return failure.
-- 
2.7.4

Re: [RFC PATCH 6/6] seg6: Add support to rearrange SRH for AH ICV calculation

[Ahmed Abdelsalam]

On Fri, 31 May 2019 09:48:40 -0700
Tom Herbert <tom@herbertland.com> wrote:

> Mutable fields related to segment routing are: destination address,
> segments left, and modifiable TLVs (those whose high order bit is set).
> 
> Add support to rearrange a segment routing (type 4) routing header to
> handle these mutability requirements. This is described in
> draft-herbert-ipv6-srh-ah-00.

Hi Tom, 
Assuming that IETF process needs to be fixed, then, IMO, should not be on the cost of breaking the kernel process here. 
Let us add to the kernel things that have been reviewed and reached some consensus.
For new features that still need to be reviewed we can have them outside the kernel tree for community to use. 
This way the community does not get blocked by IETF process but also keep the kernel tree stable.
Thanks,
Ahmed

-- 
Ahmed Abdelsalam <ahabdels.dev@gmail.com>

Re: [RFC PATCH 6/6] seg6: Add support to rearrange SRH for AH ICV calculation

[Tom Herbert]

On Fri, May 31, 2019 at 10:07 AM Ahmed Abdelsalam
<ahabdels.dev@gmail.com> wrote:
>
> On Fri, 31 May 2019 09:48:40 -0700
> Tom Herbert <tom@herbertland.com> wrote:
>
> > Mutable fields related to segment routing are: destination address,
> > segments left, and modifiable TLVs (those whose high order bit is set).
> >
> > Add support to rearrange a segment routing (type 4) routing header to
> > handle these mutability requirements. This is described in
> > draft-herbert-ipv6-srh-ah-00.
>
> Hi Tom,
> Assuming that IETF process needs to be fixed, then, IMO, should not be on the cost of breaking the kernel process here.

Ahmed,

I do not see how this is any way breaking the kernel process. The
kernel is beholden to the needs of users provide a robust and secure
implementations, not to some baroque IETF or other SDO processes. When
those are in conflict, the needs of our users should prevail.

> Let us add to the kernel things that have been reviewed and reached some consensus.

By that argument, segment routing should never have been added to the
kernel since consensus has not be reached on it yet or at least
portions of it. In fact, if you look at this patch set, most of the
changes are actually bug fixes to bring the implementation into
conformance with a later version of the draft. For instance, there was
never consensus reached on the HMAC flag; now it's gone and we need to
remove it from the implementation.

> For new features that still need to be reviewed we can have them outside the kernel tree for community to use.
> This way the community does not get blocked by IETF process but also keep the kernel tree stable.

In any case, that does not address the issue of a user using both
segment routing and authentication which leads to adverse behaviors.
AFAICT, the kernel does not prevent this today. So I ask again: what
is your alternative to address this?

Thanks,
Tom

> Thanks,
> Ahmed
>
> --
> Ahmed Abdelsalam <ahabdels.dev@gmail.com>

Re: [RFC PATCH 6/6] seg6: Add support to rearrange SRH for AH ICV calculation

[Ahmed Abdelsalam]

On Fri, 31 May 2019 10:34:03 -0700
Tom Herbert <tom@herbertland.com> wrote:

> On Fri, May 31, 2019 at 10:07 AM Ahmed Abdelsalam
> <ahabdels.dev@gmail.com> wrote:
> >
> > On Fri, 31 May 2019 09:48:40 -0700
> > Tom Herbert <tom@herbertland.com> wrote:
> >
> > > Mutable fields related to segment routing are: destination address,
> > > segments left, and modifiable TLVs (those whose high order bit is set).
> > >
> > > Add support to rearrange a segment routing (type 4) routing header to
> > > handle these mutability requirements. This is described in
> > > draft-herbert-ipv6-srh-ah-00.
> >
> > Hi Tom,
> > Assuming that IETF process needs to be fixed, then, IMO, should not be on the cost of breaking the kernel process here.
> 
> Ahmed,
> 
> I do not see how this is any way breaking the kernel process. The
> kernel is beholden to the needs of users provide a robust and secure
> implementations, not to some baroque IETF or other SDO processes. When
> those are in conflict, the needs of our users should prevail.
> 
> > Let us add to the kernel things that have been reviewed and reached some consensus.
> 
> By that argument, segment routing should never have been added to the
> kernel since consensus has not be reached on it yet or at least
> portions of it. In fact, if you look at this patch set, most of the
> changes are actually bug fixes to bring the implementation into
> conformance with a later version of the draft. For instance, there was
> never consensus reached on the HMAC flag; now it's gone and we need to
> remove it from the implementation.
> 
> > For new features that still need to be reviewed we can have them outside the kernel tree for community to use.
> > This way the community does not get blocked by IETF process but also keep the kernel tree stable.
> 
> In any case, that does not address the issue of a user using both
> segment routing and authentication which leads to adverse behaviors.
> AFAICT, the kernel does not prevent this today. So I ask again: what
> is your alternative to address this?
> 
> Thanks,
> Tom

Tom,
Yes, the needs for users should prevail. But it’s not Tom or Ahmed alone who should define users needs. 
The comparison between "draft-herbert-ipv6-srh-ah-00" and "draft-ietf-6man-segment-routing-header" is
missing some facts. The first patch of the SRH implementation was submitted to the kernel two years after
releasing the SRH draft. By this time, the draft was a working group adopted and co-authored by several
vendors, operators and academia. Please refer to the first SRH patch submitted to the kernel
(https://patchwork.ozlabs.org/patch/663176/). I still don’t see the point of rushing to upstream something 
that has been defined couple of days ago. Plus there is nothing that prevents anyone to "innovate" in his 
own private kernel tree.

-- 
Ahmed Abdelsalam <ahabdels.dev@gmail.com>

Re: [RFC PATCH 6/6] seg6: Add support to rearrange SRH for AH ICV calculation

[Tom Herbert]

On Sun, Jun 2, 2019 at 2:54 AM Ahmed Abdelsalam <ahabdels.dev@gmail.com> wrote:
>
> On Fri, 31 May 2019 10:34:03 -0700
> Tom Herbert <tom@herbertland.com> wrote:
>
> > On Fri, May 31, 2019 at 10:07 AM Ahmed Abdelsalam
> > <ahabdels.dev@gmail.com> wrote:
> > >
> > > On Fri, 31 May 2019 09:48:40 -0700
> > > Tom Herbert <tom@herbertland.com> wrote:
> > >
> > > > Mutable fields related to segment routing are: destination address,
> > > > segments left, and modifiable TLVs (those whose high order bit is set).
> > > >
> > > > Add support to rearrange a segment routing (type 4) routing header to
> > > > handle these mutability requirements. This is described in
> > > > draft-herbert-ipv6-srh-ah-00.
> > >
> > > Hi Tom,
> > > Assuming that IETF process needs to be fixed, then, IMO, should not be on the cost of breaking the kernel process here.
> >
> > Ahmed,
> >
> > I do not see how this is any way breaking the kernel process. The
> > kernel is beholden to the needs of users provide a robust and secure
> > implementations, not to some baroque IETF or other SDO processes. When
> > those are in conflict, the needs of our users should prevail.
> >
> > > Let us add to the kernel things that have been reviewed and reached some consensus.
> >
> > By that argument, segment routing should never have been added to the
> > kernel since consensus has not be reached on it yet or at least
> > portions of it. In fact, if you look at this patch set, most of the
> > changes are actually bug fixes to bring the implementation into
> > conformance with a later version of the draft. For instance, there was
> > never consensus reached on the HMAC flag; now it's gone and we need to
> > remove it from the implementation.
> >
> > > For new features that still need to be reviewed we can have them outside the kernel tree for community to use.
> > > This way the community does not get blocked by IETF process but also keep the kernel tree stable.
> >
> > In any case, that does not address the issue of a user using both
> > segment routing and authentication which leads to adverse behaviors.
> > AFAICT, the kernel does not prevent this today. So I ask again: what
> > is your alternative to address this?
> >
> > Thanks,
> > Tom
>
> Tom,
> Yes, the needs for users should prevail. But it’s not Tom or Ahmed alone who should define users needs.
> The comparison between "draft-herbert-ipv6-srh-ah-00" and "draft-ietf-6man-segment-routing-header" is
> missing some facts. The first patch of the SRH implementation was submitted to the kernel two years after
> releasing the SRH draft. By this time, the draft was a working group adopted and co-authored by several
> vendors, operators and academia. Please refer to the first SRH patch submitted to the kernel
> (https://patchwork.ozlabs.org/patch/663176/). I still don’t see the point of rushing to upstream something
> that has been defined couple of days ago. Plus there is nothing that prevents anyone to "innovate" in his
> own private kernel tree.

Ahmed,

While you seem to think that was just defined and came out the blue a
few days ago, in fact this has been in discussion for many months. The
simultaneous use of segment routing and authentication header was not
defined-- but it is defined for other routing types and extension
headers. The primary drivers of segment routing (the academics,
operators, and vendors you refer to) were reluctant to do this. For
the most part, these are mostly routing vendors who don't care about
preserving end-to-end host functionality like AH. In order to define
an interoperable protocol, the mutability of fields needs to be
defined. They were unwilling to commit to defining what is mutable in
their protocol, and it took an intervening action of the working group
chairs to force them to clarify the requirements so now we have
something.

IMO, this is straightforward bug fix. If you want to say that we need
to wait for IETF to take action, okay, but then I strongly suggest
that you actively participate in the process (i.e. send to 6man list
what you think about the draft), as opposed to just passively
deferring to it and assuming others will do the right thing.

Tom

>
> --
> Ahmed Abdelsalam <ahabdels.dev@gmail.com>