j1939_session_destroy() will free both session and session->priv. It leads to multiple use-after-free read and write in j1939_session_deactivate() when session was freed in j1939_session_deactivate_locked(). 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.
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c index e5f1a56994c6..b6448f29a4bd 100644 --- a/net/can/j1939/transport.c +++ b/net/can/j1939/transport.c @@ -1018,7 +1018,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; @@ -1031,6 +1030,9 @@ 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; } @@ -2021,6 +2023,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); @@ -2030,7 +2033,10 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk) if (!sk || sk == session->sk) { j1939_session_timers_cancel(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);
On Mon, Jul 12, 2021 at 9:44 PM Greg KH greg@kroah.com wrote:
On Mon, Jul 12, 2021 at 03:40:46PM -0700, Xiaochen Zou wrote:
Hi, It looks like there are multiple use-after-free accesses in j1939_session_deactivate()
static bool j1939_session_deactivate(struct j1939_session *session) { bool active;
j1939_session_list_lock(session->priv); active = j1939_session_deactivate_locked(session); //session can be freed inside j1939_session_list_unlock(session->priv); // It causes UAF read and write
return active; }
session can be freed by j1939_session_deactivate_locked->j1939_session_put->__j1939_session_release->j1939_session_destroy->kfree. Therefore it makes the unlock function perform UAF access.
Great, can you make up a patch to fix this issue so you can get credit for finding and solving it?
thanks,
greg k-h