[PATCH 1/6] can: j1939: fix kernel-infoleak in j1939_sk_sock2sockaddr_can()
From: Eric Dumazet <edumazet@google.com> mainline inclusion from mainline-v5.9-rc2 commit 38ba8b9241f5848a49b80fddac9ab5f4692e434e category: bugfix bugzilla: 38684 CVE: NA --------------------------- syzbot found that at least 2 bytes of kernel information were leaked during getsockname() on AF_CAN CAN_J1939 socket. Since struct sockaddr_can has in fact two holes, simply clear the whole area before filling it with useful data. BUG: KMSAN: kernel-infoleak in kmsan_copy_to_user+0x81/0x90 mm/kmsan/kmsan_hooks.c:253 CPU: 0 PID: 8466 Comm: syz-executor511 Not tainted 5.8.0-rc5-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x21c/0x280 lib/dump_stack.c:118 kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:121 kmsan_internal_check_memory+0x238/0x3d0 mm/kmsan/kmsan.c:423 kmsan_copy_to_user+0x81/0x90 mm/kmsan/kmsan_hooks.c:253 instrument_copy_to_user include/linux/instrumented.h:91 [inline] _copy_to_user+0x18e/0x260 lib/usercopy.c:39 copy_to_user include/linux/uaccess.h:186 [inline] move_addr_to_user+0x3de/0x670 net/socket.c:237 __sys_getsockname+0x407/0x5e0 net/socket.c:1909 __do_sys_getsockname net/socket.c:1920 [inline] __se_sys_getsockname+0x91/0xb0 net/socket.c:1917 __x64_sys_getsockname+0x4a/0x70 net/socket.c:1917 do_syscall_64+0xad/0x160 arch/x86/entry/common.c:386 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x440219 Code: Bad RIP value. RSP: 002b:00007ffe5ee150c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000033 RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440219 RDX: 0000000020000240 RSI: 0000000020000100 RDI: 0000000000000003 RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000401a20 R13: 0000000000401ab0 R14: 0000000000000000 R15: 0000000000000000 Local variable ----address@__sys_getsockname created at: __sys_getsockname+0x91/0x5e0 net/socket.c:1894 __sys_getsockname+0x91/0x5e0 net/socket.c:1894 Bytes 2-3 of 24 are uninitialized Memory access of size 24 starts at ffff8880ba2c7de8 Data copied to user address 0000000020000100 Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") Signed-off-by: Eric Dumazet <edumazet@google.com> Reported-by: syzbot <syzkaller@googlegroups.com> Cc: Robin van der Gracht <robin@protonic.nl> Cc: Oleksij Rempel <o.rempel@pengutronix.de> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: linux-can@vger.kernel.org Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> Link: https://lore.kernel.org/r/20200813161834.4021638-1-edumazet@google.com Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> Reviewed-by: Yue Haibing <yuehaibing@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- net/can/j1939/socket.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c index f7587428febdd..2f4e2142ef268 100644 --- a/net/can/j1939/socket.c +++ b/net/can/j1939/socket.c @@ -553,6 +553,11 @@ static int j1939_sk_connect(struct socket *sock, struct sockaddr *uaddr, static void j1939_sk_sock2sockaddr_can(struct sockaddr_can *addr, const struct j1939_sock *jsk, int peer) { + /* There are two holes (2 bytes and 3 bytes) to clear to avoid + * leaking kernel information to user space. + */ + memset(addr, 0, J1939_MIN_NAMELEN); + addr->can_family = AF_CAN; addr->can_ifindex = jsk->ifindex; addr->can_addr.j1939.pgn = jsk->addr.pgn; -- 2.25.1
From: Oleksij Rempel <o.rempel@pengutronix.de> mainline inclusion from mainline-v5.9-rc2 commit b43e3a82bc432c1caaed8950e7662c143470c54c category: bugfix bugzilla: 38684 CVE: NA --------------------------- In current J1939 stack implementation, we process all locally send messages as own messages. Even if it was send by CAN_RAW socket. To reproduce it use following commands: testj1939 -P -r can0:0x80 & cansend can0 18238040#0123 This step will trigger false positive not critical warning: j1939_simple_recv: Received already invalidated message With this patch we add additional check to make sure, related skb is own echo message. Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Link: https://lore.kernel.org/r/20200807105200.26441-2-o.rempel@pengutronix.de Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> Reviewed-by: Yue Haibing <yuehaibing@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- net/can/j1939/socket.c | 1 + net/can/j1939/transport.c | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c index 2f4e2142ef268..7f003bb365bfd 100644 --- a/net/can/j1939/socket.c +++ b/net/can/j1939/socket.c @@ -398,6 +398,7 @@ static int j1939_sk_init(struct sock *sk) spin_lock_init(&jsk->sk_session_queue_lock); INIT_LIST_HEAD(&jsk->sk_session_queue); sk->sk_destruct = j1939_sk_sock_destruct; + sk->sk_protocol = CAN_J1939; return 0; } diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c index fad210ee4c785..0ead38b2fa485 100644 --- a/net/can/j1939/transport.c +++ b/net/can/j1939/transport.c @@ -2041,6 +2041,10 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb) if (!skb->sk) return; + if (skb->sk->sk_family != AF_CAN || + skb->sk->sk_protocol != CAN_J1939) + return; + j1939_session_list_lock(priv); session = j1939_session_get_simple(priv, skb); j1939_session_list_unlock(priv); -- 2.25.1
From: Oleksij Rempel <o.rempel@pengutronix.de> mainline inclusion from mainline-v5.9-rc2 commit cd3b3636c99fcac52c598b64061f3fe4413c6a12 category: bugfix bugzilla: 38684 CVE: NA --------------------------- The current stack implementation do not support ECTS requests of not aligned TP sized blocks. If ECTS will request a block with size and offset spanning two TP blocks, this will cause memcpy() to read beyond the queued skb (which does only contain one TP sized block). Sometimes KASAN will detect this read if the memory region beyond the skb was previously allocated and freed. In other situations it will stay undetected. The ETP transfer in any case will be corrupted. This patch adds a sanity check to avoid this kind of read and abort the session with error J1939_XTP_ABORT_ECTS_TOO_BIG. Reported-by: syzbot+5322482fe520b02aea30@syzkaller.appspotmail.com Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") Cc: linux-stable <stable@vger.kernel.org> # >= v5.4 Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Link: https://lore.kernel.org/r/20200807105200.26441-3-o.rempel@pengutronix.de Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> Reviewed-by: Yue Haibing <yuehaibing@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- net/can/j1939/transport.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c index 0ead38b2fa485..96853ca25b5f5 100644 --- a/net/can/j1939/transport.c +++ b/net/can/j1939/transport.c @@ -789,6 +789,18 @@ static int j1939_session_tx_dat(struct j1939_session *session) if (len > 7) len = 7; + if (offset + len > se_skb->len) { + 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; + } + + if (!len) { + ret = -ENOBUFS; + break; + } + memcpy(&dat[1], &tpdat[offset], len); ret = j1939_tp_tx_dat(session, dat, len + 1); if (ret < 0) { @@ -1122,6 +1134,9 @@ static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer) * cleanup including propagation of the error to user space. */ break; + case -EOVERFLOW: + j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG); + break; case 0: session->tx_retry = 0; break; -- 2.25.1
From: Oleksij Rempel <o.rempel@pengutronix.de> mainline inclusion from mainline-v5.9-rc2 commit af804b7826350d5af728dca4715e473338fbd7e5 category: bugfix bugzilla: 38684 CVE: NA --------------------------- This patch adds check to ensure that the struct net_device::ml_priv is allocated, as it is used later by the j1939 stack. The allocation is done by all mainline CAN network drivers, but when using bond or team devices this is not the case. Bail out if no ml_priv is allocated. Reported-by: syzbot+f03d384f3455d28833eb@syzkaller.appspotmail.com Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") Cc: linux-stable <stable@vger.kernel.org> # >= v5.4 Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Link: https://lore.kernel.org/r/20200807105200.26441-4-o.rempel@pengutronix.de Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> Reviewed-by: Yue Haibing <yuehaibing@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- net/can/j1939/socket.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c index 7f003bb365bfd..bf9fd6ee88fe0 100644 --- a/net/can/j1939/socket.c +++ b/net/can/j1939/socket.c @@ -467,6 +467,14 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr *uaddr, int len) goto out_release_sock; } + if (!ndev->ml_priv) { + netdev_warn_once(ndev, + "No CAN mid layer private allocated, please fix your driver and use alloc_candev()!\n"); + dev_put(ndev); + ret = -ENODEV; + goto out_release_sock; + } + priv = j1939_netdev_start(ndev); dev_put(ndev); if (IS_ERR(priv)) { -- 2.25.1
From: Oleksij Rempel <o.rempel@pengutronix.de> mainline inclusion from mainline-v5.9-rc2 commit 840835c9281215341d84966a8855f267a971e6a3 category: bugfix bugzilla: 38684 CVE: NA --------------------------- Sometimes it makes no sense to search the skb by pkt.dpo, since we need next the skb within the transaction block. This may happen if we have an ETP session with CTS set to less than 255 packets. After this patch, we will be able to work with ETP sessions where the block size (ETP.CM_CTS byte 2) is less than 255 packets. Reported-by: Henrique Figueira <henrislip@gmail.com> Reported-by: https://github.com/linux-can/can-utils/issues/228 Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Link: https://lore.kernel.org/r/20200807105200.26441-5-o.rempel@pengutronix.de Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> Reviewed-by: Yue Haibing <yuehaibing@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- net/can/j1939/transport.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c index 96853ca25b5f5..dbd215cbc53d8 100644 --- a/net/can/j1939/transport.c +++ b/net/can/j1939/transport.c @@ -352,17 +352,16 @@ void j1939_session_skb_queue(struct j1939_session *session, skb_queue_tail(&session->skb_queue, skb); } -static struct sk_buff *j1939_session_skb_find(struct j1939_session *session) +static struct +sk_buff *j1939_session_skb_find_by_offset(struct j1939_session *session, + unsigned int offset_start) { struct j1939_priv *priv = session->priv; + struct j1939_sk_buff_cb *do_skcb; struct sk_buff *skb = NULL; struct sk_buff *do_skb; - struct j1939_sk_buff_cb *do_skcb; - unsigned int offset_start; unsigned long flags; - offset_start = session->pkt.dpo * 7; - spin_lock_irqsave(&session->skb_queue.lock, flags); skb_queue_walk(&session->skb_queue, do_skb) { do_skcb = j1939_skb_to_cb(do_skb); @@ -382,6 +381,14 @@ static struct sk_buff *j1939_session_skb_find(struct j1939_session *session) return skb; } +static struct sk_buff *j1939_session_skb_find(struct j1939_session *session) +{ + unsigned int offset_start; + + offset_start = session->pkt.dpo * 7; + return j1939_session_skb_find_by_offset(session, offset_start); +} + /* see if we are receiver * returns 0 for broadcasts, although we will receive them */ @@ -768,7 +775,7 @@ static int j1939_session_tx_dat(struct j1939_session *session) int ret = 0; u8 dat[8]; - se_skb = j1939_session_skb_find(session); + se_skb = j1939_session_skb_find_by_offset(session, session->pkt.tx * 7); if (!se_skb) return -ENOBUFS; @@ -1776,7 +1783,8 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, __func__, session); goto out_session_cancel; } - se_skb = j1939_session_skb_find(session); + + se_skb = j1939_session_skb_find_by_offset(session, packet * 7); if (!se_skb) { netdev_warn(priv->ndev, "%s: 0x%p: no skb found\n", __func__, session); -- 2.25.1
From: Oleksij Rempel <o.rempel@pengutronix.de> mainline inclusion from mainline-v5.9-rc2 commit e052d0540298bfe0f6cbbecdc7e2ea9b859575b2 category: bugfix bugzilla: 38684 CVE: NA --------------------------- Since the stack relays on receiving own packets, it was overwriting own transmit buffer from received packets. At least theoretically, the received echo buffer can be corrupt or changed and the session partner can request to resend previous data. In this case we will re-send bad data. With this patch we will stop to overwrite own TX buffer and use it for sanity checking. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Link: https://lore.kernel.org/r/20200807105200.26441-6-o.rempel@pengutronix.de Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> Reviewed-by: Yue Haibing <yuehaibing@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- net/can/j1939/transport.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c index dbd215cbc53d8..a8dd956b5e8e1 100644 --- a/net/can/j1939/transport.c +++ b/net/can/j1939/transport.c @@ -1803,7 +1803,20 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, } tpdat = se_skb->data; - memcpy(&tpdat[offset], &dat[1], nbytes); + if (!session->transmission) { + memcpy(&tpdat[offset], &dat[1], nbytes); + } else { + int err; + + err = memcmp(&tpdat[offset], &dat[1], nbytes); + if (err) + netdev_err_once(priv->ndev, + "%s: 0x%p: Data of RX-looped back packet (%*ph) doesn't match TX data (%*ph)!\n", + __func__, session, + nbytes, &dat[1], + nbytes, &tpdat[offset]); + } + if (packet == session->pkt.rx) session->pkt.rx++; -- 2.25.1
participants (1)
-
Yang Yingliang