[PATCH] xfrm: Fix xfrm_state refcnt leak in xfrm_input()

[PATCH] xfrm: Fix xfrm_state refcnt leak in xfrm_input()

[Xiyu Yang]

xfrm_input() invokes xfrm_state_lookup(), which returns a reference of
the specified xfrm_state object to "x" with increased refcnt and then
"x" is escaped to "sp->xvec[]".

When xfrm_input() encounters error, it calls kfree_skb() to free the
"skb" memory. Since "sp" comes from one of "skb" fields, this "free"
behavior causes "sp" becomes invalid, so the refcount for its field
should be decreased to keep refcount balanced before kfree_skb() calls.

The reference counting issue happens in several exception handling paths
of xfrm_input(). When those error scenarios occur such as skb_dst()
fails, the function forgets to decrease the refcnt increased by
xfrm_state_lookup() and directly calls kfree_skb(), causing a refcnt
leak.

Fix this issue by jumping to "put_sp" label when those error scenarios
occur.

Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
---
 net/xfrm/xfrm_input.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index aa35f23c4912..9ca534e28462 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -589,7 +589,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		skb_dst_force(skb);
 		if (!skb_dst(skb)) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINERROR);
-			goto drop;
+			goto put_sp;
 		}
 
 lock:
@@ -623,7 +623,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 		if (xfrm_tunnel_check(skb, x, family)) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMODEERROR);
-			goto drop;
+			goto put_sp;
 		}
 
 		seq_hi = htonl(xfrm_replay_seqhi(x, seq));
@@ -677,13 +677,13 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
 			if (inner_mode == NULL) {
 				XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMODEERROR);
-				goto drop;
+				goto put_sp;
 			}
 		}
 
 		if (xfrm_inner_mode_input(x, inner_mode, skb)) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMODEERROR);
-			goto drop;
+			goto put_sp;
 		}
 
 		if (x->outer_mode.flags & XFRM_MODE_FLAG_TUNNEL) {
@@ -701,7 +701,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		err = xfrm_parse_spi(skb, nexthdr, &spi, &seq);
 		if (err < 0) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINHDRERROR);
-			goto drop;
+			goto put_sp;
 		}
 		crypto_done = false;
 	} while (!err);
@@ -744,6 +744,8 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 drop_unlock:
 	spin_unlock(&x->lock);
+put_sp:
+	skb_ext_put_sp(sp);
 drop:
 	xfrm_rcv_cb(skb, family, x && x->type ? x->type->proto : nexthdr, -1);
 	kfree_skb(skb);
-- 
2.7.4

Re: [PATCH] xfrm: Fix xfrm_state refcnt leak in xfrm_input()

[Steffen Klassert]

On Thu, Apr 23, 2020 at 01:19:20PM +0800, Xiyu Yang wrote:
> xfrm_input() invokes xfrm_state_lookup(), which returns a reference of
> the specified xfrm_state object to "x" with increased refcnt and then
> "x" is escaped to "sp->xvec[]".
> 
> When xfrm_input() encounters error, it calls kfree_skb() to free the
> "skb" memory. Since "sp" comes from one of "skb" fields, this "free"
> behavior causes "sp" becomes invalid, so the refcount for its field
> should be decreased to keep refcount balanced before kfree_skb() calls.
> 
> The reference counting issue happens in several exception handling paths
> of xfrm_input(). When those error scenarios occur such as skb_dst()
> fails, the function forgets to decrease the refcnt increased by
> xfrm_state_lookup() and directly calls kfree_skb(), causing a refcnt
> leak.

kfree_skb() drops these refcounts already, why should we do that here
too?