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@xxxxxxxxx> 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
--
Xiaochen Zou
PhD Student
Department of Computer Science & Engineering
University of California, Riverside
