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 <[email protected]>
Signed-off-by: Steffen Klassert <[email protected]>
---
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
