pull request (net): ipsec 2018-12-18

1) Fix error return code in xfrm_output_one()
when no dst_entry is attached to the skb.
From Wei Yongjun.

2) The xfrm state hash bucket count reported to userspace is off by one. Fix from Benjamin Poirier.

3) Fix NULL pointer dereference in xfrm_input when skb_dst_force clears the dst_entry.

4) Fix freeing of xfrm states on acquire. We use a dedicated slab cache for the xfrm states now, so free it properly with kmem_cache_free. From Mathias Krause.

Please pull or let me know if there are problems.

Thanks!

The following changes since commit 6788fac82001ed1944b5da976bcec4a7b9accf51:

Merge git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf (2018-10-26 21:41:49 -0700)

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 4a135e538962cb00a9667c82e7d2b9e4d7cd7177:

xfrm_user: fix freeing of xfrm states on acquire (2018-11-23 07:51:32 +0100)

----------------------------------------------------------------
Benjamin Poirier (1):
xfrm: Fix bucket count reported to userspace

Mathias Krause (1):
xfrm_user: fix freeing of xfrm states on acquire

Steffen Klassert (1):
xfrm: Fix NULL pointer dereference in xfrm_input when skb_dst_force clears the dst_entry.

Wei Yongjun (1):
xfrm: Fix error return code in xfrm_output_one()

 include/net/xfrm.h     |  1 +
 net/xfrm/xfrm_input.c  |  7 ++++++-
 net/xfrm/xfrm_output.c |  1 +
 net/xfrm/xfrm_state.c  | 10 ++++++++--
 net/xfrm/xfrm_user.c   |  4 ++--
 5 files changed, 18 insertions(+), 5 deletions(-)

From: Wei Yongjun <[email protected]>

xfrm_output_one() does not return a error code when there is no dst_entry attached to the skb, it is still possible crash with a NULL pointer dereference in xfrm_output_resume(). Fix it by return error code -EHOSTUNREACH.

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

index 4ae87c5ce2e3..fef6b2da3c5d 100644
@@ -102,6 +102,7 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
 		skb_dst_force(skb);
 		if (!skb_dst(skb)) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
+			err = -EHOSTUNREACH;
 			goto error_nolock;
 		}
 

2.17.1

From: Benjamin Poirier <[email protected]>

sadhcnt is reported by ip -s xfrm state count as "buckets count", not the hash mask.

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

index b669262682c9..12cdb350c456 100644
@@ -788,7 +788,7 @@ void xfrm_sad_getinfo(struct net *net, struct xfrmk_sadinfo *si)
 {
 	spin_lock_bh(&net->xfrm.xfrm_state_lock);
 	si->sadcnt = net->xfrm.state_num;
-	si->sadhcnt = net->xfrm.state_hmask;
+	si->sadhcnt = net->xfrm.state_hmask + 1;
 	si->sadhmcnt = xfrm_state_hashmax;
 	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 }

2.17.1

Since commit 222d7dbd258d ("net: prevent dst uses after free") skb_dst_force() might clear the dst_entry attached to the skb. The xfrm code doesn't expect this to happen, so we crash with a NULL pointer dereference in this case.

Fix it by checking skb_dst(skb) for NULL after skb_dst_force() and drop the packet in case the dst_entry was cleared. We also move the skb_dst_force() to a codepath that is not used when the transformation was offloaded, because in this case we don't have a dst_entry attached to the skb.

The output and forwarding path was already fixed by commit 9e1437937807 ("xfrm: Fix NULL pointer dereference when skb_dst_force clears the dst_entry.")

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

index 684c0bc01e2c..d5635908587f 100644
@@ -346,6 +346,12 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 		skb->sp->xvec[skb->sp->len++] = x;
 
+		skb_dst_force(skb);
+		if (!skb_dst(skb)) {
+			XFRM_INC_STATS(net, LINUX_MIB_XFRMINERROR);
+			goto drop;
+		}
+
 lock:
 		spin_lock(&x->lock);
 

@@ -385,7 +391,6 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		XFRM_SKB_CB(skb)->seq.input.low = seq;
 		XFRM_SKB_CB(skb)->seq.input.hi = seq_hi;
 
-		skb_dst_force(skb);
 		dev_hold(skb->dev);
 
 		if (crypto_done)

2.17.1

From: Mathias Krause <[email protected]>

Commit 565f0fa902b6 ("xfrm: use a dedicated slab cache for struct xfrm_state") moved xfrm state objects to use their own slab cache. However, it missed to adapt xfrm_user to use this new cache when freeing xfrm states.

Fix this by introducing and make use of a new helper for freeing xfrm_state objects.

---
 include/net/xfrm.h    | 1 +
 net/xfrm/xfrm_state.c | 8 +++++++-
 net/xfrm/xfrm_user.c  | 4 ++--
 3 files changed, 10 insertions(+), 3 deletions(-)

index 0eb390c205af..da588def3c61 100644
@@ -1552,6 +1552,7 @@ int xfrm_state_walk(struct net *net, struct xfrm_state_walk *walk,
 		    int (*func)(struct xfrm_state *, int, void*), void *);
 void xfrm_state_walk_done(struct xfrm_state_walk *walk, struct net *net);
 struct xfrm_state *xfrm_state_alloc(struct net *net);
+void xfrm_state_free(struct xfrm_state *x);
 struct xfrm_state *xfrm_state_find(const xfrm_address_t *daddr,
 				   const xfrm_address_t *saddr,
 				   const struct flowi *fl,

index 12cdb350c456..cc0203efb584 100644
@@ -426,6 +426,12 @@ static void xfrm_put_mode(struct xfrm_mode *mode)
 	module_put(mode->owner);
 }
 
+void xfrm_state_free(struct xfrm_state *x)
+{
+	kmem_cache_free(xfrm_state_cache, x);
+}
+EXPORT_SYMBOL(xfrm_state_free);
+
 static void xfrm_state_gc_destroy(struct xfrm_state *x)
 {
 	tasklet_hrtimer_cancel(&x->mtimer);

@@ -452,7 +458,7 @@ static void xfrm_state_gc_destroy(struct xfrm_state *x)
 	}
 	xfrm_dev_state_free(x);
 	security_xfrm_state_free(x);
-	kmem_cache_free(xfrm_state_cache, x);
+	xfrm_state_free(x);
 }
 
 static void xfrm_state_gc_task(struct work_struct *work)

index ca7a207b81a9..683080172655 100644
@@ -2288,13 +2288,13 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	}
 
-	kfree(x);
+	xfrm_state_free(x);
 	kfree(xp);
 
 	return 0;
 
 free_state:
-	kfree(x);
+	xfrm_state_free(x);
 nomem:
 	return err;
 }

2.17.1

From: Steffen Klassert <[email protected]>
Date: Tue, 18 Dec 2018 11:37:31 +0100

Pulled, thank you!