[RFC][PATCH v2] can: isotp: fix CAN frame reception race in isotp_rcv()

[RFC][PATCH v2] can: isotp: fix CAN frame reception race in isotp_rcv()

[Oliver Hartkopp]

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, 14 insertions(+)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 02cbcb2ecf0d..b5ba1a9a9e3b 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;
 };
 
 static LIST_HEAD(isotp_notifier_list);
 static DEFINE_SPINLOCK(isotp_notifier_lock);
 static struct isotp_sock *isotp_busy_notifier;
@@ -613,10 +615,19 @@ 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.
+	 *
+	 * To not lock up the softirq just drop the frame in syzcaller case.
+	 */
+	if (!spin_trylock(&so->rx_lock))
+		return;
+
 	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;
@@ -666,10 +677,12 @@ 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;
 	}
+
+	spin_unlock(&so->rx_lock);
 }
 
 static void isotp_fill_dataframe(struct canfd_frame *cf, struct isotp_sock *so,
 				 int ae, int off)
 {
@@ -1442,10 +1455,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

Re: [RFC][PATCH v2] can: isotp: fix CAN frame reception race in isotp_rcv()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4872 bytes --]

On 28.01.2022 08:43:27, 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>
> ---
>  net/can/isotp.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 02cbcb2ecf0d..b5ba1a9a9e3b 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;

I think checkpatch wants to have a comment describing the lock.

>  };
>  
>  static LIST_HEAD(isotp_notifier_list);
>  static DEFINE_SPINLOCK(isotp_notifier_lock);
>  static struct isotp_sock *isotp_busy_notifier;
> @@ -613,10 +615,19 @@ 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.
> +	 *
> +	 * To not lock up the softirq just drop the frame in syzcaller case.
> +	 */
> +	if (!spin_trylock(&so->rx_lock))
> +		return;
> +
>  	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;

Maybe there are more returns, which are not shown in the context of this
patch.

> @@ -666,10 +677,12 @@ 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 +1455,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);

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

Re: [RFC][PATCH v2] can: isotp: fix CAN frame reception race in isotp_rcv()

[Oliver Hartkopp]

On 28.01.22 08:56, Marc Kleine-Budde wrote:
> On 28.01.2022 08:43:27, 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>
>> ---
>>   net/can/isotp.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index 02cbcb2ecf0d..b5ba1a9a9e3b 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;
> 
> I think checkpatch wants to have a comment describing the lock.

Ok.
> 
>>   };
>>   
>>   static LIST_HEAD(isotp_notifier_list);
>>   static DEFINE_SPINLOCK(isotp_notifier_lock);
>>   static struct isotp_sock *isotp_busy_notifier;
>> @@ -613,10 +615,19 @@ 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.
>> +	 *
>> +	 * To not lock up the softirq just drop the frame in syzcaller case.
>> +	 */
>> +	if (!spin_trylock(&so->rx_lock))
>> +		return;
>> +
>>   	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;
> 
> Maybe there are more returns, which are not shown in the context of this
> patch.
> 

Oh, yes! Thanks!

Will send a V3 soon.

>> @@ -666,10 +677,12 @@ 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 +1455,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);
> 
> regards,
> Marc
>