netdev - Re: [PATCH v4 net-next] ppp: Fix one deadlock issue of PPP when reentrant

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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 Aug 2016 14:35:25 +0200
From:   Guillaume Nault <g.nault@...halink.fr>
To:     fgao@...ai8.com
Cc:     paulus@...ba.org, philipp@...fish-solutions.com,
        linux-ppp@...r.kernel.org, netdev@...r.kernel.org,
        gfree.wind@...il.com
Subject: Re: [PATCH v4 net-next] ppp: Fix one deadlock issue of PPP when
 reentrant

On Mon, Aug 22, 2016 at 09:20:14AM +0800, fgao@...ai8.com wrote:
> From: Gao Feng <fgao@...ai8.com>
> 
> PPP channel holds one spinlock before send frame. But the skb may
> select the same PPP channel with wrong route policy. As a result,
> the skb reaches the same channel path. It tries to get the same
> spinlock which is held before. Bang, the deadlock comes out.
>
Thanks for following up on this case.
On my side, I've thought a bit more about it in the weekend and cooked
this patch.
It's experimental and requires cleanup and further testing, but it
should fix all issues I could think of (at least for PPP over L2TP).

The main idea is to use a per-cpu variable to detect recursion in
selected points of PPP and L2TP xmit path.

---
 drivers/net/ppp/ppp_generic.c | 49 ++++++++++++++++++++++++++++++++-----------
 net/l2tp/l2tp_core.c          | 28 +++++++++++++++++++++----
 2 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index f226db4..c33036bf 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1354,6 +1354,8 @@ static void ppp_setup(struct net_device *dev)
 	dev->netdev_ops = &ppp_netdev_ops;
 	SET_NETDEV_DEVTYPE(dev, &ppp_type);
 
+	dev->features |= NETIF_F_LLTX;
+
 	dev->hard_header_len = PPP_HDRLEN;
 	dev->mtu = PPP_MRU;
 	dev->addr_len = 0;
@@ -1367,12 +1369,7 @@ static void ppp_setup(struct net_device *dev)
  * Transmit-side routines.
  */
 
-/*
- * Called to do any work queued up on the transmit side
- * that can now be done.
- */
-static void
-ppp_xmit_process(struct ppp *ppp)
+static void __ppp_xmit_process(struct ppp *ppp)
 {
 	struct sk_buff *skb;
 
@@ -1392,6 +1389,23 @@ ppp_xmit_process(struct ppp *ppp)
 	ppp_xmit_unlock(ppp);
 }
 
+static DEFINE_PER_CPU(int, ppp_xmit_recursion);
+
+/* Called to do any work queued up on the transmit side that can now be done */
+static void ppp_xmit_process(struct ppp *ppp)
+{
+	if (unlikely(__this_cpu_read(ppp_xmit_recursion))) {
+		WARN(1, "recursion detected\n");
+		return;
+	}
+
+	__this_cpu_inc(ppp_xmit_recursion);
+	local_bh_disable();
+	__ppp_xmit_process(ppp);
+	local_bh_enable();
+	__this_cpu_dec(ppp_xmit_recursion);
+}
+
 static inline struct sk_buff *
 pad_compress_skb(struct ppp *ppp, struct sk_buff *skb)
 {
@@ -1847,11 +1861,7 @@ static int ppp_mp_explode(struct ppp *ppp, struct sk_buff *skb)
 }
 #endif /* CONFIG_PPP_MULTILINK */
 
-/*
- * Try to send data out on a channel.
- */
-static void
-ppp_channel_push(struct channel *pch)
+static void __ppp_channel_push(struct channel *pch)
 {
 	struct sk_buff *skb;
 	struct ppp *ppp;
@@ -1876,11 +1886,26 @@ ppp_channel_push(struct channel *pch)
 		read_lock_bh(&pch->upl);
 		ppp = pch->ppp;
 		if (ppp)
-			ppp_xmit_process(ppp);
+			__ppp_xmit_process(ppp);
 		read_unlock_bh(&pch->upl);
 	}
 }
 
+/* Try to send data out on a channel */
+static void ppp_channel_push(struct channel *pch)
+{
+	if (unlikely(__this_cpu_read(ppp_xmit_recursion))) {
+		WARN(1, "recursion detected\n");
+		return;
+	}
+
+	__this_cpu_inc(ppp_xmit_recursion);
+	local_bh_disable();
+	__ppp_channel_push(pch);
+	local_bh_enable();
+	__this_cpu_dec(ppp_xmit_recursion);
+}
+
 /*
  * Receive-side routines.
  */
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 1e40dac..bdfb1be 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1096,10 +1096,8 @@ static int l2tp_xmit_core(struct l2tp_session *session, struct sk_buff *skb,
 	return 0;
 }
 
-/* If caller requires the skb to have a ppp header, the header must be
- * inserted in the skb data before calling this function.
- */
-int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len)
+static int __l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb,
+			   int hdr_len)
 {
 	int data_len = skb->len;
 	struct l2tp_tunnel *tunnel = session->tunnel;
@@ -1178,6 +1176,28 @@ out_unlock:
 
 	return ret;
 }
+
+static DEFINE_PER_CPU(int, l2tp_xmit_recursion);
+
+/* If caller requires the skb to have a ppp header, the header must be
+ * inserted in the skb data before calling this function.
+ */
+int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb,
+		  int hdr_len)
+{
+	int ret;
+
+	if (unlikely(__this_cpu_read(l2tp_xmit_recursion))) {
+		WARN(1, "recursion detected\n");
+		return NET_XMIT_DROP;
+	}
+
+	__this_cpu_inc(l2tp_xmit_recursion);
+	ret = __l2tp_xmit_skb(session, skb, hdr_len);
+	__this_cpu_dec(l2tp_xmit_recursion);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(l2tp_xmit_skb);
 
 /*****************************************************************************
-- 
2.9.3