[PATCH 0/1] can: fix a potential UAF access in j1939_session_deactivate()

[Xiaochen Zou <xzou017@ucr.edu>]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]


Xiaochen Zou (1):
  can: fix a potential UAF access in j1939_session_deactivate(). Both
    session and session->priv may be freed in
    j1939_session_deactivate_locked(). It leads to potential UAF read
    and write in j1939_session_list_unlock(). The free chain is

j1939_session_deactivate_locked()->j1939_session_put()->__j1939_session_release()->j1939_session_destroy().
    To fix this bug, I moved j1939_session_put() behind
    j1939_session_deactivate_locked(), and guarded it with a check of
    active since the session would be freed only if active is true.

 net/can/j1939/transport.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.17.1

From 9c4733d093e05db22eb89825579c83e020c3c1a6 Mon Sep 17 00:00:00 2001
From: Xiaochen Zou <xzou017@ucr.edu>
Date: Tue, 13 Jul 2021 13:15:59 -0700
Subject: [PATCH 1/1] can: fix a potential UAF access in
 j1939_session_deactivate().
To: greg@kroah.com
Cc: stable@vger.kernel.org,netdev@vger.kernel.org,linux-can@vger.kernel.org
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.17.1"

This is a multi-part message in MIME format.

[-- Attachment #2: Attached Message Part --]
[-- Type: text/plain, Size: 620 bytes --]

Both session and session->priv may be freed in
j1939_session_deactivate_locked(). It leads to potential UAF read and write
in j1939_session_list_unlock(). The free chain is
j1939_session_deactivate_locked()->j1939_session_put()->__j1939_session_release()->j1939_session_destroy().
To fix this bug, I moved j1939_session_put() behind
j1939_session_deactivate_locked(), and guarded it with a check of active
since the session would be freed only if active is true.
 
Signed-off-by: Xiaochen Zou <xzou017@ucr.edu>
---
 net/can/j1939/transport.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)



[-- Attachment #3: 0001-can-fix-a-potential-UAF-access-in-j1939_session_deac.patch --]
[-- Type: text/x-patch, Size: 1387 bytes --]

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index c3946c355882..c64bd5e8118a 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1067,7 +1067,6 @@ static bool j1939_session_deactivate_locked(struct j1939_session *session)
 
 		list_del_init(&session->active_session_list_entry);
 		session->state = J1939_SESSION_DONE;
-		j1939_session_put(session);
 	}
 
 	return active;
@@ -1080,6 +1079,8 @@ static bool j1939_session_deactivate(struct j1939_session *session)
 	j1939_session_list_lock(session->priv);
 	active = j1939_session_deactivate_locked(session);
 	j1939_session_list_unlock(session->priv);
+	if (active)
+		j1939_session_put(session);
 
 	return active;
 }
@@ -2127,6 +2128,7 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb)
 int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
 {
 	struct j1939_session *session, *saved;
+	bool active;
 
 	netdev_dbg(priv->ndev, "%s, sk: %p\n", __func__, sk);
 	j1939_session_list_lock(priv);
@@ -2140,7 +2142,9 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
 				j1939_session_put(session);
 
 			session->err = ESHUTDOWN;
-			j1939_session_deactivate_locked(session);
+			active = j1939_session_deactivate_locked(session);
+			if (active)
+				j1939_session_put(session);
 		}
 	}
 	j1939_session_list_unlock(priv);