All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	stable@vger.kernel.org, Guillaume Nault <g.nault@alphalink.fr>,
	"David S. Miller" <davem@davemloft.net>,
	Will Deacon <will@kernel.org>
Subject: [PATCH 4.4 09/29] l2tp: fix duplicate session creation
Date: Sat, 11 Apr 2020 14:08:39 +0200	[thread overview]
Message-ID: <20200411115409.210027991@linuxfoundation.org> (raw)
In-Reply-To: <20200411115407.651296755@linuxfoundation.org>

From: Guillaume Nault <g.nault@alphalink.fr>

commit dbdbc73b44782e22b3b4b6e8b51e7a3d245f3086 upstream.

l2tp_session_create() relies on its caller for checking for duplicate
sessions. This is racy since a session can be concurrently inserted
after the caller's verification.

Fix this by letting l2tp_session_create() verify sessions uniqueness
upon insertion. Callers need to be adapted to check for
l2tp_session_create()'s return code instead of calling
l2tp_session_find().

pppol2tp_connect() is a bit special because it has to work on existing
sessions (if they're not connected) or to create a new session if none
is found. When acting on a preexisting session, a reference must be
held or it could go away on us. So we have to use l2tp_session_get()
instead of l2tp_session_find() and drop the reference before exiting.

Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Will Deacon <will@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 net/l2tp/l2tp_core.c |   70 ++++++++++++++++++++++++++++++++++++++-------------
 net/l2tp/l2tp_eth.c  |   10 +------
 net/l2tp/l2tp_ppp.c  |   60 +++++++++++++++++++++----------------------
 3 files changed, 84 insertions(+), 56 deletions(-)

--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -377,6 +377,48 @@ struct l2tp_session *l2tp_session_find_b
 }
 EXPORT_SYMBOL_GPL(l2tp_session_find_by_ifname);
 
+static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
+				      struct l2tp_session *session)
+{
+	struct l2tp_session *session_walk;
+	struct hlist_head *g_head;
+	struct hlist_head *head;
+	struct l2tp_net *pn;
+
+	head = l2tp_session_id_hash(tunnel, session->session_id);
+
+	write_lock_bh(&tunnel->hlist_lock);
+	hlist_for_each_entry(session_walk, head, hlist)
+		if (session_walk->session_id == session->session_id)
+			goto exist;
+
+	if (tunnel->version == L2TP_HDR_VER_3) {
+		pn = l2tp_pernet(tunnel->l2tp_net);
+		g_head = l2tp_session_id_hash_2(l2tp_pernet(tunnel->l2tp_net),
+						session->session_id);
+
+		spin_lock_bh(&pn->l2tp_session_hlist_lock);
+		hlist_for_each_entry(session_walk, g_head, global_hlist)
+			if (session_walk->session_id == session->session_id)
+				goto exist_glob;
+
+		hlist_add_head_rcu(&session->global_hlist, g_head);
+		spin_unlock_bh(&pn->l2tp_session_hlist_lock);
+	}
+
+	hlist_add_head(&session->hlist, head);
+	write_unlock_bh(&tunnel->hlist_lock);
+
+	return 0;
+
+exist_glob:
+	spin_unlock_bh(&pn->l2tp_session_hlist_lock);
+exist:
+	write_unlock_bh(&tunnel->hlist_lock);
+
+	return -EEXIST;
+}
+
 /* Lookup a tunnel by id
  */
 struct l2tp_tunnel *l2tp_tunnel_find(struct net *net, u32 tunnel_id)
@@ -1792,6 +1834,7 @@ EXPORT_SYMBOL_GPL(l2tp_session_set_heade
 struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunnel, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
 {
 	struct l2tp_session *session;
+	int err;
 
 	session = kzalloc(sizeof(struct l2tp_session) + priv_size, GFP_KERNEL);
 	if (session != NULL) {
@@ -1847,6 +1890,13 @@ struct l2tp_session *l2tp_session_create
 
 		l2tp_session_set_header_len(session, tunnel->version);
 
+		err = l2tp_session_add_to_tunnel(tunnel, session);
+		if (err) {
+			kfree(session);
+
+			return ERR_PTR(err);
+		}
+
 		/* Bump the reference count. The session context is deleted
 		 * only when this drops to zero.
 		 */
@@ -1856,28 +1906,14 @@ struct l2tp_session *l2tp_session_create
 		/* Ensure tunnel socket isn't deleted */
 		sock_hold(tunnel->sock);
 
-		/* Add session to the tunnel's hash list */
-		write_lock_bh(&tunnel->hlist_lock);
-		hlist_add_head(&session->hlist,
-			       l2tp_session_id_hash(tunnel, session_id));
-		write_unlock_bh(&tunnel->hlist_lock);
-
-		/* And to the global session list if L2TPv3 */
-		if (tunnel->version != L2TP_HDR_VER_2) {
-			struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
-
-			spin_lock_bh(&pn->l2tp_session_hlist_lock);
-			hlist_add_head_rcu(&session->global_hlist,
-					   l2tp_session_id_hash_2(pn, session_id));
-			spin_unlock_bh(&pn->l2tp_session_hlist_lock);
-		}
-
 		/* Ignore management session in session count value */
 		if (session->session_id != 0)
 			atomic_inc(&l2tp_session_count);
+
+		return session;
 	}
 
-	return session;
+	return ERR_PTR(-ENOMEM);
 }
 EXPORT_SYMBOL_GPL(l2tp_session_create);
 
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -223,12 +223,6 @@ static int l2tp_eth_create(struct net *n
 		goto out;
 	}
 
-	session = l2tp_session_find(net, tunnel, session_id);
-	if (session) {
-		rc = -EEXIST;
-		goto out;
-	}
-
 	if (cfg->ifname) {
 		dev = dev_get_by_name(net, cfg->ifname);
 		if (dev) {
@@ -242,8 +236,8 @@ static int l2tp_eth_create(struct net *n
 
 	session = l2tp_session_create(sizeof(*spriv), tunnel, session_id,
 				      peer_session_id, cfg);
-	if (!session) {
-		rc = -ENOMEM;
+	if (IS_ERR(session)) {
+		rc = PTR_ERR(session);
 		goto out;
 	}
 
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -600,6 +600,7 @@ static int pppol2tp_connect(struct socke
 	int error = 0;
 	u32 tunnel_id, peer_tunnel_id;
 	u32 session_id, peer_session_id;
+	bool drop_refcnt = false;
 	int ver = 2;
 	int fd;
 
@@ -708,36 +709,36 @@ static int pppol2tp_connect(struct socke
 	if (tunnel->peer_tunnel_id == 0)
 		tunnel->peer_tunnel_id = peer_tunnel_id;
 
-	/* Create session if it doesn't already exist. We handle the
-	 * case where a session was previously created by the netlink
-	 * interface by checking that the session doesn't already have
-	 * a socket and its tunnel socket are what we expect. If any
-	 * of those checks fail, return EEXIST to the caller.
-	 */
-	session = l2tp_session_find(sock_net(sk), tunnel, session_id);
-	if (session == NULL) {
-		/* Default MTU must allow space for UDP/L2TP/PPP
-		 * headers.
+	session = l2tp_session_get(sock_net(sk), tunnel, session_id, false);
+	if (session) {
+		drop_refcnt = true;
+		ps = l2tp_session_priv(session);
+
+		/* Using a pre-existing session is fine as long as it hasn't
+		 * been connected yet.
 		 */
-		cfg.mtu = cfg.mru = 1500 - PPPOL2TP_HEADER_OVERHEAD;
+		if (ps->sock) {
+			error = -EEXIST;
+			goto end;
+		}
 
-		/* Allocate and initialize a new session context. */
-		session = l2tp_session_create(sizeof(struct pppol2tp_session),
-					      tunnel, session_id,
-					      peer_session_id, &cfg);
-		if (session == NULL) {
-			error = -ENOMEM;
+		/* consistency checks */
+		if (ps->tunnel_sock != tunnel->sock) {
+			error = -EEXIST;
 			goto end;
 		}
 	} else {
-		ps = l2tp_session_priv(session);
-		error = -EEXIST;
-		if (ps->sock != NULL)
-			goto end;
+		/* Default MTU must allow space for UDP/L2TP/PPP headers */
+		cfg.mtu = 1500 - PPPOL2TP_HEADER_OVERHEAD;
+		cfg.mru = cfg.mtu;
 
-		/* consistency checks */
-		if (ps->tunnel_sock != tunnel->sock)
+		session = l2tp_session_create(sizeof(struct pppol2tp_session),
+					      tunnel, session_id,
+					      peer_session_id, &cfg);
+		if (IS_ERR(session)) {
+			error = PTR_ERR(session);
 			goto end;
+		}
 	}
 
 	/* Associate session with its PPPoL2TP socket */
@@ -802,6 +803,8 @@ out_no_ppp:
 		  session->name);
 
 end:
+	if (drop_refcnt)
+		l2tp_session_dec_refcount(session);
 	release_sock(sk);
 
 	return error;
@@ -829,12 +832,6 @@ static int pppol2tp_session_create(struc
 	if (tunnel->sock == NULL)
 		goto out;
 
-	/* Check that this session doesn't already exist */
-	error = -EEXIST;
-	session = l2tp_session_find(net, tunnel, session_id);
-	if (session != NULL)
-		goto out;
-
 	/* Default MTU values. */
 	if (cfg->mtu == 0)
 		cfg->mtu = 1500 - PPPOL2TP_HEADER_OVERHEAD;
@@ -842,12 +839,13 @@ static int pppol2tp_session_create(struc
 		cfg->mru = cfg->mtu;
 
 	/* Allocate and initialize a new session context. */
-	error = -ENOMEM;
 	session = l2tp_session_create(sizeof(struct pppol2tp_session),
 				      tunnel, session_id,
 				      peer_session_id, cfg);
-	if (session == NULL)
+	if (IS_ERR(session)) {
+		error = PTR_ERR(session);
 		goto out;
+	}
 
 	ps = l2tp_session_priv(session);
 	ps->tunnel_sock = tunnel->sock;



  parent reply	other threads:[~2020-04-11 12:11 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-11 12:08 [PATCH 4.4 00/29] 4.4.219-rc1 review Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 01/29] drm/bochs: downgrade pci_request_region failure from error to warning Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 02/29] ipv4: fix a RCU-list lock in fib_triestat_seq_show Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 03/29] net, ip_tunnel: fix interface lookup with no key Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 04/29] sctp: fix possibly using a bad saddr with a given dst Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 05/29] l2tp: Correctly return -EBADF from pppol2tp_getname Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 06/29] net: l2tp: Make l2tp_ip6 namespace aware Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 07/29] l2tp: fix race in l2tp_recv_common() Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 08/29] l2tp: ensure session cant get removed during pppol2tp_session_ioctl() Greg Kroah-Hartman
2020-04-11 12:08 ` Greg Kroah-Hartman [this message]
2020-04-11 12:08 ` [PATCH 4.4 10/29] l2tp: Refactor the codes with existing macros instead of literal number Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 11/29] l2tp: ensure sessions are freed after their PPPOL2TP socket Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 12/29] l2tp: fix race between l2tp_session_delete() and l2tp_tunnel_closeall() Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 13/29] usb: gadget: uac2: Drop unused device qualifier descriptor Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 14/29] usb: gadget: printer: " Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 15/29] padata: always acquire cpu_hotplug_lock before pinst->lock Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 16/29] mm: mempolicy: require at least one nodeid for MPOL_PREFERRED Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 17/29] net: stmmac: dwmac1000: fix out-of-bounds mac address reg setting Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 18/29] slcan: Dont transmit uninitialized stack data in padding Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 19/29] random: always use batched entropy for get_random_u{32,64} Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 20/29] tools/accounting/getdelays.c: fix netlink attribute length Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 21/29] power: supply: axp288_charger: Fix unchecked return value Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 22/29] xen-netfront: Fix mismatched rtnl_unlock Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 23/29] xen-netfront: Update features after registering netdev Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 24/29] ASoC: jz4740-i2s: Fix divider written at incorrect offset in register Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 25/29] IB/hfi1: Call kobject_put() when kobject_init_and_add() fails Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 26/29] Bluetooth: RFCOMM: fix ODEBUG bug in rfcomm_dev_ioctl Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 27/29] RDMA/cm: Update num_paths in cma_resolve_iboe_route error flow Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 28/29] clk: qcom: rcg: Return failure for RCG update Greg Kroah-Hartman
2020-04-11 12:08 ` [PATCH 4.4 29/29] drm_dp_mst_topology: fix broken drm_dp_sideband_parse_remote_dpcd_read() Greg Kroah-Hartman
2020-04-11 20:37 ` [PATCH 4.4 00/29] 4.4.219-rc1 review Guenter Roeck
2020-04-12 10:05 ` Naresh Kamboju
2020-04-13 19:44 ` Chris Paterson
2020-04-14 10:35 ` Jon Hunter
2020-04-14 10:35   ` Jon Hunter
     [not found]   ` <63b31c56-b5c9-2ced-ee00-772fa9a1dcaf-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2020-04-14 10:57     ` Greg Kroah-Hartman
2020-04-14 10:57       ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200411115409.210027991@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=davem@davemloft.net \
    --cc=g.nault@alphalink.fr \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.