From: Xiaochen Zou <xzou017@xxxxxxx> 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, move the j1939_session_put() behind j1939_session_deactivate_locked(), and guard it with a check of active since the session would be freed only if active is true. Link: https://lore.kernel.org/all/CAE1SXrv3Ouwt4Y9NEWGi0WO701w1YP1ruMSxraZr4PZTGsUZgg@xxxxxxxxxxxxxx Link: https://lore.kernel.org/all/aa64ef28-35d8-9deb-2756-8080296b7e3e@xxxxxxx Cc: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx> Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> Signed-off-by: Xiaochen Zou <xzou017@xxxxxxx> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> --- Hello, I picked up the patch from Xiaochen Zou. I think it is the proposed fix for: | https://syzkaller.appspot.com/bug?extid=a47537d3964ef6c874e1 It turned out that | 0c71437dd50d can: j1939: j1939_session_deactivate(): clarify lifetime of session object is wrong, and should be removed, as Ziyang Xuan proposed in: | https://lore.kernel.org/all/20210906094200.95868-1-william.xuanziyang@xxxxxxxxxx Ziyang Xuan, Oleksij, can you have a look at Xiaochen Zou's patch and give me an Ack, then I'll take both patches upstream. regards, Marc net/can/j1939/transport.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c index dc3c30810833..35530b09c84f 100644 --- a/net/can/j1939/transport.c +++ b/net/can/j1939/transport.c @@ -1072,7 +1072,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; @@ -1086,6 +1085,8 @@ static bool j1939_session_deactivate(struct j1939_session *session) j1939_session_list_lock(priv); active = j1939_session_deactivate_locked(session); j1939_session_list_unlock(priv); + if (active) + j1939_session_put(session); return active; } @@ -2152,6 +2153,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); @@ -2165,7 +2167,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); -- 2.33.0
