All of lore.kernel.org
 help / color / mirror / Atom feed
* pull-request: can 2019-11-13
@ 2019-11-13  9:55 Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 1/9] can: af_can: export can_sock_destruct() Marc Kleine-Budde
                   ` (9 more replies)
  0 siblings, 10 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-can, kernel

Hello David,

this is a pull request of 9 patches for net/master, hopefully for the v5.4
release cycle.

All nine patches are by Oleksij Rempel and fix locking and use-after-free bugs
in the j1939 stack found by the syzkaller syzbot.

regards,
Marc

---

The following changes since commit 5aa4277d4368c099223bbcd3a9086f3351a12ce9:

  dpaa2-eth: free already allocated channels on probe defer (2019-11-12 19:49:27 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-5.4-20191113

for you to fetch changes up to 4a15d574e68afffbe8d7265e015cda2ac2a248ec:

  can: j1939: warn if resources are still linked on destroy (2019-11-13 10:42:34 +0100)

----------------------------------------------------------------
linux-can-fixes-for-5.4-20191113

----------------------------------------------------------------
Oleksij Rempel (9):
      can: af_can: export can_sock_destruct()
      can: j1939: move j1939_priv_put() into sk_destruct callback
      can: j1939: main: j1939_ndev_to_priv(): avoid crash if can_ml_priv is NULL
      can: j1939: socket: rework socket locking for j1939_sk_release() and j1939_sk_sendmsg()
      can: j1939: transport: make sure the aborted session will be deactivated only once
      can: j1939: make sure socket is held as long as session exists
      can: j1939: transport: j1939_cancel_active_session(): use hrtimer_try_to_cancel() instead of hrtimer_cancel()
      can: j1939: j1939_can_recv(): add priv refcounting
      can: j1939: warn if resources are still linked on destroy

 include/linux/can/core.h  |  1 +
 net/can/af_can.c          |  3 +-
 net/can/j1939/main.c      |  9 +++++
 net/can/j1939/socket.c    | 94 +++++++++++++++++++++++++++++++++++++----------
 net/can/j1939/transport.c | 36 +++++++++++++-----
 5 files changed, 113 insertions(+), 30 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/9] can: af_can: export can_sock_destruct()
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
@ 2019-11-13  9:55 ` Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 2/9] can: j1939: move j1939_priv_put() into sk_destruct callback Marc Kleine-Budde
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-can, kernel, Oleksij Rempel

From: Oleksij Rempel <o.rempel@pengutronix.de>

In j1939 we need our own struct sock::sk_destruct callback. Export the
generic af_can can_sock_destruct() that allows us to chain-call it.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/linux/can/core.h | 1 +
 net/can/af_can.c         | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 8339071ab08b..e20a0cd09ba5 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -65,5 +65,6 @@ extern void can_rx_unregister(struct net *net, struct net_device *dev,
 			      void *data);
 
 extern int can_send(struct sk_buff *skb, int loop);
+void can_sock_destruct(struct sock *sk);
 
 #endif /* !_CAN_CORE_H */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 5518a7d9eed9..128d37a4c2e0 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -86,11 +86,12 @@ static atomic_t skbcounter = ATOMIC_INIT(0);
 
 /* af_can socket functions */
 
-static void can_sock_destruct(struct sock *sk)
+void can_sock_destruct(struct sock *sk)
 {
 	skb_queue_purge(&sk->sk_receive_queue);
 	skb_queue_purge(&sk->sk_error_queue);
 }
+EXPORT_SYMBOL(can_sock_destruct);
 
 static const struct can_proto *can_get_proto(int protocol)
 {
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/9] can: j1939: move j1939_priv_put() into sk_destruct callback
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 1/9] can: af_can: export can_sock_destruct() Marc Kleine-Budde
@ 2019-11-13  9:55 ` Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 3/9] can: j1939: main: j1939_ndev_to_priv(): avoid crash if can_ml_priv is NULL Marc Kleine-Budde
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-can, kernel, Oleksij Rempel

From: Oleksij Rempel <o.rempel@pengutronix.de>

This patch delays the j1939_priv_put() until the socket is destroyed via
the sk_destruct callback, to avoid use-after-free problems.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/can/j1939/socket.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 4d8ba701e15d..aee94b09ef08 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -78,7 +78,6 @@ static void j1939_jsk_add(struct j1939_priv *priv, struct j1939_sock *jsk)
 {
 	jsk->state |= J1939_SOCK_BOUND;
 	j1939_priv_get(priv);
-	jsk->priv = priv;
 
 	spin_lock_bh(&priv->j1939_socks_lock);
 	list_add_tail(&jsk->list, &priv->j1939_socks);
@@ -91,7 +90,6 @@ static void j1939_jsk_del(struct j1939_priv *priv, struct j1939_sock *jsk)
 	list_del_init(&jsk->list);
 	spin_unlock_bh(&priv->j1939_socks_lock);
 
-	jsk->priv = NULL;
 	j1939_priv_put(priv);
 	jsk->state &= ~J1939_SOCK_BOUND;
 }
@@ -349,6 +347,34 @@ void j1939_sk_recv(struct j1939_priv *priv, struct sk_buff *skb)
 	spin_unlock_bh(&priv->j1939_socks_lock);
 }
 
+static void j1939_sk_sock_destruct(struct sock *sk)
+{
+	struct j1939_sock *jsk = j1939_sk(sk);
+
+	/* This function will be call by the generic networking code, when then
+	 * the socket is ultimately closed (sk->sk_destruct).
+	 *
+	 * The race between
+	 * - processing a received CAN frame
+	 *   (can_receive -> j1939_can_recv)
+	 *   and accessing j1939_priv
+	 * ... and ...
+	 * - closing a socket
+	 *   (j1939_can_rx_unregister -> can_rx_unregister)
+	 *   and calling the final j1939_priv_put()
+	 *
+	 * is avoided by calling the final j1939_priv_put() from this
+	 * RCU deferred cleanup call.
+	 */
+	if (jsk->priv) {
+		j1939_priv_put(jsk->priv);
+		jsk->priv = NULL;
+	}
+
+	/* call generic CAN sock destruct */
+	can_sock_destruct(sk);
+}
+
 static int j1939_sk_init(struct sock *sk)
 {
 	struct j1939_sock *jsk = j1939_sk(sk);
@@ -371,6 +397,7 @@ static int j1939_sk_init(struct sock *sk)
 	atomic_set(&jsk->skb_pending, 0);
 	spin_lock_init(&jsk->sk_session_queue_lock);
 	INIT_LIST_HEAD(&jsk->sk_session_queue);
+	sk->sk_destruct = j1939_sk_sock_destruct;
 
 	return 0;
 }
@@ -443,6 +470,12 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 		}
 
 		jsk->ifindex = addr->can_ifindex;
+
+		/* the corresponding j1939_priv_put() is called via
+		 * sk->sk_destruct, which points to j1939_sk_sock_destruct()
+		 */
+		j1939_priv_get(priv);
+		jsk->priv = priv;
 	}
 
 	/* set default transmit pgn */
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/9] can: j1939: main: j1939_ndev_to_priv(): avoid crash if can_ml_priv is NULL
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 1/9] can: af_can: export can_sock_destruct() Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 2/9] can: j1939: move j1939_priv_put() into sk_destruct callback Marc Kleine-Budde
@ 2019-11-13  9:55 ` Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 4/9] can: j1939: socket: rework socket locking for j1939_sk_release() and j1939_sk_sendmsg() Marc Kleine-Budde
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev
  Cc: davem, linux-can, kernel, Oleksij Rempel, syzbot+95c8e0d9dffde15b6c5c

From: Oleksij Rempel <o.rempel@pengutronix.de>

This patch avoids a NULL pointer deref crash if ndev->ml_priv is NULL.

Reported-by: syzbot+95c8e0d9dffde15b6c5c@syzkaller.appspotmail.com
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/can/j1939/main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index def2f813ffce..8dc935dc2e54 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -207,6 +207,9 @@ static inline struct j1939_priv *j1939_ndev_to_priv(struct net_device *ndev)
 {
 	struct can_ml_priv *can_ml_priv = ndev->ml_priv;
 
+	if (!can_ml_priv)
+		return NULL;
+
 	return can_ml_priv->j1939_priv;
 }
 
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/9] can: j1939: socket: rework socket locking for j1939_sk_release() and j1939_sk_sendmsg()
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
                   ` (2 preceding siblings ...)
  2019-11-13  9:55 ` [PATCH 3/9] can: j1939: main: j1939_ndev_to_priv(): avoid crash if can_ml_priv is NULL Marc Kleine-Budde
@ 2019-11-13  9:55 ` Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 5/9] can: j1939: transport: make sure the aborted session will be deactivated only once Marc Kleine-Budde
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev
  Cc: davem, linux-can, kernel, Oleksij Rempel,
	syzbot+afd421337a736d6c1ee6, syzbot+6d04f6a1b31a0ae12ca9

From: Oleksij Rempel <o.rempel@pengutronix.de>

j1939_sk_sendmsg() should be protected by lock_sock() to avoid race with
j1939_sk_bind() and j1939_sk_release().

Reported-by: syzbot+afd421337a736d6c1ee6@syzkaller.appspotmail.com
Reported-by: syzbot+6d04f6a1b31a0ae12ca9@syzkaller.appspotmail.com
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/can/j1939/socket.c | 57 +++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 18 deletions(-)

diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index aee94b09ef08..de09b0a65791 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -593,8 +593,8 @@ static int j1939_sk_release(struct socket *sock)
 	if (!sk)
 		return 0;
 
-	jsk = j1939_sk(sk);
 	lock_sock(sk);
+	jsk = j1939_sk(sk);
 
 	if (jsk->state & J1939_SOCK_BOUND) {
 		struct j1939_priv *priv = jsk->priv;
@@ -1092,51 +1092,72 @@ static int j1939_sk_sendmsg(struct socket *sock, struct msghdr *msg,
 {
 	struct sock *sk = sock->sk;
 	struct j1939_sock *jsk = j1939_sk(sk);
-	struct j1939_priv *priv = jsk->priv;
+	struct j1939_priv *priv;
 	int ifindex;
 	int ret;
 
+	lock_sock(sock->sk);
 	/* various socket state tests */
-	if (!(jsk->state & J1939_SOCK_BOUND))
-		return -EBADFD;
+	if (!(jsk->state & J1939_SOCK_BOUND)) {
+		ret = -EBADFD;
+		goto sendmsg_done;
+	}
 
+	priv = jsk->priv;
 	ifindex = jsk->ifindex;
 
-	if (!jsk->addr.src_name && jsk->addr.sa == J1939_NO_ADDR)
+	if (!jsk->addr.src_name && jsk->addr.sa == J1939_NO_ADDR) {
 		/* no source address assigned yet */
-		return -EBADFD;
+		ret = -EBADFD;
+		goto sendmsg_done;
+	}
 
 	/* deal with provided destination address info */
 	if (msg->msg_name) {
 		struct sockaddr_can *addr = msg->msg_name;
 
-		if (msg->msg_namelen < J1939_MIN_NAMELEN)
-			return -EINVAL;
+		if (msg->msg_namelen < J1939_MIN_NAMELEN) {
+			ret = -EINVAL;
+			goto sendmsg_done;
+		}
 
-		if (addr->can_family != AF_CAN)
-			return -EINVAL;
+		if (addr->can_family != AF_CAN) {
+			ret = -EINVAL;
+			goto sendmsg_done;
+		}
 
-		if (addr->can_ifindex && addr->can_ifindex != ifindex)
-			return -EBADFD;
+		if (addr->can_ifindex && addr->can_ifindex != ifindex) {
+			ret = -EBADFD;
+			goto sendmsg_done;
+		}
 
 		if (j1939_pgn_is_valid(addr->can_addr.j1939.pgn) &&
-		    !j1939_pgn_is_clean_pdu(addr->can_addr.j1939.pgn))
-			return -EINVAL;
+		    !j1939_pgn_is_clean_pdu(addr->can_addr.j1939.pgn)) {
+			ret = -EINVAL;
+			goto sendmsg_done;
+		}
 
 		if (!addr->can_addr.j1939.name &&
 		    addr->can_addr.j1939.addr == J1939_NO_ADDR &&
-		    !sock_flag(sk, SOCK_BROADCAST))
+		    !sock_flag(sk, SOCK_BROADCAST)) {
 			/* broadcast, but SO_BROADCAST not set */
-			return -EACCES;
+			ret = -EACCES;
+			goto sendmsg_done;
+		}
 	} else {
 		if (!jsk->addr.dst_name && jsk->addr.da == J1939_NO_ADDR &&
-		    !sock_flag(sk, SOCK_BROADCAST))
+		    !sock_flag(sk, SOCK_BROADCAST)) {
 			/* broadcast, but SO_BROADCAST not set */
-			return -EACCES;
+			ret = -EACCES;
+			goto sendmsg_done;
+		}
 	}
 
 	ret = j1939_sk_send_loop(priv, sk, msg, size);
 
+sendmsg_done:
+	release_sock(sock->sk);
+
 	return ret;
 }
 
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/9] can: j1939: transport: make sure the aborted session will be deactivated only once
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
                   ` (3 preceding siblings ...)
  2019-11-13  9:55 ` [PATCH 4/9] can: j1939: socket: rework socket locking for j1939_sk_release() and j1939_sk_sendmsg() Marc Kleine-Budde
@ 2019-11-13  9:55 ` Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 6/9] can: j1939: make sure socket is held as long as session exists Marc Kleine-Budde
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-can, kernel, Oleksij Rempel

From: Oleksij Rempel <o.rempel@pengutronix.de>

j1939_session_cancel() was modifying session->state without protecting
it by locks and without checking actual state of the session.

This patch moves j1939_tp_set_rxtimeout() into j1939_session_cancel()
and adds the missing locking.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/can/j1939/transport.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index e5f1a56994c6..ecdedfc0b10c 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1042,12 +1042,13 @@ j1939_session_deactivate_activate_next(struct j1939_session *session)
 		j1939_sk_queue_activate_next(session);
 }
 
-static void j1939_session_cancel(struct j1939_session *session,
+static void __j1939_session_cancel(struct j1939_session *session,
 				 enum j1939_xtp_abort err)
 {
 	struct j1939_priv *priv = session->priv;
 
 	WARN_ON_ONCE(!err);
+	lockdep_assert_held(&session->priv->active_session_list_lock);
 
 	session->err = j1939_xtp_abort_to_errno(priv, err);
 	/* do not send aborts on incoming broadcasts */
@@ -1062,6 +1063,20 @@ static void j1939_session_cancel(struct j1939_session *session,
 		j1939_sk_send_loop_abort(session->sk, session->err);
 }
 
+static void j1939_session_cancel(struct j1939_session *session,
+				 enum j1939_xtp_abort err)
+{
+	j1939_session_list_lock(session->priv);
+
+	if (session->state >= J1939_SESSION_ACTIVE &&
+	    session->state < J1939_SESSION_WAITING_ABORT) {
+		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
+		__j1939_session_cancel(session, err);
+	}
+
+	j1939_session_list_unlock(session->priv);
+}
+
 static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
 {
 	struct j1939_session *session =
@@ -1108,8 +1123,6 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
 		netdev_alert(priv->ndev, "%s: 0x%p: tx aborted with unknown reason: %i\n",
 			     __func__, session, ret);
 		if (session->skcb.addr.type != J1939_SIMPLE) {
-			j1939_tp_set_rxtimeout(session,
-					       J1939_XTP_ABORT_TIMEOUT_MS);
 			j1939_session_cancel(session, J1939_XTP_ABORT_OTHER);
 		} else {
 			session->err = ret;
@@ -1169,7 +1182,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
 			hrtimer_start(&session->rxtimer,
 				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
 				      HRTIMER_MODE_REL_SOFT);
-			j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
+			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
 		}
 		j1939_session_list_unlock(session->priv);
 	}
@@ -1375,7 +1388,6 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
 
  out_session_cancel:
 	j1939_session_timers_cancel(session);
-	j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
 	j1939_session_cancel(session, err);
 }
 
@@ -1572,7 +1584,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
 
 		/* RTS on active session */
 		j1939_session_timers_cancel(session);
-		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
 		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
 	}
 
@@ -1583,7 +1594,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
 			     session->last_cmd);
 
 		j1939_session_timers_cancel(session);
-		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
 		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
 
 		return -EBUSY;
@@ -1785,7 +1795,6 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
 
  out_session_cancel:
 	j1939_session_timers_cancel(session);
-	j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
 	j1939_session_cancel(session, J1939_XTP_ABORT_FAULT);
 	j1939_session_put(session);
 }
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 6/9] can: j1939: make sure socket is held as long as session exists
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
                   ` (4 preceding siblings ...)
  2019-11-13  9:55 ` [PATCH 5/9] can: j1939: transport: make sure the aborted session will be deactivated only once Marc Kleine-Budde
@ 2019-11-13  9:55 ` Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 7/9] can: j1939: transport: j1939_cancel_active_session(): use hrtimer_try_to_cancel() instead of hrtimer_cancel() Marc Kleine-Budde
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-can, kernel, Oleksij Rempel

From: Oleksij Rempel <o.rempel@pengutronix.de>

We link the socket to the session to be able provide socket specific
notifications. For example messages over error queue.

We need to keep the socket held, while we have a reference to it.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/can/j1939/transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index ecdedfc0b10c..afc2adfd97e4 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -255,6 +255,7 @@ static void __j1939_session_drop(struct j1939_session *session)
 		return;
 
 	j1939_sock_pending_del(session->sk);
+	sock_put(session->sk);
 }
 
 static void j1939_session_destroy(struct j1939_session *session)
@@ -1875,6 +1876,7 @@ struct j1939_session *j1939_tp_send(struct j1939_priv *priv,
 		return ERR_PTR(-ENOMEM);
 
 	/* skb is recounted in j1939_session_new() */
+	sock_hold(skb->sk);
 	session->sk = skb->sk;
 	session->transmission = true;
 	session->pkt.total = (size + 6) / 7;
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 7/9] can: j1939: transport: j1939_cancel_active_session(): use hrtimer_try_to_cancel() instead of hrtimer_cancel()
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
                   ` (5 preceding siblings ...)
  2019-11-13  9:55 ` [PATCH 6/9] can: j1939: make sure socket is held as long as session exists Marc Kleine-Budde
@ 2019-11-13  9:55 ` Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 8/9] can: j1939: j1939_can_recv(): add priv refcounting Marc Kleine-Budde
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-can, kernel, Oleksij Rempel

From: Oleksij Rempel <o.rempel@pengutronix.de>

This part of the code protected by lock used in the hrtimer as well.
Using hrtimer_cancel() will trigger dead lock.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/can/j1939/transport.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index afc2adfd97e4..0c62b8fc4b20 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -2039,7 +2039,11 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
 				 &priv->active_session_list,
 				 active_session_list_entry) {
 		if (!sk || sk == session->sk) {
-			j1939_session_timers_cancel(session);
+			if (hrtimer_try_to_cancel(&session->txtimer) == 1)
+				j1939_session_put(session);
+			if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
+				j1939_session_put(session);
+
 			session->err = ESHUTDOWN;
 			j1939_session_deactivate_locked(session);
 		}
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 8/9] can: j1939: j1939_can_recv(): add priv refcounting
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
                   ` (6 preceding siblings ...)
  2019-11-13  9:55 ` [PATCH 7/9] can: j1939: transport: j1939_cancel_active_session(): use hrtimer_try_to_cancel() instead of hrtimer_cancel() Marc Kleine-Budde
@ 2019-11-13  9:55 ` Marc Kleine-Budde
  2019-11-13  9:55 ` [PATCH 9/9] can: j1939: warn if resources are still linked on destroy Marc Kleine-Budde
  2019-11-13 19:47 ` pull-request: can 2019-11-13 David Miller
  9 siblings, 0 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev
  Cc: davem, linux-can, kernel, Oleksij Rempel,
	syzbot+ca172a0ac477ac90f045, syzbot+07ca5bce8530070a5650,
	syzbot+a47537d3964ef6c874e1

From: Oleksij Rempel <o.rempel@pengutronix.de>

j1939_can_recv() can be called in parallel with socket release. In this
case sk_release and sk_destruct can be done earlier than
j1939_can_recv() is processed.

Reported-by: syzbot+ca172a0ac477ac90f045@syzkaller.appspotmail.com
Reported-by: syzbot+07ca5bce8530070a5650@syzkaller.appspotmail.com
Reported-by: syzbot+a47537d3964ef6c874e1@syzkaller.appspotmail.com
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/can/j1939/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 8dc935dc2e54..2afcf27c72c8 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -51,6 +51,7 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
 	if (!skb)
 		return;
 
+	j1939_priv_get(priv);
 	can_skb_set_owner(skb, iskb->sk);
 
 	/* get a pointer to the header of the skb
@@ -104,6 +105,7 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
 	j1939_simple_recv(priv, skb);
 	j1939_sk_recv(priv, skb);
  done:
+	j1939_priv_put(priv);
 	kfree_skb(skb);
 }
 
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 9/9] can: j1939: warn if resources are still linked on destroy
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
                   ` (7 preceding siblings ...)
  2019-11-13  9:55 ` [PATCH 8/9] can: j1939: j1939_can_recv(): add priv refcounting Marc Kleine-Budde
@ 2019-11-13  9:55 ` Marc Kleine-Budde
  2019-11-13 19:47 ` pull-request: can 2019-11-13 David Miller
  9 siblings, 0 replies; 11+ messages in thread
From: Marc Kleine-Budde @ 2019-11-13  9:55 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-can, kernel, Oleksij Rempel

From: Oleksij Rempel <o.rempel@pengutronix.de>

j1939_session_destroy() and __j1939_priv_release() should be called only
if session, ecu or socket are not linked or used by any one else. If at
least one of these resources is linked, then the reference counting is
broken somewhere.

This warning will be triggered before KASAN will do, and will make it
easier to debug initial issue. This works on platforms without KASAN
support.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/can/j1939/main.c      | 4 ++++
 net/can/j1939/transport.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 2afcf27c72c8..137054bff9ec 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -152,6 +152,10 @@ static void __j1939_priv_release(struct kref *kref)
 
 	netdev_dbg(priv->ndev, "%s: 0x%p\n", __func__, priv);
 
+	WARN_ON_ONCE(!list_empty(&priv->active_session_list));
+	WARN_ON_ONCE(!list_empty(&priv->ecus));
+	WARN_ON_ONCE(!list_empty(&priv->j1939_socks));
+
 	dev_put(ndev);
 	kfree(priv);
 }
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 0c62b8fc4b20..9f99af5b0b11 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -267,6 +267,9 @@ static void j1939_session_destroy(struct j1939_session *session)
 
 	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
 
+	WARN_ON_ONCE(!list_empty(&session->sk_session_queue_entry));
+	WARN_ON_ONCE(!list_empty(&session->active_session_list_entry));
+
 	skb_queue_purge(&session->skb_queue);
 	__j1939_session_drop(session);
 	j1939_priv_put(session->priv);
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: pull-request: can 2019-11-13
  2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
                   ` (8 preceding siblings ...)
  2019-11-13  9:55 ` [PATCH 9/9] can: j1939: warn if resources are still linked on destroy Marc Kleine-Budde
@ 2019-11-13 19:47 ` David Miller
  9 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2019-11-13 19:47 UTC (permalink / raw)
  To: mkl; +Cc: netdev, linux-can, kernel

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Wed, 13 Nov 2019 10:55:41 +0100

> this is a pull request of 9 patches for net/master, hopefully for the v5.4
> release cycle.
> 
> All nine patches are by Oleksij Rempel and fix locking and use-after-free bugs
> in the j1939 stack found by the syzkaller syzbot.

Pulled, thanks Marc.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-11-13 19:47 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-13  9:55 pull-request: can 2019-11-13 Marc Kleine-Budde
2019-11-13  9:55 ` [PATCH 1/9] can: af_can: export can_sock_destruct() Marc Kleine-Budde
2019-11-13  9:55 ` [PATCH 2/9] can: j1939: move j1939_priv_put() into sk_destruct callback Marc Kleine-Budde
2019-11-13  9:55 ` [PATCH 3/9] can: j1939: main: j1939_ndev_to_priv(): avoid crash if can_ml_priv is NULL Marc Kleine-Budde
2019-11-13  9:55 ` [PATCH 4/9] can: j1939: socket: rework socket locking for j1939_sk_release() and j1939_sk_sendmsg() Marc Kleine-Budde
2019-11-13  9:55 ` [PATCH 5/9] can: j1939: transport: make sure the aborted session will be deactivated only once Marc Kleine-Budde
2019-11-13  9:55 ` [PATCH 6/9] can: j1939: make sure socket is held as long as session exists Marc Kleine-Budde
2019-11-13  9:55 ` [PATCH 7/9] can: j1939: transport: j1939_cancel_active_session(): use hrtimer_try_to_cancel() instead of hrtimer_cancel() Marc Kleine-Budde
2019-11-13  9:55 ` [PATCH 8/9] can: j1939: j1939_can_recv(): add priv refcounting Marc Kleine-Budde
2019-11-13  9:55 ` [PATCH 9/9] can: j1939: warn if resources are still linked on destroy Marc Kleine-Budde
2019-11-13 19:47 ` pull-request: can 2019-11-13 David Miller

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.