From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steffen Klassert Subject: Re: [BUG] xfrm: unable to handle kernel NULL pointer dereference Date: Thu, 22 Nov 2018 07:44:18 +0100 Message-ID: <20181122064418.GE3581@gauss3.secunet.de> References: <20181116184800.GY16768@wantstofly.org> <20181116191246.GN8742@gauss3.secunet.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: , Jean-Philippe Menil , , , , To: Lennert Buytenhek Return-path: Received: from a.mx.secunet.com ([62.96.220.36]:49290 "EHLO a.mx.secunet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726958AbeKVRWW (ORCPT ); Thu, 22 Nov 2018 12:22:22 -0500 Content-Disposition: inline In-Reply-To: <20181116191246.GN8742@gauss3.secunet.de> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Nov 16, 2018 at 08:12:46PM +0100, Steffen Klassert wrote: > On Fri, Nov 16, 2018 at 08:48:00PM +0200, Lennert Buytenhek wrote: > > On Sat, Nov 10, 2018 at 08:34:34PM +0100, Jean-Philippe Menil wrote: > > > > > we're seeing unexpected crashes from kernel 4.15 to 4.18.17, using > > > IPsec VTI interfaces, on several vpn hosts, since upgrade from 4.4. > > > > I looked into this with Jean-Philippe, and it appears to be crashing > > on a NULL pointer dereference in the inlined xfrm_policy_check() call > > in vti_rcv_cb(), and specifically on the skb_dst(skb) dereference in > > __xfrm_policy_check2(): > > > > return (!net->xfrm.policy_count[dir] && !skb->sp) || > > (skb_dst(skb)->flags & DST_NOPOLICY) || <===== > > __xfrm_policy_check(sk, ndir, skb, family); > > > > Commit 9e1437937807 ("xfrm: Fix NULL pointer dereference when > > skb_dst_force clears the dst_entry.") fixes a very similar problem on > > the output and forward paths, but our issue seems to be triggering on > > the input path. > > Yes, this is the same problem. skb_dst_force() does not > really force a refcount anymore, it might clear the dst > pointer instead (maybe this function should be renamed). I plan to apply this patch to the ipsec tree: [PATCH RFC] xfrm: Fix NULL pointer dereference in xfrm_input when skb_dst_force clears the dst_entry. Since commit 222d7dbd258d ("net: prevent dst uses after free") skb_dst_force() might clear the dst_entry attached to the skb. The xfrm code don'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 cast 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.") Fixes: 222d7dbd258d ("net: prevent dst uses after free") Reported-by: Jean-Philippe Menil Signed-off-by: Steffen Klassert --- net/xfrm/xfrm_input.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c index 684c0bc01e2c..d5635908587f 100644 --- a/net/xfrm/xfrm_input.c +++ b/net/xfrm/xfrm_input.c @@ -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