> 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 I think the session kref >= 2 when it is active state. The j1939_session_put() in j1939_session_deactivate_locked() will not trigger __j1939_session_release() to free session. 0c71437dd50d ("can: j1939: j1939_session_deactivate(): clarify lifetime of session object") just only partly wrong. j1939_session_deactivate() is called not only when session is active, it may be called when session is not active already. And I think the 0c71437dd50d may be the real fix for: https://syzkaller.appspot.com/bug?extid=a47537d3964ef6c874e1 I can not find an exact scenario as Xiaochen Zou's patch mentioned. So I can not agree. Or can you give an exact scenario @Xiaochen Zou. Thank you! > > 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); >
