pull request (net): ipsec 2018-05-07

PATCH New Steffen Klassert 2 participants 5 messages May 7, 2018 – May 8, 2018

Subsystems (4)

Networking Network drivers IPv4/IPv6 Networking IPsec

lore

Share & tools

lore

Steffen Klassert

author maintainer

2018-05-07 08:43 UTC #0 lore raw

1) Always verify length of provided sadb_key to fix a slab-out-of-bounds read in pfkey_add. From Kevin Easton.

2) Make sure that all states are really deleted before we check that the state lists are empty. Otherwise we trigger a warning.

3) Fix MTU handling of the VTI6 interfaces on
interfamily tunnels. From Stefano Brivio.

Please pull or let me know if there are problems.

Thanks!

The following changes since commit 76327a35caabd1a932e83d6a42b967aa08584e5d:

dp83640: Ensure against premature access to PHY registers after reset (2018-04-08 19:58:52 -0400)

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 b4331a681822b420511b3258f1c3db35001fde48:

vti6: Change minimum MTU to IPV4_MIN_MTU, vti6 can carry IPv4 too (2018-04-27 07:29:23 +0200)

----------------------------------------------------------------
Kevin Easton (1):
af_key: Always verify length of provided sadb_key

Stefano Brivio (1):
vti6: Change minimum MTU to IPV4_MIN_MTU, vti6 can carry IPv4 too

Steffen Klassert (1):
xfrm: Fix warning in xfrm6_tunnel_net_exit.

 include/net/xfrm.h      |  1 +
 net/ipv6/ip6_vti.c      |  4 ++--
 net/ipv6/xfrm6_tunnel.c |  3 +++
 net/key/af_key.c        | 45 +++++++++++++++++++++++++++++++++++----------
 net/xfrm/xfrm_state.c   |  6 ++++++
 5 files changed, 47 insertions(+), 12 deletions(-)

Steffen Klassert

author maintainer

2018-05-07 08:43 UTC #1 lore raw

PATCH 1/3 af_key: Always verify length of provided sadb_key

From: Kevin Easton <[email protected]>

Key extensions (struct sadb_key) include a user-specified number of key bits. The kernel uses that number to determine how much key data to copy out of the message in pfkey_msg2xfrm_state().

The length of the sadb_key message must be verified to be long enough, even in the case of SADB_X_AALG_NULL. Furthermore, the sadb_key_len value must be long enough to include both the key data and the struct sadb_key itself.

Introduce a helper function verify_key_len(), and call it from parse_exthdrs() where other exthdr types are similarly checked for correctness.

Signed-off-by Kevin Easton <[email protected]>

Reported-by syzbot+5022a34ca5a3d49b84223653fab632dfb7b4cf37@syzkaller.appspotmail.com

Signed-off-by Steffen Klassert <[email protected]>

---
 net/key/af_key.c | 45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

net/key/af_key.c +35 ~~-10~~

index 7e2e7188e7f4..e62e52e8f141 100644
@@ -437,6 +437,24 @@ static int verify_address_len(const void *p)
 	return 0;
 }
 
+static inline int sadb_key_len(const struct sadb_key *key)
+{
+	int key_bytes = DIV_ROUND_UP(key->sadb_key_bits, 8);
+
+	return DIV_ROUND_UP(sizeof(struct sadb_key) + key_bytes,
+			    sizeof(uint64_t));
+}
+
+static int verify_key_len(const void *p)
+{
+	const struct sadb_key *key = p;
+
+	if (sadb_key_len(key) > key->sadb_key_len)
+		return -EINVAL;
+
+	return 0;
+}
+
 static inline int pfkey_sec_ctx_len(const struct sadb_x_sec_ctx *sec_ctx)
 {
 	return DIV_ROUND_UP(sizeof(struct sadb_x_sec_ctx) +

@@ -533,16 +551,25 @@ static int parse_exthdrs(struct sk_buff *skb, const struct sadb_msg *hdr, void *
 				return -EINVAL;
 			if (ext_hdrs[ext_type-1] != NULL)
 				return -EINVAL;
-			if (ext_type == SADB_EXT_ADDRESS_SRC ||
-			    ext_type == SADB_EXT_ADDRESS_DST ||
-			    ext_type == SADB_EXT_ADDRESS_PROXY ||
-			    ext_type == SADB_X_EXT_NAT_T_OA) {
+			switch (ext_type) {
+			case SADB_EXT_ADDRESS_SRC:
+			case SADB_EXT_ADDRESS_DST:
+			case SADB_EXT_ADDRESS_PROXY:
+			case SADB_X_EXT_NAT_T_OA:
 				if (verify_address_len(p))
 					return -EINVAL;
-			}
-			if (ext_type == SADB_X_EXT_SEC_CTX) {
+				break;
+			case SADB_X_EXT_SEC_CTX:
 				if (verify_sec_ctx_len(p))
 					return -EINVAL;
+				break;
+			case SADB_EXT_KEY_AUTH:
+			case SADB_EXT_KEY_ENCRYPT:
+				if (verify_key_len(p))
+					return -EINVAL;
+				break;
+			default:
+				break;
 			}
 			ext_hdrs[ext_type-1] = (void *) p;
 		}

@@ -1104,14 +1131,12 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 	key = ext_hdrs[SADB_EXT_KEY_AUTH - 1];
 	if (key != NULL &&
 	    sa->sadb_sa_auth != SADB_X_AALG_NULL &&
-	    ((key->sadb_key_bits+7) / 8 == 0 ||
-	     (key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+	    key->sadb_key_bits == 0)
 		return ERR_PTR(-EINVAL);
 	key = ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
 	if (key != NULL &&
 	    sa->sadb_sa_encrypt != SADB_EALG_NULL &&
-	    ((key->sadb_key_bits+7) / 8 == 0 ||
-	     (key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+	    key->sadb_key_bits == 0)
 		return ERR_PTR(-EINVAL);
 
 	x = xfrm_state_alloc(net);

Signature · 1 line

2.14.1

Steffen Klassert

author maintainer

2018-05-07 08:43 UTC #2 lore raw

PATCH 2/3 xfrm: Fix warning in xfrm6_tunnel_net_exit.

In reply to Steffen Klassert

We need to make sure that all states are really deleted before we check that the state lists are empty. Otherwise we trigger a warning.

Fixes baeb0dbbb5659 ("xfrm6_tunnel: exit_net cleanup check added")

Reported-and-tested-by [email protected]

Signed-off-by Steffen Klassert <[email protected]>

---
 include/net/xfrm.h      | 1 +
 net/ipv6/xfrm6_tunnel.c | 3 +++
 net/xfrm/xfrm_state.c   | 6 ++++++
 3 files changed, 10 insertions(+)

include/net/xfrm.h +1 -0

index a872379b69da..45e75c36b738 100644
@@ -375,6 +375,7 @@ struct xfrm_input_afinfo {
 int xfrm_input_register_afinfo(const struct xfrm_input_afinfo *afinfo);
 int xfrm_input_unregister_afinfo(const struct xfrm_input_afinfo *afinfo);
 
+void xfrm_flush_gc(void);
 void xfrm_state_delete_tunnel(struct xfrm_state *x);
 
 struct xfrm_type {

net/ipv6/xfrm6_tunnel.c +3 -0

index f85f0d7480ac..4a46df8441c9 100644
@@ -341,6 +341,9 @@ static void __net_exit xfrm6_tunnel_net_exit(struct net *net)
 	struct xfrm6_tunnel_net *xfrm6_tn = xfrm6_tunnel_pernet(net);
 	unsigned int i;
 
+	xfrm_state_flush(net, IPSEC_PROTO_ANY, false);
+	xfrm_flush_gc();
+
 	for (i = 0; i < XFRM6_TUNNEL_SPI_BYADDR_HSIZE; i++)
 		WARN_ON_ONCE(!hlist_empty(&xfrm6_tn->spi_byaddr[i]));

net/xfrm/xfrm_state.c +6 -0

index f9d2f2233f09..6c177ae7a6d9 100644
@@ -2175,6 +2175,12 @@ struct xfrm_state_afinfo *xfrm_state_get_afinfo(unsigned int family)
 	return afinfo;
 }
 
+void xfrm_flush_gc(void)
+{
+	flush_work(&xfrm_state_gc_work);
+}
+EXPORT_SYMBOL(xfrm_flush_gc);
+
 /* Temporarily located here until net/xfrm/xfrm_tunnel.c is created */
 void xfrm_state_delete_tunnel(struct xfrm_state *x)
 {

Signature · 1 line

2.14.1

Steffen Klassert

author maintainer

2018-05-07 08:43 UTC #3 lore raw

PATCH 3/3 vti6: Change minimum MTU to IPV4_MIN_MTU, vti6 can carry IPv4 too

In reply to Steffen Klassert

From: Stefano Brivio <[email protected]>

A vti6 interface can carry IPv4 as well, so it makes no sense to enforce a minimum MTU of IPV6_MIN_MTU.

If the user sets an MTU below IPV6_MIN_MTU, IPv6 will be disabled on the interface, courtesy of addrconf_notify().

Reported-by Xin Long <[email protected]>

Fixes b96f9afee4eb ("ipv4/6: use core net MTU range checking")

Fixes c6741fbed6dc ("vti6: Properly adjust vti6 MTU from MTU of lower device")

Fixes 53c81e95df17 ("ip6_vti: adjust vti mtu according to mtu of lower device")

Signed-off-by Stefano Brivio <[email protected]>

Signed-off-by Steffen Klassert <[email protected]>

---
 net/ipv6/ip6_vti.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/ipv6/ip6_vti.c +2 -2

index c214ffec02f0..ca957dd93a29 100644
@@ -669,7 +669,7 @@ static void vti6_link_config(struct ip6_tnl *t, bool keep_mtu)
 	else
 		mtu = ETH_DATA_LEN - LL_MAX_HEADER - sizeof(struct ipv6hdr);
 
-	dev->mtu = max_t(int, mtu, IPV6_MIN_MTU);
+	dev->mtu = max_t(int, mtu, IPV4_MIN_MTU);
 }
 
 /**

@@ -881,7 +881,7 @@ static void vti6_dev_setup(struct net_device *dev)
 	dev->priv_destructor = vti6_dev_free;
 
 	dev->type = ARPHRD_TUNNEL6;
-	dev->min_mtu = IPV6_MIN_MTU;
+	dev->min_mtu = IPV4_MIN_MTU;
 	dev->max_mtu = IP_MAX_MTU - sizeof(struct ipv6hdr);
 	dev->flags |= IFF_NOARP;
 	dev->addr_len = sizeof(struct in6_addr);

Signature · 1 line

2.14.1

David Miller

maintainer

2018-05-08 03:52 UTC #4 lore raw

In reply to Steffen Klassert

From: Steffen Klassert <[email protected]>
Date: Mon, 7 May 2018 10:43:20 +0200

Quoted 11 lines

1) Always verify length of provided sadb_key to fix a slab-out-of-bounds read in pfkey_add. From Kevin Easton.
2) Make sure that all states are really deleted before we check that the state lists are empty. Otherwise we trigger a warning.
3) Fix MTU handling of the VTI6 interfaces on
interfamily tunnels. From Stefano Brivio.
Please pull or let me know if there are problems.

Pulled, thanks Steffen.