pull request (net): ipsec 2017-12-22

1) Check for valid id proto in validate_tmpl(), otherwise we may trigger a warning in xfrm_state_fini(). From Cong Wang.

2) Fix a typo on XFRMA_OUTPUT_MARK policy attribute. From Michal Kubecek.

3) Verify the state is valid when encap_type < 0,
otherwise we may crash on IPsec GRO .
From Aviv Heller.

4) Fix stack-out-of-bounds read on socket policy lookup. We access the flowi of the wrong address family in the IPv4 mapped IPv6 case, fix this by catching address family missmatches before we do the lookup.

5) fix xfrm_do_migrate() with AEAD to copy the geniv field too. Otherwise the state is not fully initialized and migration fails. From Antony Antony.

6) Fix stack-out-of-bounds with misconfigured transport mode policies. Our policy template validation is not strict enough. It is possible to configure policies with transport mode template where the address family of the template does not match the selectors address family. Fix this by refusing such a configuration, address family can not change on transport mode.

7) Fix a policy reference leak when reusing pcpu xdst entry. From Florian Westphal.

8) Reinject transport-mode packets through tasklet, otherwise it is possible to reate a recursion loop. From Herbert Xu.

Please pull or let me know if there are problems.

Thanks!

The following changes since commit d51aae68b142f48232257e96ce317db25445418d:

net: sched: cbq: create block for q->link.block (2017-11-28 16:04:26 -0500)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master

for you to fetch changes up to acf568ee859f098279eadf551612f103afdacb4e:

xfrm: Reinject transport-mode packets through tasklet (2017-12-19 08:23:21 +0100)

----------------------------------------------------------------
Antony Antony (1):
xfrm: fix xfrm_do_migrate() with AEAD e.g(AES-GCM)

Aviv Heller (1):
xfrm: Fix xfrm_input() to verify state is valid when (encap_type < 0)

Cong Wang (1):
xfrm: check id proto in validate_tmpl()

Florian Westphal (1):
xfrm: put policies when reusing pcpu xdst entry

Herbert Xu (1):
xfrm: Reinject transport-mode packets through tasklet

Michal Kubecek (1):
xfrm: fix XFRMA_OUTPUT_MARK policy entry

Steffen Klassert (2):
xfrm: Fix stack-out-of-bounds read on socket policy lookup.
xfrm: Fix stack-out-of-bounds with misconfigured transport mode policies.

 include/net/xfrm.h     |  3 +++
 net/ipv4/xfrm4_input.c | 12 ++++++++-
 net/ipv6/xfrm6_input.c | 10 +++++++-
 net/xfrm/xfrm_input.c  | 69 +++++++++++++++++++++++++++++++++++++++++++++++++-
 net/xfrm/xfrm_policy.c |  9 ++++++-
 net/xfrm/xfrm_state.c  |  1 +
 net/xfrm/xfrm_user.c   | 26 ++++++++++++++++++-
 7 files changed, 125 insertions(+), 5 deletions(-)

From: Cong Wang <[email protected]>

syzbot reported a kernel warning in xfrm_state_fini(), which indicates that we have entries left in the list net->xfrm.state_all whose proto is zero. And xfrm_id_proto_match() doesn't consider them as a match with IPSEC_PROTO_ANY in this case.

Proto with value 0 is probably not a valid value, at least verify_newsa_info() doesn't consider it valid either.

This patch fixes it by checking the proto value in validate_tmpl() and rejecting invalid ones, like what iproute2 does in xfrm_xfrmproto_getbyname().

---
 net/xfrm/xfrm_user.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

index 983b0233767b..c2cfcc6fdb34 100644
@@ -1445,6 +1445,21 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
 		default:
 			return -EINVAL;
 		}
+
+		switch (ut[i].id.proto) {
+		case IPPROTO_AH:
+		case IPPROTO_ESP:
+		case IPPROTO_COMP:
+#if IS_ENABLED(CONFIG_IPV6)
+		case IPPROTO_ROUTING:
+		case IPPROTO_DSTOPTS:
+#endif
+		case IPSEC_PROTO_ANY:
+			break;
+		default:
+			return -EINVAL;
+		}
+
 	}
 
 	return 0;

2.14.1

From: Michal Kubecek <[email protected]>

This seems to be an obvious typo, NLA_U32 is type of the attribute, not its (minimal) length.

---
 net/xfrm/xfrm_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c2cfcc6fdb34..ff58c37469d6 100644
@@ -2485,7 +2485,7 @@ static const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
 	[XFRMA_PROTO]		= { .type = NLA_U8 },
 	[XFRMA_ADDRESS_FILTER]	= { .len = sizeof(struct xfrm_address_filter) },
 	[XFRMA_OFFLOAD_DEV]	= { .len = sizeof(struct xfrm_user_offload) },
-	[XFRMA_OUTPUT_MARK]	= { .len = NLA_U32 },
+	[XFRMA_OUTPUT_MARK]	= { .type = NLA_U32 },
 };
 
 static const struct nla_policy xfrma_spd_policy[XFRMA_SPD_MAX+1] = {

2.14.1

From: Aviv Heller <[email protected]>

Code path when (encap_type < 0) does not verify the state is valid before progressing.

This will result in a crash if, for instance, x->km.state == XFRM_STATE_ACQ.

---
 net/xfrm/xfrm_input.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

index 347ab31574d5..da6447389ffb 100644
@@ -207,7 +207,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	xfrm_address_t *daddr;
 	struct xfrm_mode *inner_mode;
 	u32 mark = skb->mark;
-	unsigned int family;
+	unsigned int family = AF_UNSPEC;
 	int decaps = 0;
 	int async = 0;
 	bool xfrm_gro = false;

@@ -216,6 +216,16 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 	if (encap_type < 0) {
 		x = xfrm_input_state(skb);
+
+		if (unlikely(x->km.state != XFRM_STATE_VALID)) {
+			if (x->km.state == XFRM_STATE_ACQ)
+				XFRM_INC_STATS(net, LINUX_MIB_XFRMACQUIREERROR);
+			else
+				XFRM_INC_STATS(net,
+					       LINUX_MIB_XFRMINSTATEINVALID);
+			goto drop;
+		}
+
 		family = x->outer_mode->afinfo->family;
 
 		/* An encap_type of -1 indicates async resumption. */

2.14.1

When we do tunnel or beet mode, we pass saddr and daddr from the template to xfrm_state_find(), this is ok. On transport mode, we pass the addresses from the flowi, assuming that the IP addresses (and address family) don't change during transformation. This assumption is wrong in the IPv4 mapped IPv6 case, packet is IPv4 and template is IPv6.

Fix this by catching address family missmatches of the policy and the flow already before we do the lookup.

---
 net/xfrm/xfrm_policy.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 9542975eb2f9..038ec68f6901 100644
@@ -1168,9 +1168,15 @@ static struct xfrm_policy *xfrm_sk_policy_lookup(const struct sock *sk, int dir,
  again:
 	pol = rcu_dereference(sk->sk_policy[dir]);
 	if (pol != NULL) {
-		bool match = xfrm_selector_match(&pol->selector, fl, family);
+		bool match;
 		int err = 0;
 
+		if (pol->family != family) {
+			pol = NULL;
+			goto out;
+		}
+
+		match = xfrm_selector_match(&pol->selector, fl, family);
 		if (match) {
 			if ((sk->sk_mark & pol->mark.m) != pol->mark.v) {
 				pol = NULL;

2.14.1

From: Antony Antony <[email protected]>

copy geniv when cloning the xfrm state.

x->geniv was not copied to the new state and migration would fail.

xfrm_do_migrate
..
xfrm_state_clone()
..
..
esp_init_aead()
crypto_alloc_aead()
crypto_alloc_tfm()
crypto_find_alg() return EAGAIN and failed

---
 net/xfrm/xfrm_state.c | 1 +
 1 file changed, 1 insertion(+)

index 1f5cee2269af..88d0a563e141 100644
@@ -1344,6 +1344,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
 
 	if (orig->aead) {
 		x->aead = xfrm_algo_aead_clone(orig->aead);
+		x->geniv = orig->geniv;
 		if (!x->aead)
 			goto error;
 	}

2.14.1

On policies with a transport mode template, we pass the addresses from the flowi to xfrm_state_find(), assuming that the IP addresses (and address family) don't change during transformation.

Unfortunately our policy template validation is not strict enough. It is possible to configure policies with transport mode template where the address family of the template does not match the selectors address family. This lead to stack-out-of-bound reads because we compare arddesses of the wrong family. Fix this by refusing such a configuration, address family can not change on transport mode.

We use the assumption that, on transport mode, the first templates address family must match the address family of the policy selector. Subsequent transport mode templates must mach the address family of the previous template.

---
 net/xfrm/xfrm_user.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index ff58c37469d6..bdb48e5dba04 100644
@@ -1419,11 +1419,14 @@ static void copy_templates(struct xfrm_policy *xp, struct xfrm_user_tmpl *ut,
 
 static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
 {
+	u16 prev_family;
 	int i;
 
 	if (nr > XFRM_MAX_DEPTH)
 		return -EINVAL;
 
+	prev_family = family;
+
 	for (i = 0; i < nr; i++) {
 		/* We never validated the ut->family value, so many
 		 * applications simply leave it at zero.  The check was

@@ -1435,6 +1438,12 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
 		if (!ut[i].family)
 			ut[i].family = family;
 
+		if ((ut[i].mode == XFRM_MODE_TRANSPORT) &&
+		    (ut[i].family != prev_family))
+			return -EINVAL;
+
+		prev_family = ut[i].family;
+
 		switch (ut[i].family) {
 		case AF_INET:
 			break;

2.14.1

From: Florian Westphal <[email protected]>

We need to put the policies when re-using the pcpu xdst entry, else this leaks the reference.

---
 net/xfrm/xfrm_policy.c | 1 +
 1 file changed, 1 insertion(+)

index 038ec68f6901..70aa5cb0c659 100644
@@ -1839,6 +1839,7 @@ xfrm_resolve_and_create_bundle(struct xfrm_policy **pols, int num_pols,
 		   sizeof(struct xfrm_policy *) * num_pols) == 0 &&
 	    xfrm_xdst_can_reuse(xdst, xfrm, err)) {
 		dst_hold(&xdst->u.dst);
+		xfrm_pols_put(pols, num_pols);
 		while (err > 0)
 			xfrm_state_put(xfrm[--err]);
 		return xdst;

2.14.1

From: Herbert Xu <[email protected]>

This is an old bugbear of mine:

https://www.mail-archive.com/[email protected]/msg03894.html

By crafting special packets, it is possible to cause recursion in our kernel when processing transport-mode packets at levels that are only limited by packet size.

The easiest one is with DNAT, but an even worse one is where UDP encapsulation is used in which case you just have to insert an UDP encapsulation header in between each level of recursion.

This patch avoids this problem by reinjecting tranport-mode packets through a tasklet.

---
 include/net/xfrm.h     |  3 +++
 net/ipv4/xfrm4_input.c | 12 ++++++++++-
 net/ipv6/xfrm6_input.c | 10 ++++++++-
 net/xfrm/xfrm_input.c  | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 80 insertions(+), 2 deletions(-)

index dc28a98ce97c..ae35991b5877 100644
@@ -1570,6 +1570,9 @@ int xfrm_init_state(struct xfrm_state *x);
 int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb);
 int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type);
 int xfrm_input_resume(struct sk_buff *skb, int nexthdr);
+int xfrm_trans_queue(struct sk_buff *skb,
+		     int (*finish)(struct net *, struct sock *,
+				   struct sk_buff *));
 int xfrm_output_resume(struct sk_buff *skb, int err);
 int xfrm_output(struct sock *sk, struct sk_buff *skb);
 int xfrm_inner_extract_output(struct xfrm_state *x, struct sk_buff *skb);

index e50b7fea57ee..bcfc00e88756 100644
@@ -23,6 +23,12 @@ int xfrm4_extract_input(struct xfrm_state *x, struct sk_buff *skb)
 	return xfrm4_extract_header(skb);
 }
 
+static int xfrm4_rcv_encap_finish2(struct net *net, struct sock *sk,
+				   struct sk_buff *skb)
+{
+	return dst_input(skb);
+}
+
 static inline int xfrm4_rcv_encap_finish(struct net *net, struct sock *sk,
 					 struct sk_buff *skb)
 {

@@ -33,7 +39,11 @@ static inline int xfrm4_rcv_encap_finish(struct net *net, struct sock *sk,
 					 iph->tos, skb->dev))
 			goto drop;
 	}
-	return dst_input(skb);
+
+	if (xfrm_trans_queue(skb, xfrm4_rcv_encap_finish2))
+		goto drop;
+
+	return 0;
 drop:
 	kfree_skb(skb);
 	return NET_RX_DROP;

index fe04e23af986..841f4a07438e 100644
@@ -32,6 +32,14 @@ int xfrm6_rcv_spi(struct sk_buff *skb, int nexthdr, __be32 spi,
 }
 EXPORT_SYMBOL(xfrm6_rcv_spi);
 
+static int xfrm6_transport_finish2(struct net *net, struct sock *sk,
+				   struct sk_buff *skb)
+{
+	if (xfrm_trans_queue(skb, ip6_rcv_finish))
+		__kfree_skb(skb);
+	return -1;
+}
+
 int xfrm6_transport_finish(struct sk_buff *skb, int async)
 {
 	struct xfrm_offload *xo = xfrm_offload(skb);

@@ -56,7 +64,7 @@ int xfrm6_transport_finish(struct sk_buff *skb, int async)
 
 	NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING,
 		dev_net(skb->dev), NULL, skb, skb->dev, NULL,
-		ip6_rcv_finish);
+		xfrm6_transport_finish2);
 	return -1;
 }
 

2.14.1

From: Steffen Klassert <[email protected]>
Date: Fri, 22 Dec 2017 10:44:53 +0100

Pulled, thank you very much!