netdev - [PATCH] xfrm: Fix xfrm_state refcnt leak in xfrm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <1587619161-14094-1-git-send-email-xiyuyang19@fudan.edu.cn>
Date:   Thu, 23 Apr 2020 13:19:20 +0800
From:   Xiyu Yang <xiyuyang19@...an.edu.cn>
To:     Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     yuanxzhang@...an.edu.cn, kjlu@....edu,
        Xiyu Yang <xiyuyang19@...an.edu.cn>,
        Xin Tan <tanxin.ctf@...il.com>
Subject: [PATCH] xfrm: Fix xfrm_state refcnt leak in xfrm_input()

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@...an.edu.cn>
Signed-off-by: Xin Tan <tanxin.ctf@...il.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