* [PATCH] can: isotp: fix potential CAN frame reception race in isotp_rcv()
@ 2022-02-08 20:00 Oliver Hartkopp
2022-02-09 7:48 ` Marc Kleine-Budde
0 siblings, 1 reply; 2+ messages in thread
From: Oliver Hartkopp @ 2022-02-08 20:00 UTC (permalink / raw)
To: netdev, linux-can, william.xuanziyang, mkl
Cc: Oliver Hartkopp, syzbot+4c63f36709a642f801c5
When receiving a CAN frame the current code logic does not consider
concurrently receiving processes which do not show up in real world
usage.
Ziyang Xuan writes:
The following syz problem is one of the scenarios. so->rx.len is
changed by isotp_rcv_ff() during isotp_rcv_cf(), so->rx.len equals
0 before alloc_skb() and equals 4096 after alloc_skb(). That will
trigger skb_over_panic() in skb_put().
=======================================================
CPU: 1 PID: 19 Comm: ksoftirqd/1 Not tainted 5.16.0-rc8-syzkaller #0
RIP: 0010:skb_panic+0x16c/0x16e net/core/skbuff.c:113
Call Trace:
<TASK>
skb_over_panic net/core/skbuff.c:118 [inline]
skb_put.cold+0x24/0x24 net/core/skbuff.c:1990
isotp_rcv_cf net/can/isotp.c:570 [inline]
isotp_rcv+0xa38/0x1e30 net/can/isotp.c:668
deliver net/can/af_can.c:574 [inline]
can_rcv_filter+0x445/0x8d0 net/can/af_can.c:635
can_receive+0x31d/0x580 net/can/af_can.c:665
can_rcv+0x120/0x1c0 net/can/af_can.c:696
__netif_receive_skb_one_core+0x114/0x180 net/core/dev.c:5465
__netif_receive_skb+0x24/0x1b0 net/core/dev.c:5579
Therefore we make sure the state changes and data structures stay
consistent at CAN frame reception time by adding a spin_lock in
isotp_rcv(). This fixes the issue reported by syzkaller but does not
affect real world operation.
Link: https://lore.kernel.org/linux-can/d7e69278-d741-c706-65e1-e87623d9a8e8@huawei.com/T/
Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Reported-by: syzbot+4c63f36709a642f801c5@syzkaller.appspotmail.com
Reported-by: Ziyang Xuan <william.xuanziyang@huawei.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/isotp.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 02cbcb2ecf0d..9149e8d8aefc 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -54,10 +54,11 @@
*/
#include <linux/module.h>
#include <linux/init.h>
#include <linux/interrupt.h>
+#include <linux/spinlock.h>
#include <linux/hrtimer.h>
#include <linux/wait.h>
#include <linux/uio.h>
#include <linux/net.h>
#include <linux/netdevice.h>
@@ -143,10 +144,11 @@ struct isotp_sock {
u32 force_tx_stmin;
u32 force_rx_stmin;
struct tpcon rx, tx;
struct list_head notifier;
wait_queue_head_t wait;
+ spinlock_t rx_lock; /* protect single thread state machine */
};
static LIST_HEAD(isotp_notifier_list);
static DEFINE_SPINLOCK(isotp_notifier_lock);
static struct isotp_sock *isotp_busy_notifier;
@@ -613,15 +615,21 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
if (ae && cf->data[0] != so->opt.rx_ext_address)
return;
n_pci_type = cf->data[ae] & 0xF0;
+ /* Make sure the state changes and data structures stay consistent at
+ * CAN frame reception time. This locking is not needed in real world
+ * use cases but the inconsistency can be triggered with syzkaller.
+ */
+ spin_lock(&so->rx_lock);
+
if (so->opt.flags & CAN_ISOTP_HALF_DUPLEX) {
/* check rx/tx path half duplex expectations */
if ((so->tx.state != ISOTP_IDLE && n_pci_type != N_PCI_FC) ||
(so->rx.state != ISOTP_IDLE && n_pci_type == N_PCI_FC))
- return;
+ goto out_unlock;
}
switch (n_pci_type) {
case N_PCI_FC:
/* tx path: flow control frame containing the FC parameters */
@@ -666,10 +674,13 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
case N_PCI_CF:
/* rx path: consecutive frame */
isotp_rcv_cf(sk, cf, ae, skb);
break;
}
+
+out_unlock:
+ spin_unlock(&so->rx_lock);
}
static void isotp_fill_dataframe(struct canfd_frame *cf, struct isotp_sock *so,
int ae, int off)
{
@@ -1442,10 +1453,11 @@ static int isotp_init(struct sock *sk)
so->rxtimer.function = isotp_rx_timer_handler;
hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
so->txtimer.function = isotp_tx_timer_handler;
init_waitqueue_head(&so->wait);
+ spin_lock_init(&so->rx_lock);
spin_lock(&isotp_notifier_lock);
list_add_tail(&so->notifier, &isotp_notifier_list);
spin_unlock(&isotp_notifier_lock);
--
2.30.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] can: isotp: fix potential CAN frame reception race in isotp_rcv()
2022-02-08 20:00 [PATCH] can: isotp: fix potential CAN frame reception race in isotp_rcv() Oliver Hartkopp
@ 2022-02-09 7:48 ` Marc Kleine-Budde
0 siblings, 0 replies; 2+ messages in thread
From: Marc Kleine-Budde @ 2022-02-09 7:48 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: netdev, linux-can, william.xuanziyang, syzbot+4c63f36709a642f801c5
[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]
On 08.02.2022 21:00:26, Oliver Hartkopp wrote:
> When receiving a CAN frame the current code logic does not consider
> concurrently receiving processes which do not show up in real world
> usage.
>
> Ziyang Xuan writes:
>
> The following syz problem is one of the scenarios. so->rx.len is
> changed by isotp_rcv_ff() during isotp_rcv_cf(), so->rx.len equals
> 0 before alloc_skb() and equals 4096 after alloc_skb(). That will
> trigger skb_over_panic() in skb_put().
>
> =======================================================
> CPU: 1 PID: 19 Comm: ksoftirqd/1 Not tainted 5.16.0-rc8-syzkaller #0
> RIP: 0010:skb_panic+0x16c/0x16e net/core/skbuff.c:113
> Call Trace:
> <TASK>
> skb_over_panic net/core/skbuff.c:118 [inline]
> skb_put.cold+0x24/0x24 net/core/skbuff.c:1990
> isotp_rcv_cf net/can/isotp.c:570 [inline]
> isotp_rcv+0xa38/0x1e30 net/can/isotp.c:668
> deliver net/can/af_can.c:574 [inline]
> can_rcv_filter+0x445/0x8d0 net/can/af_can.c:635
> can_receive+0x31d/0x580 net/can/af_can.c:665
> can_rcv+0x120/0x1c0 net/can/af_can.c:696
> __netif_receive_skb_one_core+0x114/0x180 net/core/dev.c:5465
> __netif_receive_skb+0x24/0x1b0 net/core/dev.c:5579
>
> Therefore we make sure the state changes and data structures stay
> consistent at CAN frame reception time by adding a spin_lock in
> isotp_rcv(). This fixes the issue reported by syzkaller but does not
> affect real world operation.
>
> Link: https://lore.kernel.org/linux-can/d7e69278-d741-c706-65e1-e87623d9a8e8@huawei.com/T/
> Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
> Reported-by: syzbot+4c63f36709a642f801c5@syzkaller.appspotmail.com
> Reported-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Applied to linux-can/testing. Added stable on Cc.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-02-09 7:48 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-08 20:00 [PATCH] can: isotp: fix potential CAN frame reception race in isotp_rcv() Oliver Hartkopp
2022-02-09 7:48 ` Marc Kleine-Budde
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.