Skip to content

Commit b3dff0e

Browse files
davem330davem330
committed
Merge tag 'linux-can-fixes-for-5.4-20191113' of git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can
Marc Kleine-Budde says: ==================== pull-request: can 2019-11-13 this is a pull request of 9 patches for net/master, hopefully for the v5.4 release cycle. All nine patches are by Oleksij Rempel and fix locking and use-after-free bugs in the j1939 stack found by the syzkaller syzbot. ==================== Signed-off-by: David S. Miller <[email protected]>
2 parents c3afb7e + 4a15d57 commit b3dff0e

File tree

5 files changed

+113
-30
lines changed

5 files changed

+113
-30
lines changed

‎include/linux/can/core.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,5 +65,6 @@ extern void can_rx_unregister(struct net *net, struct net_device *dev,
6565
void *data);
6666

6767
extern int can_send(struct sk_buff *skb, int loop);
68+
void can_sock_destruct(struct sock *sk);
6869

6970
#endif /* !_CAN_CORE_H */

‎net/can/af_can.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,11 +86,12 @@ static atomic_t skbcounter = ATOMIC_INIT(0);
8686

8787
/* af_can socket functions */
8888

89-
static void can_sock_destruct(struct sock *sk)
89+
void can_sock_destruct(struct sock *sk)
9090
{
9191
skb_queue_purge(&sk->sk_receive_queue);
9292
skb_queue_purge(&sk->sk_error_queue);
9393
}
94+
EXPORT_SYMBOL(can_sock_destruct);
9495

9596
static const struct can_proto *can_get_proto(int protocol)
9697
{

‎net/can/j1939/main.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
5151
if (!skb)
5252
return;
5353

54+
j1939_priv_get(priv);
5455
can_skb_set_owner(skb, iskb->sk);
5556

5657
/* get a pointer to the header of the skb
@@ -104,6 +105,7 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
104105
j1939_simple_recv(priv, skb);
105106
j1939_sk_recv(priv, skb);
106107
done:
108+
j1939_priv_put(priv);
107109
kfree_skb(skb);
108110
}
109111

@@ -150,6 +152,10 @@ static void __j1939_priv_release(struct kref *kref)
150152

151153
netdev_dbg(priv->ndev, "%s: 0x%p\n", __func__, priv);
152154

155+
WARN_ON_ONCE(!list_empty(&priv->active_session_list));
156+
WARN_ON_ONCE(!list_empty(&priv->ecus));
157+
WARN_ON_ONCE(!list_empty(&priv->j1939_socks));
158+
153159
dev_put(ndev);
154160
kfree(priv);
155161
}
@@ -207,6 +213,9 @@ static inline struct j1939_priv *j1939_ndev_to_priv(struct net_device *ndev)
207213
{
208214
struct can_ml_priv *can_ml_priv = ndev->ml_priv;
209215

216+
if (!can_ml_priv)
217+
return NULL;
218+
210219
return can_ml_priv->j1939_priv;
211220
}
212221

‎net/can/j1939/socket.c

Lines changed: 74 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@ static void j1939_jsk_add(struct j1939_priv *priv, struct j1939_sock *jsk)
7878
{
7979
jsk->state |= J1939_SOCK_BOUND;
8080
j1939_priv_get(priv);
81-
jsk->priv = priv;
8281

8382
spin_lock_bh(&priv->j1939_socks_lock);
8483
list_add_tail(&jsk->list, &priv->j1939_socks);
@@ -91,7 +90,6 @@ static void j1939_jsk_del(struct j1939_priv *priv, struct j1939_sock *jsk)
9190
list_del_init(&jsk->list);
9291
spin_unlock_bh(&priv->j1939_socks_lock);
9392

94-
jsk->priv = NULL;
9593
j1939_priv_put(priv);
9694
jsk->state &= ~J1939_SOCK_BOUND;
9795
}
@@ -349,6 +347,34 @@ void j1939_sk_recv(struct j1939_priv *priv, struct sk_buff *skb)
349347
spin_unlock_bh(&priv->j1939_socks_lock);
350348
}
351349

350+
static void j1939_sk_sock_destruct(struct sock *sk)
351+
{
352+
struct j1939_sock *jsk = j1939_sk(sk);
353+
354+
/* This function will be call by the generic networking code, when then
355+
* the socket is ultimately closed (sk->sk_destruct).
356+
*
357+
* The race between
358+
* - processing a received CAN frame
359+
* (can_receive -> j1939_can_recv)
360+
* and accessing j1939_priv
361+
* ... and ...
362+
* - closing a socket
363+
* (j1939_can_rx_unregister -> can_rx_unregister)
364+
* and calling the final j1939_priv_put()
365+
*
366+
* is avoided by calling the final j1939_priv_put() from this
367+
* RCU deferred cleanup call.
368+
*/
369+
if (jsk->priv) {
370+
j1939_priv_put(jsk->priv);
371+
jsk->priv = NULL;
372+
}
373+
374+
/* call generic CAN sock destruct */
375+
can_sock_destruct(sk);
376+
}
377+
352378
static int j1939_sk_init(struct sock *sk)
353379
{
354380
struct j1939_sock *jsk = j1939_sk(sk);
@@ -371,6 +397,7 @@ static int j1939_sk_init(struct sock *sk)
371397
atomic_set(&jsk->skb_pending, 0);
372398
spin_lock_init(&jsk->sk_session_queue_lock);
373399
INIT_LIST_HEAD(&jsk->sk_session_queue);
400+
sk->sk_destruct = j1939_sk_sock_destruct;
374401

375402
return 0;
376403
}
@@ -443,6 +470,12 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
443470
}
444471

445472
jsk->ifindex = addr->can_ifindex;
473+
474+
/* the corresponding j1939_priv_put() is called via
475+
* sk->sk_destruct, which points to j1939_sk_sock_destruct()
476+
*/
477+
j1939_priv_get(priv);
478+
jsk->priv = priv;
446479
}
447480

448481
/* set default transmit pgn */
@@ -560,8 +593,8 @@ static int j1939_sk_release(struct socket *sock)
560593
if (!sk)
561594
return 0;
562595

563-
jsk = j1939_sk(sk);
564596
lock_sock(sk);
597+
jsk = j1939_sk(sk);
565598

566599
if (jsk->state & J1939_SOCK_BOUND) {
567600
struct j1939_priv *priv = jsk->priv;
@@ -1059,51 +1092,72 @@ static int j1939_sk_sendmsg(struct socket *sock, struct msghdr *msg,
10591092
{
10601093
struct sock *sk = sock->sk;
10611094
struct j1939_sock *jsk = j1939_sk(sk);
1062-
struct j1939_priv *priv = jsk->priv;
1095+
struct j1939_priv *priv;
10631096
int ifindex;
10641097
int ret;
10651098

1099+
lock_sock(sock->sk);
10661100
/* various socket state tests */
1067-
if (!(jsk->state & J1939_SOCK_BOUND))
1068-
return -EBADFD;
1101+
if (!(jsk->state & J1939_SOCK_BOUND)) {
1102+
ret = -EBADFD;
1103+
goto sendmsg_done;
1104+
}
10691105

1106+
priv = jsk->priv;
10701107
ifindex = jsk->ifindex;
10711108

1072-
if (!jsk->addr.src_name && jsk->addr.sa == J1939_NO_ADDR)
1109+
if (!jsk->addr.src_name && jsk->addr.sa == J1939_NO_ADDR) {
10731110
/* no source address assigned yet */
1074-
return -EBADFD;
1111+
ret = -EBADFD;
1112+
goto sendmsg_done;
1113+
}
10751114

10761115
/* deal with provided destination address info */
10771116
if (msg->msg_name) {
10781117
struct sockaddr_can *addr = msg->msg_name;
10791118

1080-
if (msg->msg_namelen < J1939_MIN_NAMELEN)
1081-
return -EINVAL;
1119+
if (msg->msg_namelen < J1939_MIN_NAMELEN) {
1120+
ret = -EINVAL;
1121+
goto sendmsg_done;
1122+
}
10821123

1083-
if (addr->can_family != AF_CAN)
1084-
return -EINVAL;
1124+
if (addr->can_family != AF_CAN) {
1125+
ret = -EINVAL;
1126+
goto sendmsg_done;
1127+
}
10851128

1086-
if (addr->can_ifindex && addr->can_ifindex != ifindex)
1087-
return -EBADFD;
1129+
if (addr->can_ifindex && addr->can_ifindex != ifindex) {
1130+
ret = -EBADFD;
1131+
goto sendmsg_done;
1132+
}
10881133

10891134
if (j1939_pgn_is_valid(addr->can_addr.j1939.pgn) &&
1090-
!j1939_pgn_is_clean_pdu(addr->can_addr.j1939.pgn))
1091-
return -EINVAL;
1135+
!j1939_pgn_is_clean_pdu(addr->can_addr.j1939.pgn)) {
1136+
ret = -EINVAL;
1137+
goto sendmsg_done;
1138+
}
10921139

10931140
if (!addr->can_addr.j1939.name &&
10941141
addr->can_addr.j1939.addr == J1939_NO_ADDR &&
1095-
!sock_flag(sk, SOCK_BROADCAST))
1142+
!sock_flag(sk, SOCK_BROADCAST)) {
10961143
/* broadcast, but SO_BROADCAST not set */
1097-
return -EACCES;
1144+
ret = -EACCES;
1145+
goto sendmsg_done;
1146+
}
10981147
} else {
10991148
if (!jsk->addr.dst_name && jsk->addr.da == J1939_NO_ADDR &&
1100-
!sock_flag(sk, SOCK_BROADCAST))
1149+
!sock_flag(sk, SOCK_BROADCAST)) {
11011150
/* broadcast, but SO_BROADCAST not set */
1102-
return -EACCES;
1151+
ret = -EACCES;
1152+
goto sendmsg_done;
1153+
}
11031154
}
11041155

11051156
ret = j1939_sk_send_loop(priv, sk, msg, size);
11061157

1158+
sendmsg_done:
1159+
release_sock(sock->sk);
1160+
11071161
return ret;
11081162
}
11091163

‎net/can/j1939/transport.c

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,7 @@ static void __j1939_session_drop(struct j1939_session *session)
255255
return;
256256

257257
j1939_sock_pending_del(session->sk);
258+
sock_put(session->sk);
258259
}
259260

260261
static void j1939_session_destroy(struct j1939_session *session)
@@ -266,6 +267,9 @@ static void j1939_session_destroy(struct j1939_session *session)
266267

267268
netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
268269

270+
WARN_ON_ONCE(!list_empty(&session->sk_session_queue_entry));
271+
WARN_ON_ONCE(!list_empty(&session->active_session_list_entry));
272+
269273
skb_queue_purge(&session->skb_queue);
270274
__j1939_session_drop(session);
271275
j1939_priv_put(session->priv);
@@ -1042,12 +1046,13 @@ j1939_session_deactivate_activate_next(struct j1939_session *session)
10421046
j1939_sk_queue_activate_next(session);
10431047
}
10441048

1045-
static void j1939_session_cancel(struct j1939_session *session,
1049+
static void __j1939_session_cancel(struct j1939_session *session,
10461050
enum j1939_xtp_abort err)
10471051
{
10481052
struct j1939_priv *priv = session->priv;
10491053

10501054
WARN_ON_ONCE(!err);
1055+
lockdep_assert_held(&session->priv->active_session_list_lock);
10511056

10521057
session->err = j1939_xtp_abort_to_errno(priv, err);
10531058
/* do not send aborts on incoming broadcasts */
@@ -1062,6 +1067,20 @@ static void j1939_session_cancel(struct j1939_session *session,
10621067
j1939_sk_send_loop_abort(session->sk, session->err);
10631068
}
10641069

1070+
static void j1939_session_cancel(struct j1939_session *session,
1071+
enum j1939_xtp_abort err)
1072+
{
1073+
j1939_session_list_lock(session->priv);
1074+
1075+
if (session->state >= J1939_SESSION_ACTIVE &&
1076+
session->state < J1939_SESSION_WAITING_ABORT) {
1077+
j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
1078+
__j1939_session_cancel(session, err);
1079+
}
1080+
1081+
j1939_session_list_unlock(session->priv);
1082+
}
1083+
10651084
static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
10661085
{
10671086
struct j1939_session *session =
@@ -1108,8 +1127,6 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
11081127
netdev_alert(priv->ndev, "%s: 0x%p: tx aborted with unknown reason: %i\n",
11091128
__func__, session, ret);
11101129
if (session->skcb.addr.type != J1939_SIMPLE) {
1111-
j1939_tp_set_rxtimeout(session,
1112-
J1939_XTP_ABORT_TIMEOUT_MS);
11131130
j1939_session_cancel(session, J1939_XTP_ABORT_OTHER);
11141131
} else {
11151132
session->err = ret;
@@ -1169,7 +1186,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
11691186
hrtimer_start(&session->rxtimer,
11701187
ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
11711188
HRTIMER_MODE_REL_SOFT);
1172-
j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
1189+
__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
11731190
}
11741191
j1939_session_list_unlock(session->priv);
11751192
}
@@ -1375,7 +1392,6 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
13751392

13761393
out_session_cancel:
13771394
j1939_session_timers_cancel(session);
1378-
j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
13791395
j1939_session_cancel(session, err);
13801396
}
13811397

@@ -1572,7 +1588,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
15721588

15731589
/* RTS on active session */
15741590
j1939_session_timers_cancel(session);
1575-
j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
15761591
j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
15771592
}
15781593

@@ -1583,7 +1598,6 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
15831598
session->last_cmd);
15841599

15851600
j1939_session_timers_cancel(session);
1586-
j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
15871601
j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
15881602

15891603
return -EBUSY;
@@ -1785,7 +1799,6 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
17851799

17861800
out_session_cancel:
17871801
j1939_session_timers_cancel(session);
1788-
j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
17891802
j1939_session_cancel(session, J1939_XTP_ABORT_FAULT);
17901803
j1939_session_put(session);
17911804
}
@@ -1866,6 +1879,7 @@ struct j1939_session *j1939_tp_send(struct j1939_priv *priv,
18661879
return ERR_PTR(-ENOMEM);
18671880

18681881
/* skb is recounted in j1939_session_new() */
1882+
sock_hold(skb->sk);
18691883
session->sk = skb->sk;
18701884
session->transmission = true;
18711885
session->pkt.total = (size + 6) / 7;
@@ -2028,7 +2042,11 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
20282042
&priv->active_session_list,
20292043
active_session_list_entry) {
20302044
if (!sk || sk == session->sk) {
2031-
j1939_session_timers_cancel(session);
2045+
if (hrtimer_try_to_cancel(&session->txtimer) == 1)
2046+
j1939_session_put(session);
2047+
if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
2048+
j1939_session_put(session);
2049+
20322050
session->err = ESHUTDOWN;
20332051
j1939_session_deactivate_locked(session);
20342052
}

0 commit comments

Comments
 (0)