|
| 1 | +can: j1939: prevent deadlock by changing j1939_socks_lock to rwlock |
| 2 | + |
| 3 | +jira LE-1907 |
| 4 | +cve CVE-2023-52638 |
| 5 | +Rebuild_History Non-Buildable kernel-5.14.0-427.26.1.el9_4 |
| 6 | +commit-author Ziqi Zhao < [email protected]> |
| 7 | +commit 6cdedc18ba7b9dacc36466e27e3267d201948c8d |
| 8 | +Empty-Commit: Cherry-Pick Conflicts during history rebuild. |
| 9 | +Will be included in final tarball splat. Ref for failed cherry-pick at: |
| 10 | +ciq/ciq_backports/kernel-5.14.0-427.26.1.el9_4/6cdedc18.failed |
| 11 | + |
| 12 | +The following 3 locks would race against each other, causing the |
| 13 | +deadlock situation in the Syzbot bug report: |
| 14 | + |
| 15 | +- j1939_socks_lock |
| 16 | +- active_session_list_lock |
| 17 | +- sk_session_queue_lock |
| 18 | + |
| 19 | +A reasonable fix is to change j1939_socks_lock to an rwlock, since in |
| 20 | +the rare situations where a write lock is required for the linked list |
| 21 | +that j1939_socks_lock is protecting, the code does not attempt to |
| 22 | +acquire any more locks. This would break the circular lock dependency, |
| 23 | +where, for example, the current thread already locks j1939_socks_lock |
| 24 | +and attempts to acquire sk_session_queue_lock, and at the same time, |
| 25 | +another thread attempts to acquire j1939_socks_lock while holding |
| 26 | +sk_session_queue_lock. |
| 27 | + |
| 28 | +NOTE: This patch along does not fix the unregister_netdevice bug |
| 29 | +reported by Syzbot; instead, it solves a deadlock situation to prepare |
| 30 | +for one or more further patches to actually fix the Syzbot bug, which |
| 31 | +appears to be a reference counting problem within the j1939 codebase. |
| 32 | + |
| 33 | + |
| 34 | + Signed-off-by: Ziqi Zhao < [email protected]> |
| 35 | + Reviewed-by: Oleksij Rempel < [email protected]> |
| 36 | + Acked-by: Oleksij Rempel < [email protected]> |
| 37 | +Link: https://lore.kernel.org/all/ [email protected] |
| 38 | +[mkl: remove unrelated newline change] |
| 39 | + |
| 40 | + Signed-off-by: Marc Kleine-Budde < [email protected]> |
| 41 | +(cherry picked from commit 6cdedc18ba7b9dacc36466e27e3267d201948c8d) |
| 42 | + Signed-off-by: Jonathan Maple < [email protected]> |
| 43 | + |
| 44 | +# Conflicts: |
| 45 | +# net/can/j1939/socket.c |
| 46 | +diff --cc net/can/j1939/socket.c |
| 47 | +index f0a16f3f8bb4,94cfc2315e54..000000000000 |
| 48 | +--- a/net/can/j1939/socket.c |
| 49 | ++++ b/net/can/j1939/socket.c |
| 50 | +@@@ -1008,8 -1067,34 +1008,32 @@@ void j1939_sk_errqueue(struct j1939_ses |
| 51 | + kfree_skb(skb); |
| 52 | + }; |
| 53 | + |
| 54 | +++<<<<<<< HEAD |
| 55 | +++======= |
| 56 | ++ void j1939_sk_errqueue(struct j1939_session *session, |
| 57 | ++ enum j1939_sk_errqueue_type type) |
| 58 | ++ { |
| 59 | ++ struct j1939_priv *priv = session->priv; |
| 60 | ++ struct j1939_sock *jsk; |
| 61 | ++ |
| 62 | ++ if (session->sk) { |
| 63 | ++ /* send TX notifications to the socket of origin */ |
| 64 | ++ __j1939_sk_errqueue(session, session->sk, type); |
| 65 | ++ return; |
| 66 | ++ } |
| 67 | ++ |
| 68 | ++ /* spread RX notifications to all sockets subscribed to this session */ |
| 69 | ++ read_lock_bh(&priv->j1939_socks_lock); |
| 70 | ++ list_for_each_entry(jsk, &priv->j1939_socks, list) { |
| 71 | ++ if (j1939_sk_recv_match_one(jsk, &session->skcb)) |
| 72 | ++ __j1939_sk_errqueue(session, &jsk->sk, type); |
| 73 | ++ } |
| 74 | ++ read_unlock_bh(&priv->j1939_socks_lock); |
| 75 | ++ }; |
| 76 | ++ |
| 77 | +++>>>>>>> 6cdedc18ba7b (can: j1939: prevent deadlock by changing j1939_socks_lock to rwlock) |
| 78 | + void j1939_sk_send_loop_abort(struct sock *sk, int err) |
| 79 | + { |
| 80 | + - struct j1939_sock *jsk = j1939_sk(sk); |
| 81 | + - |
| 82 | + - if (jsk->state & J1939_SOCK_ERRQUEUE) |
| 83 | + - return; |
| 84 | + - |
| 85 | + sk->sk_err = err; |
| 86 | + |
| 87 | + sk_error_report(sk); |
| 88 | +diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h |
| 89 | +index 12369b604ce9..2cb81ad81ecd 100644 |
| 90 | +--- a/net/can/j1939/j1939-priv.h |
| 91 | ++++ b/net/can/j1939/j1939-priv.h |
| 92 | +@@ -83,7 +83,7 @@ struct j1939_priv { |
| 93 | + unsigned int tp_max_packet_size; |
| 94 | + |
| 95 | + /* lock for j1939_socks list */ |
| 96 | +- spinlock_t j1939_socks_lock; |
| 97 | ++ rwlock_t j1939_socks_lock; |
| 98 | + struct list_head j1939_socks; |
| 99 | + |
| 100 | + struct kref rx_kref; |
| 101 | +diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c |
| 102 | +index 0e9af9075069..1d7e2f554d15 100644 |
| 103 | +--- a/net/can/j1939/main.c |
| 104 | ++++ b/net/can/j1939/main.c |
| 105 | +@@ -264,7 +264,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev) |
| 106 | + return ERR_PTR(-ENOMEM); |
| 107 | + |
| 108 | + j1939_tp_init(priv); |
| 109 | +- spin_lock_init(&priv->j1939_socks_lock); |
| 110 | ++ rwlock_init(&priv->j1939_socks_lock); |
| 111 | + INIT_LIST_HEAD(&priv->j1939_socks); |
| 112 | + |
| 113 | + spin_lock(&j1939_netdev_lock); |
| 114 | +* Unmerged path net/can/j1939/socket.c |
0 commit comments