FAILED: patch "[PATCH] can: isotp: isotp_sendmsg(): fix TX buffer concurrent access" failed to apply to 5.10-stable tree

FAILED: patch "[PATCH] can: isotp: isotp_sendmsg(): fix TX buffer concurrent access" failed to apply to 5.10-stable tree

[gregkh]

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,

Re: FAILED: patch "[PATCH] can: isotp: isotp_sendmsg(): fix TX buffer concurrent access" failed to apply to 5.10-stable tree

[Oliver Hartkopp]

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,
> 

Re: FAILED: patch "[PATCH] can: isotp: isotp_sendmsg(): fix TX buffer concurrent access" failed to apply to 5.10-stable tree

[Greg KH]

On Sun, Oct 24, 2021 at 03:14:50PM +0200, Oliver Hartkopp wrote:
> 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()

Now queued up, thanks.

greg k-h