* Re: FAILED: patch "[PATCH] can: isotp: isotp_sendmsg(): fix TX buffer concurrent access" failed to apply to 5.10-stable tree
[not found] <16350748387398@kroah.com>
@ 2021-10-24 13:14 ` Oliver Hartkopp
2021-10-25 7:47 ` Greg KH
0 siblings, 1 reply; 2+ messages in thread
From: Oliver Hartkopp @ 2021-10-24 13:14 UTC (permalink / raw)
To: gregkh, william.xuanziyang, mkl; +Cc: stable, linux-can
Hello Greg,
Linux 5.10 is missing the SF_BROADCAST support which was introduced in
5.11 - that broke the application of the original patch.
Just sent out a patch for 5.10-stable some minutes ago:
[PATCH 5.10-stable] can: isotp: isotp_sendmsg(): fix TX buffer
concurrent access in isotp_sendmsg()
Thanks for your work!
Best regards,
Oliver
On 24.10.21 13:27, gregkh@linuxfoundation.org wrote:
>
> The patch below does not apply to the 5.10-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> From 43a08c3bdac4cb42eff8fe5e2278bffe0c5c3daa Mon Sep 17 00:00:00 2001
> From: Ziyang Xuan <william.xuanziyang@huawei.com>
> Date: Sat, 9 Oct 2021 15:40:30 +0800
> Subject: [PATCH] can: isotp: isotp_sendmsg(): fix TX buffer concurrent access
> in isotp_sendmsg()
>
> 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")
> Link: https://lore.kernel.org/all/c2517874fbdf4188585cf9ddf67a8fa74d5dbde5.1633764159.git.william.xuanziyang@huawei.com
> Cc: stable@vger.kernel.org
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>
> 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] 2+ messages in thread