* [PATCH net v2 0/2] fix tx buffer concurrent access protection
@ 2021-10-09 7:40 Ziyang Xuan
2021-10-09 7:40 ` [PATCH net v2 1/2] can: isotp: add result check for wait_event_interruptible() Ziyang Xuan
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ziyang Xuan @ 2021-10-09 7:40 UTC (permalink / raw)
To: socketcan, mkl; +Cc: davem, kuba, linux-can, netdev, linux-kernel
Fix tx buffer concurrent access protection in isotp_sendmsg().
v2:
- Change state of struct tpcon to u32 for cmpxchg just support 4-byte
and 8-byte in some architectures.
Ziyang Xuan (2):
can: isotp: add result check for wait_event_interruptible()
can: isotp: fix tx buffer concurrent access in isotp_sendmsg()
net/can/isotp.c | 48 +++++++++++++++++++++++++++++++++---------------
1 file changed, 33 insertions(+), 15 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH net v2 1/2] can: isotp: add result check for wait_event_interruptible() 2021-10-09 7:40 [PATCH net v2 0/2] fix tx buffer concurrent access protection Ziyang Xuan @ 2021-10-09 7:40 ` Ziyang Xuan 2021-10-11 19:10 ` Oliver Hartkopp 2021-10-09 7:40 ` [PATCH net v2 2/2] can: isotp: fix tx buffer concurrent access in isotp_sendmsg() Ziyang Xuan 2021-10-17 12:22 ` [PATCH net v2 0/2] fix tx buffer concurrent access protection Marc Kleine-Budde 2 siblings, 1 reply; 6+ messages in thread From: Ziyang Xuan @ 2021-10-09 7:40 UTC (permalink / raw) To: socketcan, mkl; +Cc: davem, kuba, linux-can, netdev, linux-kernel Using wait_event_interruptible() to wait for complete transmission, but do not check the result of wait_event_interruptible() which can be interrupted. It will result in tx buffer has multiple accessers and the later process interferes with the previous process. Following is one of the problems reported by syzbot. ============================================================= WARNING: CPU: 0 PID: 0 at net/can/isotp.c:840 isotp_tx_timer_handler+0x2e0/0x4c0 CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc7+ #68 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1 04/01/2014 RIP: 0010:isotp_tx_timer_handler+0x2e0/0x4c0 Call Trace: <IRQ> ? isotp_setsockopt+0x390/0x390 __hrtimer_run_queues+0xb8/0x610 hrtimer_run_softirq+0x91/0xd0 ? rcu_read_lock_sched_held+0x4d/0x80 __do_softirq+0xe8/0x553 irq_exit_rcu+0xf8/0x100 sysvec_apic_timer_interrupt+0x9e/0xc0 </IRQ> asm_sysvec_apic_timer_interrupt+0x12/0x20 Add result check for wait_event_interruptible() in isotp_sendmsg() to avoid multiple accessers for tx buffer. Reported-by: syzbot+78bab6958a614b0c80b9@syzkaller.appspotmail.com Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- net/can/isotp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/net/can/isotp.c b/net/can/isotp.c index caaa532ece94..2ac29c2b2ca6 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -865,7 +865,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) return -EAGAIN; /* wait for complete transmission of current pdu */ - wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); + err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); + if (err) + return err; } if (!size || size > MAX_MSG_LENGTH) -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 1/2] can: isotp: add result check for wait_event_interruptible() 2021-10-09 7:40 ` [PATCH net v2 1/2] can: isotp: add result check for wait_event_interruptible() Ziyang Xuan @ 2021-10-11 19:10 ` Oliver Hartkopp 0 siblings, 0 replies; 6+ messages in thread From: Oliver Hartkopp @ 2021-10-11 19:10 UTC (permalink / raw) To: Ziyang Xuan, mkl; +Cc: davem, kuba, linux-can, netdev, linux-kernel On 09.10.21 09:40, Ziyang Xuan wrote: > Using wait_event_interruptible() to wait for complete transmission, > but do not check the result of wait_event_interruptible() which can > be interrupted. It will result in tx buffer has multiple accessers > and the later process interferes with the previous process. > > Following is one of the problems reported by syzbot. > > ============================================================= > WARNING: CPU: 0 PID: 0 at net/can/isotp.c:840 isotp_tx_timer_handler+0x2e0/0x4c0 > CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc7+ #68 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1 04/01/2014 > RIP: 0010:isotp_tx_timer_handler+0x2e0/0x4c0 > Call Trace: > <IRQ> > ? isotp_setsockopt+0x390/0x390 > __hrtimer_run_queues+0xb8/0x610 > hrtimer_run_softirq+0x91/0xd0 > ? rcu_read_lock_sched_held+0x4d/0x80 > __do_softirq+0xe8/0x553 > irq_exit_rcu+0xf8/0x100 > sysvec_apic_timer_interrupt+0x9e/0xc0 > </IRQ> > asm_sysvec_apic_timer_interrupt+0x12/0x20 > > Add result check for wait_event_interruptible() in isotp_sendmsg() > to avoid multiple accessers for tx buffer. > > Reported-by: syzbot+78bab6958a614b0c80b9@syzkaller.appspotmail.com > Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net> Many thanks! Oliver > --- > net/can/isotp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/can/isotp.c b/net/can/isotp.c > index caaa532ece94..2ac29c2b2ca6 100644 > --- a/net/can/isotp.c > +++ b/net/can/isotp.c > @@ -865,7 +865,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) > return -EAGAIN; > > /* wait for complete transmission of current pdu */ > - wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); > + err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); > + if (err) > + return err; > } > > if (!size || size > MAX_MSG_LENGTH) > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net v2 2/2] can: isotp: fix tx buffer concurrent access in isotp_sendmsg() 2021-10-09 7:40 [PATCH net v2 0/2] fix tx buffer concurrent access protection Ziyang Xuan 2021-10-09 7:40 ` [PATCH net v2 1/2] can: isotp: add result check for wait_event_interruptible() Ziyang Xuan @ 2021-10-09 7:40 ` Ziyang Xuan 2021-10-11 19:11 ` Oliver Hartkopp 2021-10-17 12:22 ` [PATCH net v2 0/2] fix tx buffer concurrent access protection Marc Kleine-Budde 2 siblings, 1 reply; 6+ messages in thread From: Ziyang Xuan @ 2021-10-09 7:40 UTC (permalink / raw) To: socketcan, mkl; +Cc: davem, kuba, linux-can, netdev, linux-kernel When isotp_sendmsg() concurrent, tx.state of all tx processes can be ISOTP_IDLE. The conditions so->tx.state != ISOTP_IDLE and wq_has_sleeper(&so->wait) can not protect tx buffer from being accessed by multiple tx processes. We can use cmpxchg() to try to modify tx.state to ISOTP_SENDING firstly. If the modification of the previous process succeed, the later process must wait tx.state to ISOTP_IDLE firstly. Thus, we can ensure tx buffer is accessed by only one process at the same time. And we should also restore the original tx.state at the subsequent error processes. Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- net/can/isotp.c | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/net/can/isotp.c b/net/can/isotp.c index 2ac29c2b2ca6..d1f54273c0bb 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -121,7 +121,7 @@ enum { struct tpcon { int idx; int len; - u8 state; + u32 state; u8 bs; u8 sn; u8 ll_dl; @@ -848,6 +848,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) { struct sock *sk = sock->sk; struct isotp_sock *so = isotp_sk(sk); + u32 old_state = so->tx.state; struct sk_buff *skb; struct net_device *dev; struct canfd_frame *cf; @@ -860,47 +861,55 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) return -EADDRNOTAVAIL; /* we do not support multiple buffers - for now */ - if (so->tx.state != ISOTP_IDLE || wq_has_sleeper(&so->wait)) { - if (msg->msg_flags & MSG_DONTWAIT) - return -EAGAIN; + if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE || + wq_has_sleeper(&so->wait)) { + if (msg->msg_flags & MSG_DONTWAIT) { + err = -EAGAIN; + goto err_out; + } /* wait for complete transmission of current pdu */ err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); if (err) - return err; + goto err_out; } - if (!size || size > MAX_MSG_LENGTH) - return -EINVAL; + if (!size || size > MAX_MSG_LENGTH) { + err = -EINVAL; + goto err_out; + } /* take care of a potential SF_DL ESC offset for TX_DL > 8 */ off = (so->tx.ll_dl > CAN_MAX_DLEN) ? 1 : 0; /* does the given data fit into a single frame for SF_BROADCAST? */ if ((so->opt.flags & CAN_ISOTP_SF_BROADCAST) && - (size > so->tx.ll_dl - SF_PCI_SZ4 - ae - off)) - return -EINVAL; + (size > so->tx.ll_dl - SF_PCI_SZ4 - ae - off)) { + err = -EINVAL; + goto err_out; + } err = memcpy_from_msg(so->tx.buf, msg, size); if (err < 0) - return err; + goto err_out; dev = dev_get_by_index(sock_net(sk), so->ifindex); - if (!dev) - return -ENXIO; + if (!dev) { + err = -ENXIO; + goto err_out; + } skb = sock_alloc_send_skb(sk, so->ll.mtu + sizeof(struct can_skb_priv), msg->msg_flags & MSG_DONTWAIT, &err); if (!skb) { dev_put(dev); - return err; + goto err_out; } can_skb_reserve(skb); can_skb_prv(skb)->ifindex = dev->ifindex; can_skb_prv(skb)->skbcnt = 0; - so->tx.state = ISOTP_SENDING; so->tx.len = size; so->tx.idx = 0; @@ -956,7 +965,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) if (err) { pr_notice_once("can-isotp: %s: can_send_ret %pe\n", __func__, ERR_PTR(err)); - return err; + goto err_out; } if (wait_tx_done) { @@ -965,6 +974,13 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) } return size; + +err_out: + so->tx.state = old_state; + if (so->tx.state == ISOTP_IDLE) + wake_up_interruptible(&so->wait); + + return err; } static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, size_t size, -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 2/2] can: isotp: fix tx buffer concurrent access in isotp_sendmsg() 2021-10-09 7:40 ` [PATCH net v2 2/2] can: isotp: fix tx buffer concurrent access in isotp_sendmsg() Ziyang Xuan @ 2021-10-11 19:11 ` Oliver Hartkopp 0 siblings, 0 replies; 6+ messages in thread From: Oliver Hartkopp @ 2021-10-11 19:11 UTC (permalink / raw) To: Ziyang Xuan, mkl; +Cc: davem, kuba, linux-can, netdev, linux-kernel On 09.10.21 09:40, Ziyang Xuan wrote: > When isotp_sendmsg() concurrent, tx.state of all tx processes can be > ISOTP_IDLE. The conditions so->tx.state != ISOTP_IDLE and > wq_has_sleeper(&so->wait) can not protect tx buffer from being accessed > by multiple tx processes. > > We can use cmpxchg() to try to modify tx.state to ISOTP_SENDING firstly. > If the modification of the previous process succeed, the later process > must wait tx.state to ISOTP_IDLE firstly. Thus, we can ensure tx buffer > is accessed by only one process at the same time. And we should also > restore the original tx.state at the subsequent error processes. > > Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net> Many thanks! Oliver > --- > net/can/isotp.c | 46 +++++++++++++++++++++++++++++++--------------- > 1 file changed, 31 insertions(+), 15 deletions(-) > > diff --git a/net/can/isotp.c b/net/can/isotp.c > index 2ac29c2b2ca6..d1f54273c0bb 100644 > --- a/net/can/isotp.c > +++ b/net/can/isotp.c > @@ -121,7 +121,7 @@ enum { > struct tpcon { > int idx; > int len; > - u8 state; > + u32 state; > u8 bs; > u8 sn; > u8 ll_dl; > @@ -848,6 +848,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) > { > struct sock *sk = sock->sk; > struct isotp_sock *so = isotp_sk(sk); > + u32 old_state = so->tx.state; > struct sk_buff *skb; > struct net_device *dev; > struct canfd_frame *cf; > @@ -860,47 +861,55 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) > return -EADDRNOTAVAIL; > > /* we do not support multiple buffers - for now */ > - if (so->tx.state != ISOTP_IDLE || wq_has_sleeper(&so->wait)) { > - if (msg->msg_flags & MSG_DONTWAIT) > - return -EAGAIN; > + if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE || > + wq_has_sleeper(&so->wait)) { > + if (msg->msg_flags & MSG_DONTWAIT) { > + err = -EAGAIN; > + goto err_out; > + } > > /* wait for complete transmission of current pdu */ > err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); > if (err) > - return err; > + goto err_out; > } > > - if (!size || size > MAX_MSG_LENGTH) > - return -EINVAL; > + if (!size || size > MAX_MSG_LENGTH) { > + err = -EINVAL; > + goto err_out; > + } > > /* take care of a potential SF_DL ESC offset for TX_DL > 8 */ > off = (so->tx.ll_dl > CAN_MAX_DLEN) ? 1 : 0; > > /* does the given data fit into a single frame for SF_BROADCAST? */ > if ((so->opt.flags & CAN_ISOTP_SF_BROADCAST) && > - (size > so->tx.ll_dl - SF_PCI_SZ4 - ae - off)) > - return -EINVAL; > + (size > so->tx.ll_dl - SF_PCI_SZ4 - ae - off)) { > + err = -EINVAL; > + goto err_out; > + } > > err = memcpy_from_msg(so->tx.buf, msg, size); > if (err < 0) > - return err; > + goto err_out; > > dev = dev_get_by_index(sock_net(sk), so->ifindex); > - if (!dev) > - return -ENXIO; > + if (!dev) { > + err = -ENXIO; > + goto err_out; > + } > > skb = sock_alloc_send_skb(sk, so->ll.mtu + sizeof(struct can_skb_priv), > msg->msg_flags & MSG_DONTWAIT, &err); > if (!skb) { > dev_put(dev); > - return err; > + goto err_out; > } > > can_skb_reserve(skb); > can_skb_prv(skb)->ifindex = dev->ifindex; > can_skb_prv(skb)->skbcnt = 0; > > - so->tx.state = ISOTP_SENDING; > so->tx.len = size; > so->tx.idx = 0; > > @@ -956,7 +965,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) > if (err) { > pr_notice_once("can-isotp: %s: can_send_ret %pe\n", > __func__, ERR_PTR(err)); > - return err; > + goto err_out; > } > > if (wait_tx_done) { > @@ -965,6 +974,13 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) > } > > return size; > + > +err_out: > + so->tx.state = old_state; > + if (so->tx.state == ISOTP_IDLE) > + wake_up_interruptible(&so->wait); > + > + return err; > } > > static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, size_t size, > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 0/2] fix tx buffer concurrent access protection 2021-10-09 7:40 [PATCH net v2 0/2] fix tx buffer concurrent access protection Ziyang Xuan 2021-10-09 7:40 ` [PATCH net v2 1/2] can: isotp: add result check for wait_event_interruptible() Ziyang Xuan 2021-10-09 7:40 ` [PATCH net v2 2/2] can: isotp: fix tx buffer concurrent access in isotp_sendmsg() Ziyang Xuan @ 2021-10-17 12:22 ` Marc Kleine-Budde 2 siblings, 0 replies; 6+ messages in thread From: Marc Kleine-Budde @ 2021-10-17 12:22 UTC (permalink / raw) To: Ziyang Xuan; +Cc: socketcan, davem, kuba, linux-can, netdev, linux-kernel [-- Attachment #1: Type: text/plain, Size: 733 bytes --] On 09.10.2021 15:40:08, Ziyang Xuan wrote: > Fix tx buffer concurrent access protection in isotp_sendmsg(). > > v2: > - Change state of struct tpcon to u32 for cmpxchg just support 4-byte > and 8-byte in some architectures. > > Ziyang Xuan (2): > can: isotp: add result check for wait_event_interruptible() > can: isotp: fix tx buffer concurrent access in isotp_sendmsg() Applied to linux-can/testing, added stable on Cc. Thanks, 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] 6+ messages in thread
end of thread, other threads:[~2021-10-17 12:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-09 7:40 [PATCH net v2 0/2] fix tx buffer concurrent access protection Ziyang Xuan 2021-10-09 7:40 ` [PATCH net v2 1/2] can: isotp: add result check for wait_event_interruptible() Ziyang Xuan 2021-10-11 19:10 ` Oliver Hartkopp 2021-10-09 7:40 ` [PATCH net v2 2/2] can: isotp: fix tx buffer concurrent access in isotp_sendmsg() Ziyang Xuan 2021-10-11 19:11 ` Oliver Hartkopp 2021-10-17 12:22 ` [PATCH net v2 0/2] fix tx buffer concurrent access protection 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.