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 <jpme...@gmail.com> Signed-off-by: Steffen Klassert <steffen.klass...@secunet.com> --- 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