From 2030043e616cab40f510299f09b636285e0a3678 Mon Sep 17 00:00:00 2001 From: Oleksij Rempel Date: Fri, 21 May 2021 13:57:20 +0200 Subject: can: j1939: fix Use-after-Free, hold skb ref while in use This patch fixes a Use-after-Free found by the syzbot. The problem is that a skb is taken from the per-session skb queue, without incrementing the ref count. This leads to a Use-after-Free if the skb is taken concurrently from the session queue due to a CTS. Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") Link: https://lore.kernel.org/r/20210521115720.7533-1-o.rempel@pengutronix.de Cc: Hillf Danton Cc: linux-stable Reported-by: syzbot+220c1a29987a9a490903@syzkaller.appspotmail.com Reported-by: syzbot+45199c1b73b4013525cf@syzkaller.appspotmail.com Signed-off-by: Oleksij Rempel Signed-off-by: Marc Kleine-Budde --- net/can/j1939/transport.c | 54 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 14 deletions(-) (limited to 'net') diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c index e09d087ba240..c3946c355882 100644 --- a/net/can/j1939/transport.c +++ b/net/can/j1939/transport.c @@ -330,6 +330,9 @@ static void j1939_session_skb_drop_old(struct j1939_session *session) if ((do_skcb->offset + do_skb->len) < offset_start) { __skb_unlink(do_skb, &session->skb_queue); + /* drop ref taken in j1939_session_skb_queue() */ + skb_unref(do_skb); + kfree_skb(do_skb); } spin_unlock_irqrestore(&session->skb_queue.lock, flags); @@ -349,12 +352,13 @@ void j1939_session_skb_queue(struct j1939_session *session, skcb->flags |= J1939_ECU_LOCAL_SRC; + skb_get(skb); skb_queue_tail(&session->skb_queue, skb); } static struct -sk_buff *j1939_session_skb_find_by_offset(struct j1939_session *session, - unsigned int offset_start) +sk_buff *j1939_session_skb_get_by_offset(struct j1939_session *session, + unsigned int offset_start) { struct j1939_priv *priv = session->priv; struct j1939_sk_buff_cb *do_skcb; @@ -371,6 +375,10 @@ sk_buff *j1939_session_skb_find_by_offset(struct j1939_session *session, skb = do_skb; } } + + if (skb) + skb_get(skb); + spin_unlock_irqrestore(&session->skb_queue.lock, flags); if (!skb) @@ -381,12 +389,12 @@ sk_buff *j1939_session_skb_find_by_offset(struct j1939_session *session, return skb; } -static struct sk_buff *j1939_session_skb_find(struct j1939_session *session) +static struct sk_buff *j1939_session_skb_get(struct j1939_session *session) { unsigned int offset_start; offset_start = session->pkt.dpo * 7; - return j1939_session_skb_find_by_offset(session, offset_start); + return j1939_session_skb_get_by_offset(session, offset_start); } /* see if we are receiver @@ -776,7 +784,7 @@ static int j1939_session_tx_dat(struct j1939_session *session) int ret = 0; u8 dat[8]; - se_skb = j1939_session_skb_find_by_offset(session, session->pkt.tx * 7); + se_skb = j1939_session_skb_get_by_offset(session, session->pkt.tx * 7); if (!se_skb) return -ENOBUFS; @@ -801,7 +809,8 @@ static int j1939_session_tx_dat(struct j1939_session *session) netdev_err_once(priv->ndev, "%s: 0x%p: requested data outside of queued buffer: offset %i, len %i, pkt.tx: %i\n", __func__, session, skcb->offset, se_skb->len , session->pkt.tx); - return -EOVERFLOW; + ret = -EOVERFLOW; + goto out_free; } if (!len) { @@ -835,6 +844,12 @@ static int j1939_session_tx_dat(struct j1939_session *session) if (pkt_done) j1939_tp_set_rxtimeout(session, 250); + out_free: + if (ret) + kfree_skb(se_skb); + else + consume_skb(se_skb); + return ret; } @@ -1007,7 +1022,7 @@ static int j1939_xtp_txnext_receiver(struct j1939_session *session) static int j1939_simple_txnext(struct j1939_session *session) { struct j1939_priv *priv = session->priv; - struct sk_buff *se_skb = j1939_session_skb_find(session); + struct sk_buff *se_skb = j1939_session_skb_get(session); struct sk_buff *skb; int ret; @@ -1015,8 +1030,10 @@ static int j1939_simple_txnext(struct j1939_session *session) return 0; skb = skb_clone(se_skb, GFP_ATOMIC); - if (!skb) - return -ENOMEM; + if (!skb) { + ret = -ENOMEM; + goto out_free; + } can_skb_set_owner(skb, se_skb->sk); @@ -1024,12 +1041,18 @@ static int j1939_simple_txnext(struct j1939_session *session) ret = j1939_send_one(priv, skb); if (ret) - return ret; + goto out_free; j1939_sk_errqueue(session, J1939_ERRQUEUE_SCHED); j1939_sk_queue_activate_next(session); - return 0; + out_free: + if (ret) + kfree_skb(se_skb); + else + consume_skb(se_skb); + + return ret; } static bool j1939_session_deactivate_locked(struct j1939_session *session) @@ -1170,9 +1193,10 @@ static void j1939_session_completed(struct j1939_session *session) struct sk_buff *skb; if (!session->transmission) { - skb = j1939_session_skb_find(session); + skb = j1939_session_skb_get(session); /* distribute among j1939 receivers */ j1939_sk_recv(session->priv, skb); + consume_skb(skb); } j1939_session_deactivate_activate_next(session); @@ -1744,7 +1768,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, { struct j1939_priv *priv = session->priv; struct j1939_sk_buff_cb *skcb; - struct sk_buff *se_skb; + struct sk_buff *se_skb = NULL; const u8 *dat; u8 *tpdat; int offset; @@ -1786,7 +1810,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, goto out_session_cancel; } - se_skb = j1939_session_skb_find_by_offset(session, packet * 7); + se_skb = j1939_session_skb_get_by_offset(session, packet * 7); if (!se_skb) { netdev_warn(priv->ndev, "%s: 0x%p: no skb found\n", __func__, session); @@ -1848,11 +1872,13 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, j1939_tp_set_rxtimeout(session, 250); } session->last_cmd = 0xff; + consume_skb(se_skb); j1939_session_put(session); return; out_session_cancel: + kfree_skb(se_skb); j1939_session_timers_cancel(session); j1939_session_cancel(session, J1939_XTP_ABORT_FAULT); j1939_session_put(session); -- cgit v1.2.3-58-ga151 From 8d0caedb759683041d9db82069937525999ada53 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa Date: Sat, 5 Jun 2021 19:26:35 +0900 Subject: can: bcm/raw/isotp: use per module netdevice notifier syzbot is reporting hung task at register_netdevice_notifier() [1] and unregister_netdevice_notifier() [2], for cleanup_net() might perform time consuming operations while CAN driver's raw/bcm/isotp modules are calling {register,unregister}_netdevice_notifier() on each socket. Change raw/bcm/isotp modules to call register_netdevice_notifier() from module's __init function and call unregister_netdevice_notifier() from module's __exit function, as with gw/j1939 modules are doing. Link: https://syzkaller.appspot.com/bug?id=391b9498827788b3cc6830226d4ff5be87107c30 [1] Link: https://syzkaller.appspot.com/bug?id=1724d278c83ca6e6df100a2e320c10d991cf2bce [2] Link: https://lore.kernel.org/r/54a5f451-05ed-f977-8534-79e7aa2bcc8f@i-love.sakura.ne.jp Cc: linux-stable Reported-by: syzbot Reported-by: syzbot Reviewed-by: Kirill Tkhai Tested-by: syzbot Tested-by: Oliver Hartkopp Signed-off-by: Tetsuo Handa Signed-off-by: Marc Kleine-Budde --- net/can/bcm.c | 59 ++++++++++++++++++++++++++++++++++++++++++------------ net/can/isotp.c | 61 ++++++++++++++++++++++++++++++++++++++++++++------------ net/can/raw.c | 62 ++++++++++++++++++++++++++++++++++++++++++++------------- 3 files changed, 142 insertions(+), 40 deletions(-) (limited to 'net') diff --git a/net/can/bcm.c b/net/can/bcm.c index 909b9e684e04..f00176b2a6c3 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -125,7 +125,7 @@ struct bcm_sock { struct sock sk; int bound; int ifindex; - struct notifier_block notifier; + struct list_head notifier; struct list_head rx_ops; struct list_head tx_ops; unsigned long dropped_usr_msgs; @@ -133,6 +133,10 @@ struct bcm_sock { char procname [32]; /* inode number in decimal with \0 */ }; +static LIST_HEAD(bcm_notifier_list); +static DEFINE_SPINLOCK(bcm_notifier_lock); +static struct bcm_sock *bcm_busy_notifier; + static inline struct bcm_sock *bcm_sk(const struct sock *sk) { return (struct bcm_sock *)sk; @@ -1378,20 +1382,15 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) /* * notification handler for netdevice status changes */ -static int bcm_notifier(struct notifier_block *nb, unsigned long msg, - void *ptr) +static void bcm_notify(struct bcm_sock *bo, unsigned long msg, + struct net_device *dev) { - struct net_device *dev = netdev_notifier_info_to_dev(ptr); - struct bcm_sock *bo = container_of(nb, struct bcm_sock, notifier); struct sock *sk = &bo->sk; struct bcm_op *op; int notify_enodev = 0; if (!net_eq(dev_net(dev), sock_net(sk))) - return NOTIFY_DONE; - - if (dev->type != ARPHRD_CAN) - return NOTIFY_DONE; + return; switch (msg) { @@ -1426,7 +1425,28 @@ static int bcm_notifier(struct notifier_block *nb, unsigned long msg, sk->sk_error_report(sk); } } +} +static int bcm_notifier(struct notifier_block *nb, unsigned long msg, + void *ptr) +{ + struct net_device *dev = netdev_notifier_info_to_dev(ptr); + + if (dev->type != ARPHRD_CAN) + return NOTIFY_DONE; + if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN) + return NOTIFY_DONE; + if (unlikely(bcm_busy_notifier)) /* Check for reentrant bug. */ + return NOTIFY_DONE; + + spin_lock(&bcm_notifier_lock); + list_for_each_entry(bcm_busy_notifier, &bcm_notifier_list, notifier) { + spin_unlock(&bcm_notifier_lock); + bcm_notify(bcm_busy_notifier, msg, dev); + spin_lock(&bcm_notifier_lock); + } + bcm_busy_notifier = NULL; + spin_unlock(&bcm_notifier_lock); return NOTIFY_DONE; } @@ -1446,9 +1466,9 @@ static int bcm_init(struct sock *sk) INIT_LIST_HEAD(&bo->rx_ops); /* set notifier */ - bo->notifier.notifier_call = bcm_notifier; - - register_netdevice_notifier(&bo->notifier); + spin_lock(&bcm_notifier_lock); + list_add_tail(&bo->notifier, &bcm_notifier_list); + spin_unlock(&bcm_notifier_lock); return 0; } @@ -1471,7 +1491,14 @@ static int bcm_release(struct socket *sock) /* remove bcm_ops, timer, rx_unregister(), etc. */ - unregister_netdevice_notifier(&bo->notifier); + spin_lock(&bcm_notifier_lock); + while (bcm_busy_notifier == bo) { + spin_unlock(&bcm_notifier_lock); + schedule_timeout_uninterruptible(1); + spin_lock(&bcm_notifier_lock); + } + list_del(&bo->notifier); + spin_unlock(&bcm_notifier_lock); lock_sock(sk); @@ -1692,6 +1719,10 @@ static struct pernet_operations canbcm_pernet_ops __read_mostly = { .exit = canbcm_pernet_exit, }; +static struct notifier_block canbcm_notifier = { + .notifier_call = bcm_notifier +}; + static int __init bcm_module_init(void) { int err; @@ -1705,12 +1736,14 @@ static int __init bcm_module_init(void) } register_pernet_subsys(&canbcm_pernet_ops); + register_netdevice_notifier(&canbcm_notifier); return 0; } static void __exit bcm_module_exit(void) { can_proto_unregister(&bcm_can_proto); + unregister_netdevice_notifier(&canbcm_notifier); unregister_pernet_subsys(&canbcm_pernet_ops); } diff --git a/net/can/isotp.c b/net/can/isotp.c index 253b24417c8e..be6183f8ca11 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -143,10 +143,14 @@ struct isotp_sock { u32 force_tx_stmin; u32 force_rx_stmin; struct tpcon rx, tx; - struct notifier_block notifier; + struct list_head notifier; wait_queue_head_t wait; }; +static LIST_HEAD(isotp_notifier_list); +static DEFINE_SPINLOCK(isotp_notifier_lock); +static struct isotp_sock *isotp_busy_notifier; + static inline struct isotp_sock *isotp_sk(const struct sock *sk) { return (struct isotp_sock *)sk; @@ -1013,7 +1017,14 @@ static int isotp_release(struct socket *sock) /* wait for complete transmission of current pdu */ wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); - unregister_netdevice_notifier(&so->notifier); + spin_lock(&isotp_notifier_lock); + while (isotp_busy_notifier == so) { + spin_unlock(&isotp_notifier_lock); + schedule_timeout_uninterruptible(1); + spin_lock(&isotp_notifier_lock); + } + list_del(&so->notifier); + spin_unlock(&isotp_notifier_lock); lock_sock(sk); @@ -1317,21 +1328,16 @@ static int isotp_getsockopt(struct socket *sock, int level, int optname, return 0; } -static int isotp_notifier(struct notifier_block *nb, unsigned long msg, - void *ptr) +static void isotp_notify(struct isotp_sock *so, unsigned long msg, + struct net_device *dev) { - struct net_device *dev = netdev_notifier_info_to_dev(ptr); - struct isotp_sock *so = container_of(nb, struct isotp_sock, notifier); struct sock *sk = &so->sk; if (!net_eq(dev_net(dev), sock_net(sk))) - return NOTIFY_DONE; - - if (dev->type != ARPHRD_CAN) - return NOTIFY_DONE; + return; if (so->ifindex != dev->ifindex) - return NOTIFY_DONE; + return; switch (msg) { case NETDEV_UNREGISTER: @@ -1357,7 +1363,28 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg, sk->sk_error_report(sk); break; } +} +static int isotp_notifier(struct notifier_block *nb, unsigned long msg, + void *ptr) +{ + struct net_device *dev = netdev_notifier_info_to_dev(ptr); + + if (dev->type != ARPHRD_CAN) + return NOTIFY_DONE; + if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN) + return NOTIFY_DONE; + if (unlikely(isotp_busy_notifier)) /* Check for reentrant bug. */ + return NOTIFY_DONE; + + spin_lock(&isotp_notifier_lock); + list_for_each_entry(isotp_busy_notifier, &isotp_notifier_list, notifier) { + spin_unlock(&isotp_notifier_lock); + isotp_notify(isotp_busy_notifier, msg, dev); + spin_lock(&isotp_notifier_lock); + } + isotp_busy_notifier = NULL; + spin_unlock(&isotp_notifier_lock); return NOTIFY_DONE; } @@ -1394,8 +1421,9 @@ static int isotp_init(struct sock *sk) init_waitqueue_head(&so->wait); - so->notifier.notifier_call = isotp_notifier; - register_netdevice_notifier(&so->notifier); + spin_lock(&isotp_notifier_lock); + list_add_tail(&so->notifier, &isotp_notifier_list); + spin_unlock(&isotp_notifier_lock); return 0; } @@ -1442,6 +1470,10 @@ static const struct can_proto isotp_can_proto = { .prot = &isotp_proto, }; +static struct notifier_block canisotp_notifier = { + .notifier_call = isotp_notifier +}; + static __init int isotp_module_init(void) { int err; @@ -1451,6 +1483,8 @@ static __init int isotp_module_init(void) err = can_proto_register(&isotp_can_proto); if (err < 0) pr_err("can: registration of isotp protocol failed\n"); + else + register_netdevice_notifier(&canisotp_notifier); return err; } @@ -1458,6 +1492,7 @@ static __init int isotp_module_init(void) static __exit void isotp_module_exit(void) { can_proto_unregister(&isotp_can_proto); + unregister_netdevice_notifier(&canisotp_notifier); } module_init(isotp_module_init); diff --git a/net/can/raw.c b/net/can/raw.c index 139d9471ddcf..ac96fc210025 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -83,7 +83,7 @@ struct raw_sock { struct sock sk; int bound; int ifindex; - struct notifier_block notifier; + struct list_head notifier; int loopback; int recv_own_msgs; int fd_frames; @@ -95,6 +95,10 @@ struct raw_sock { struct uniqframe __percpu *uniq; }; +static LIST_HEAD(raw_notifier_list); +static DEFINE_SPINLOCK(raw_notifier_lock); +static struct raw_sock *raw_busy_notifier; + /* Return pointer to store the extra msg flags for raw_recvmsg(). * We use the space of one unsigned int beyond the 'struct sockaddr_can' * in skb->cb. @@ -263,21 +267,16 @@ static int raw_enable_allfilters(struct net *net, struct net_device *dev, return err; } -static int raw_notifier(struct notifier_block *nb, - unsigned long msg, void *ptr) +static void raw_notify(struct raw_sock *ro, unsigned long msg, + struct net_device *dev) { - struct net_device *dev = netdev_notifier_info_to_dev(ptr); - struct raw_sock *ro = container_of(nb, struct raw_sock, notifier); struct sock *sk = &ro->sk; if (!net_eq(dev_net(dev), sock_net(sk))) - return NOTIFY_DONE; - - if (dev->type != ARPHRD_CAN) - return NOTIFY_DONE; + return; if (ro->ifindex != dev->ifindex) - return NOTIFY_DONE; + return; switch (msg) { case NETDEV_UNREGISTER: @@ -305,7 +304,28 @@ static int raw_notifier(struct notifier_block *nb, sk->sk_error_report(sk); break; } +} + +static int raw_notifier(struct notifier_block *nb, unsigned long msg, + void *ptr) +{ + struct net_device *dev = netdev_notifier_info_to_dev(ptr); + + if (dev->type != ARPHRD_CAN) + return NOTIFY_DONE; + if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN) + return NOTIFY_DONE; + if (unlikely(raw_busy_notifier)) /* Check for reentrant bug. */ + return NOTIFY_DONE; + spin_lock(&raw_notifier_lock); + list_for_each_entry(raw_busy_notifier, &raw_notifier_list, notifier) { + spin_unlock(&raw_notifier_lock); + raw_notify(raw_busy_notifier, msg, dev); + spin_lock(&raw_notifier_lock); + } + raw_busy_notifier = NULL; + spin_unlock(&raw_notifier_lock); return NOTIFY_DONE; } @@ -334,9 +354,9 @@ static int raw_init(struct sock *sk) return -ENOMEM; /* set notifier */ - ro->notifier.notifier_call = raw_notifier; - - register_netdevice_notifier(&ro->notifier); + spin_lock(&raw_notifier_lock); + list_add_tail(&ro->notifier, &raw_notifier_list); + spin_unlock(&raw_notifier_lock); return 0; } @@ -351,7 +371,14 @@ static int raw_release(struct socket *sock) ro = raw_sk(sk); - unregister_netdevice_notifier(&ro->notifier); + spin_lock(&raw_notifier_lock); + while (raw_busy_notifier == ro) { + spin_unlock(&raw_notifier_lock); + schedule_timeout_uninterruptible(1); + spin_lock(&raw_notifier_lock); + } + list_del(&ro->notifier); + spin_unlock(&raw_notifier_lock); lock_sock(sk); @@ -889,6 +916,10 @@ static const struct can_proto raw_can_proto = { .prot = &raw_proto, }; +static struct notifier_block canraw_notifier = { + .notifier_call = raw_notifier +}; + static __init int raw_module_init(void) { int err; @@ -898,6 +929,8 @@ static __init int raw_module_init(void) err = can_proto_register(&raw_can_proto); if (err < 0) pr_err("can: registration of raw protocol failed\n"); + else + register_netdevice_notifier(&canraw_notifier); return err; } @@ -905,6 +938,7 @@ static __init int raw_module_init(void) static __exit void raw_module_exit(void) { can_proto_unregister(&raw_can_proto); + unregister_netdevice_notifier(&canraw_notifier); } module_init(raw_module_init); -- cgit v1.2.3-58-ga151 From 5e87ddbe3942e27e939bdc02deb8579b0cbd8ecc Mon Sep 17 00:00:00 2001 From: Norbert Slusarek Date: Sat, 12 Jun 2021 22:18:54 +0200 Subject: can: bcm: fix infoleak in struct bcm_msg_head On 64-bit systems, struct bcm_msg_head has an added padding of 4 bytes between struct members count and ival1. Even though all struct members are initialized, the 4-byte hole will contain data from the kernel stack. This patch zeroes out struct bcm_msg_head before usage, preventing infoleaks to userspace. Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol") Link: https://lore.kernel.org/r/trinity-7c1b2e82-e34f-4885-8060-2cd7a13769ce-1623532166177@3c-app-gmx-bs52 Cc: linux-stable Signed-off-by: Norbert Slusarek Acked-by: Oliver Hartkopp Signed-off-by: Marc Kleine-Budde --- net/can/bcm.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'net') diff --git a/net/can/bcm.c b/net/can/bcm.c index f00176b2a6c3..f3e4d9528fa3 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -406,6 +406,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer) if (!op->count && (op->flags & TX_COUNTEVT)) { /* create notification to user */ + memset(&msg_head, 0, sizeof(msg_head)); msg_head.opcode = TX_EXPIRED; msg_head.flags = op->flags; msg_head.count = op->count; @@ -443,6 +444,7 @@ static void bcm_rx_changed(struct bcm_op *op, struct canfd_frame *data) /* this element is not throttled anymore */ data->flags &= (BCM_CAN_FLAGS_MASK|RX_RECV); + memset(&head, 0, sizeof(head)); head.opcode = RX_CHANGED; head.flags = op->flags; head.count = op->count; @@ -564,6 +566,7 @@ static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer) } /* create notification to user */ + memset(&msg_head, 0, sizeof(msg_head)); msg_head.opcode = RX_TIMEOUT; msg_head.flags = op->flags; msg_head.count = op->count; -- cgit v1.2.3-58-ga151