[PATCH] Add TX limit for SocketCAN.

[PATCH] Add TX limit for SocketCAN.

[zhongjie.shi]

From: "Shi, Zhongjie" <zhongjie.shi@intel.com>

This will be used to prevent malicious or unintentional
flooding of messages via SocketCAN.

Change-Id: I6c2e122e12a594d9ad7dc4bdeebe15a1929eb893
Tracked-On: https://jira01.devtools.intel.com/browse/OAM-48496
Signed-off-by: Shi, Zhongjie <zhongjie.shi@intel.com>
---
 net/can/Kconfig  | 13 +++++++++++++
 net/can/af_can.c | 15 +++++++++++++++
 net/can/af_can.h |  5 +++++
 net/can/proc.c   | 13 +++++++++++++
 4 files changed, 46 insertions(+)

diff --git a/net/can/Kconfig b/net/can/Kconfig
index a15c0e0..aeba3d0 100644
--- a/net/can/Kconfig
+++ b/net/can/Kconfig
@@ -51,6 +51,19 @@ config CAN_GW
 	  They can be modified with AND/OR/XOR/SET operations as configured
 	  by the netlink configuration interface known e.g. from iptables.
 
+config CAN_TX_ATTEMPT_RATE_LIMIT
+	int "Tx attempt rate limit"
+	default "5"
+	---help---
+	  This TX attempt rate limit can be used to prevent flood of messages
+	  from user space. It's TX "attempt" rate here instead of the "actual"
+	  TX rate because we do the statistics for the TX attempt no matter if
+	  the message TX is actually sent successful or not. The current
+	  default value is "5" which means 5 messages per second that is proper
+	  for an in-vehicle infotainment (IVI) system. For the use cases other
+	  than IVI, this can be set to the value corresponding to the specific
+	  requirement.
+
 source "drivers/net/can/Kconfig"
 
 endif
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 88edac0..4981c15 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -83,6 +83,9 @@ static DEFINE_MUTEX(proto_tab_lock);
 
 static atomic_t skbcounter = ATOMIC_INIT(0);
 
+#define SOCK_TX_ATTEMPT_RATE_LIMIT    (CONFIG_CAN_TX_ATTEMPT_RATE_LIMIT)
+
+
 /*
  * af_can socket functions
  */
@@ -212,6 +215,7 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
  *  -EPERM when trying to send on a non-CAN interface
  *  -EMSGSIZE CAN frame size is bigger than CAN interface MTU
  *  -EINVAL when the skb->data does not contain a valid CAN frame
+ *  -EDQUOT when current tx rate reach its limit
  */
 int can_send(struct sk_buff *skb, int loop)
 {
@@ -251,6 +255,17 @@ int can_send(struct sk_buff *skb, int loop)
 		goto inval_skb;
 	}
 
+	/* update statistics */
+	can_stats->tx_attempt_frames++;
+	can_stats->tx_frames_attempt_delta++;
+
+	/* throttle if needed */
+	if (can_stats->current_tx_attempt_rate > SOCK_TX_ATTEMPT_RATE_LIMIT) {
+		printk_ratelimited(KERN_WARNING "can: inhibit tx due to "
+			"attempt rate: %lu\n", can_stats->current_tx_attempt_rate);
+		return EDQUOT;
+	}
+
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	skb_reset_mac_header(skb);
diff --git a/net/can/af_can.h b/net/can/af_can.h
index d0ef45b..6f50e82 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -83,22 +83,27 @@ struct s_stats {
 
 	unsigned long rx_frames;
 	unsigned long tx_frames;
+	unsigned long tx_attempt_frames;
 	unsigned long matches;
 
 	unsigned long total_rx_rate;
 	unsigned long total_tx_rate;
+	unsigned long total_tx_attempt_rate;
 	unsigned long total_rx_match_ratio;
 
 	unsigned long current_rx_rate;
 	unsigned long current_tx_rate;
+	unsigned long current_tx_attempt_rate;
 	unsigned long current_rx_match_ratio;
 
 	unsigned long max_rx_rate;
 	unsigned long max_tx_rate;
+	unsigned long max_tx_attempt_rate;
 	unsigned long max_rx_match_ratio;
 
 	unsigned long rx_frames_delta;
 	unsigned long tx_frames_delta;
+	unsigned long tx_frames_attempt_delta;
 	unsigned long matches_delta;
 };
 
diff --git a/net/can/proc.c b/net/can/proc.c
index 83045f0..96eb745 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -148,6 +148,8 @@ void can_stat_update(unsigned long data)
 
 	can_stats->total_tx_rate = calc_rate(can_stats->jiffies_init, j,
 					    can_stats->tx_frames);
+	can_stats->total_tx_attempt_rate = calc_rate(can_stats->jiffies_init, j,
+					    can_stats->tx_attempt_frames);
 	can_stats->total_rx_rate = calc_rate(can_stats->jiffies_init, j,
 					    can_stats->rx_frames);
 
@@ -158,12 +160,16 @@ void can_stat_update(unsigned long data)
 			can_stats->rx_frames_delta;
 
 	can_stats->current_tx_rate = calc_rate(0, HZ, can_stats->tx_frames_delta);
+	can_stats->current_tx_attempt_rate = calc_rate(0, HZ, can_stats->tx_frames_attempt_delta);
 	can_stats->current_rx_rate = calc_rate(0, HZ, can_stats->rx_frames_delta);
 
 	/* check / update maximum values */
 	if (can_stats->max_tx_rate < can_stats->current_tx_rate)
 		can_stats->max_tx_rate = can_stats->current_tx_rate;
 
+	if (can_stats->max_tx_attempt_rate < can_stats->current_tx_attempt_rate)
+		can_stats->max_tx_attempt_rate = can_stats->current_tx_attempt_rate;
+
 	if (can_stats->max_rx_rate < can_stats->current_rx_rate)
 		can_stats->max_rx_rate = can_stats->current_rx_rate;
 
@@ -172,6 +178,7 @@ void can_stat_update(unsigned long data)
 
 	/* clear values for 'current rate' calculation */
 	can_stats->tx_frames_delta = 0;
+	can_stats->tx_frames_attempt_delta = 0;
 	can_stats->rx_frames_delta = 0;
 	can_stats->matches_delta   = 0;
 
@@ -227,6 +234,8 @@ static int can_stats_proc_show(struct seq_file *m, void *v)
 
 		seq_printf(m, " %8ld frames/s total tx rate (TXR)\n",
 				can_stats->total_tx_rate);
+		seq_printf(m, " %8ld frames/s total attempt tx rate (ATXR)\n",
+				can_stats->total_tx_attempt_rate);
 		seq_printf(m, " %8ld frames/s total rx rate (RXR)\n",
 				can_stats->total_rx_rate);
 
@@ -237,6 +246,8 @@ static int can_stats_proc_show(struct seq_file *m, void *v)
 
 		seq_printf(m, " %8ld frames/s current tx rate (CTXR)\n",
 				can_stats->current_tx_rate);
+		seq_printf(m, " %8ld frames/s current attempt tx rate (CATXR)\n",
+				can_stats->current_tx_attempt_rate);
 		seq_printf(m, " %8ld frames/s current rx rate (CRXR)\n",
 				can_stats->current_rx_rate);
 
@@ -247,6 +258,8 @@ static int can_stats_proc_show(struct seq_file *m, void *v)
 
 		seq_printf(m, " %8ld frames/s max tx rate (MTXR)\n",
 				can_stats->max_tx_rate);
+		seq_printf(m, " %8ld frames/s max attempt tx rate (MATXR)\n",
+				can_stats->max_tx_attempt_rate);
 		seq_printf(m, " %8ld frames/s max rx rate (MRXR)\n",
 				can_stats->max_rx_rate);
 
-- 
2.7.4

Re: [PATCH] Add TX limit for SocketCAN.

[Austin Schuh]

On Thu, Aug 31, 2017 at 7:14 PM,  <zhongjie.shi@intel.com> wrote:
> From: "Shi, Zhongjie" <zhongjie.shi@intel.com>
>
> This will be used to prevent malicious or unintentional
> flooding of messages via SocketCAN.
>
> Change-Id: I6c2e122e12a594d9ad7dc4bdeebe15a1929eb893
> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-48496
> Signed-off-by: Shi, Zhongjie <zhongjie.shi@intel.com>
> ---
>  net/can/Kconfig  | 13 +++++++++++++
>  net/can/af_can.c | 15 +++++++++++++++
>  net/can/af_can.h |  5 +++++
>  net/can/proc.c   | 13 +++++++++++++
>  4 files changed, 46 insertions(+)
>
> diff --git a/net/can/Kconfig b/net/can/Kconfig
> index a15c0e0..aeba3d0 100644
> --- a/net/can/Kconfig
> +++ b/net/can/Kconfig
> @@ -51,6 +51,19 @@ config CAN_GW
>           They can be modified with AND/OR/XOR/SET operations as configured
>           by the netlink configuration interface known e.g. from iptables.
>
> +config CAN_TX_ATTEMPT_RATE_LIMIT
> +       int "Tx attempt rate limit"
> +       default "5"
> +       ---help---
> +         This TX attempt rate limit can be used to prevent flood of messages
> +         from user space. It's TX "attempt" rate here instead of the "actual"
> +         TX rate because we do the statistics for the TX attempt no matter if
> +         the message TX is actually sent successful or not. The current
> +         default value is "5" which means 5 messages per second that is proper
> +         for an in-vehicle infotainment (IVI) system. For the use cases other
> +         than IVI, this can be set to the value corresponding to the specific
> +         requirement.
> +
>  source "drivers/net/can/Kconfig"
>
>  endif

This default breaks our systems.  We send hundreds of messages/sec,
and at worst case when pushing an update out over CAN to other ECUs,
saturate the bus.  This should be opt in.

I have vague recollections from Oliver long ago that you can use
queueing disciplines to rate limit the interface, which should
implement this without any kernel changes.

Austin

RE: [PATCH] Add TX limit for SocketCAN.

[Shi, Zhongjie]

Hi Austin,

It's appreciated for your review and let me know your use cases!
Just a quick thought, for your case, is it OK to set the TX to some big value, eg. Some value as 10 * 1024 * 1024?
Please point out if anything I missed in your comment/point.

It will be also appreciated if you or Oliver can share with me the details of how to "use queuing discipline" to achieve this.

Thanks!

Zhongjie


-----Original Message-----
From: Austin Schuh [mailto:austin@peloton-tech.com] 
Sent: Friday, September 01, 2017 11:02 AM
To: Shi, Zhongjie <zhongjie.shi@intel.com>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>; Marc Kleine-Budde <mkl@pengutronix.de>; linux-can <linux-can@vger.kernel.org>
Subject: Re: [PATCH] Add TX limit for SocketCAN.

On Thu, Aug 31, 2017 at 7:14 PM,  <zhongjie.shi@intel.com> wrote:
> From: "Shi, Zhongjie" <zhongjie.shi@intel.com>
>
> This will be used to prevent malicious or unintentional flooding of 
> messages via SocketCAN.
>
> Change-Id: I6c2e122e12a594d9ad7dc4bdeebe15a1929eb893
> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-48496
> Signed-off-by: Shi, Zhongjie <zhongjie.shi@intel.com>
> ---
>  net/can/Kconfig  | 13 +++++++++++++
>  net/can/af_can.c | 15 +++++++++++++++  net/can/af_can.h |  5 +++++
>  net/can/proc.c   | 13 +++++++++++++
>  4 files changed, 46 insertions(+)
>
> diff --git a/net/can/Kconfig b/net/can/Kconfig index a15c0e0..aeba3d0 
> 100644
> --- a/net/can/Kconfig
> +++ b/net/can/Kconfig
> @@ -51,6 +51,19 @@ config CAN_GW
>           They can be modified with AND/OR/XOR/SET operations as configured
>           by the netlink configuration interface known e.g. from iptables.
>
> +config CAN_TX_ATTEMPT_RATE_LIMIT
> +       int "Tx attempt rate limit"
> +       default "5"
> +       ---help---
> +         This TX attempt rate limit can be used to prevent flood of messages
> +         from user space. It's TX "attempt" rate here instead of the "actual"
> +         TX rate because we do the statistics for the TX attempt no matter if
> +         the message TX is actually sent successful or not. The current
> +         default value is "5" which means 5 messages per second that is proper
> +         for an in-vehicle infotainment (IVI) system. For the use cases other
> +         than IVI, this can be set to the value corresponding to the specific
> +         requirement.
> +
>  source "drivers/net/can/Kconfig"
>
>  endif

This default breaks our systems.  We send hundreds of messages/sec, and at worst case when pushing an update out over CAN to other ECUs, saturate the bus.  This should be opt in.

I have vague recollections from Oliver long ago that you can use queueing disciplines to rate limit the interface, which should implement this without any kernel changes.

Austin

Re: [PATCH] Add TX limit for SocketCAN.

[Justin Delegard]

This strikes me as something that should be configured via a sysctl,
not a kernel config option.

Isn't CAN itself robust against such a problem?  Low priority senders
should be clobbered by higher priority ones.

5 packets per second is also extremely slow.  Even on slow buses (eg
250kbps in J1939) the bitrate supports thousands of packets per
second.

What are you trying to protect with this change?  There's no guarantee
any other device on the bus will respect this, and an attacker using
linux could just revert this change.


Justin

On Thu, Aug 31, 2017 at 8:22 PM, Shi, Zhongjie <zhongjie.shi@intel.com> wrote:
> Hi Austin,
>
> It's appreciated for your review and let me know your use cases!
> Just a quick thought, for your case, is it OK to set the TX to some big value, eg. Some value as 10 * 1024 * 1024?
> Please point out if anything I missed in your comment/point.
>
> It will be also appreciated if you or Oliver can share with me the details of how to "use queuing discipline" to achieve this.
>
> Thanks!
>
> Zhongjie
>
>
> -----Original Message-----
> From: Austin Schuh [mailto:austin@peloton-tech.com]
> Sent: Friday, September 01, 2017 11:02 AM
> To: Shi, Zhongjie <zhongjie.shi@intel.com>
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>; Marc Kleine-Budde <mkl@pengutronix.de>; linux-can <linux-can@vger.kernel.org>
> Subject: Re: [PATCH] Add TX limit for SocketCAN.
>
> On Thu, Aug 31, 2017 at 7:14 PM,  <zhongjie.shi@intel.com> wrote:
>> From: "Shi, Zhongjie" <zhongjie.shi@intel.com>
>>
>> This will be used to prevent malicious or unintentional flooding of
>> messages via SocketCAN.
>>
>> Change-Id: I6c2e122e12a594d9ad7dc4bdeebe15a1929eb893
>> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-48496
>> Signed-off-by: Shi, Zhongjie <zhongjie.shi@intel.com>
>> ---
>>  net/can/Kconfig  | 13 +++++++++++++
>>  net/can/af_can.c | 15 +++++++++++++++  net/can/af_can.h |  5 +++++
>>  net/can/proc.c   | 13 +++++++++++++
>>  4 files changed, 46 insertions(+)
>>
>> diff --git a/net/can/Kconfig b/net/can/Kconfig index a15c0e0..aeba3d0
>> 100644
>> --- a/net/can/Kconfig
>> +++ b/net/can/Kconfig
>> @@ -51,6 +51,19 @@ config CAN_GW
>>           They can be modified with AND/OR/XOR/SET operations as configured
>>           by the netlink configuration interface known e.g. from iptables.
>>
>> +config CAN_TX_ATTEMPT_RATE_LIMIT
>> +       int "Tx attempt rate limit"
>> +       default "5"
>> +       ---help---
>> +         This TX attempt rate limit can be used to prevent flood of messages
>> +         from user space. It's TX "attempt" rate here instead of the "actual"
>> +         TX rate because we do the statistics for the TX attempt no matter if
>> +         the message TX is actually sent successful or not. The current
>> +         default value is "5" which means 5 messages per second that is proper
>> +         for an in-vehicle infotainment (IVI) system. For the use cases other
>> +         than IVI, this can be set to the value corresponding to the specific
>> +         requirement.
>> +
>>  source "drivers/net/can/Kconfig"
>>
>>  endif
>
> This default breaks our systems.  We send hundreds of messages/sec, and at worst case when pushing an update out over CAN to other ECUs, saturate the bus.  This should be opt in.
>
> I have vague recollections from Oliver long ago that you can use queueing disciplines to rate limit the interface, which should implement this without any kernel changes.
>
> Austin



-- 
Justin

RE: [PATCH] Add TX limit for SocketCAN.

[Shi, Zhongjie]

Hi Justin,

Sorry for my late response. 
First, I need to clarify that this limit is just for user space programs.
And please let me try to clarify the requirement in more details on our side:

(1) The change will be used for some IVI product.
(2) In IVI, there are hundreds of use space processes running which has network access which has more risk to be compromised.
(3) And for the IVI system, the TX rate isn't (or shouldn't be) that high as you mentioned. Only possible trigger of TX is from human: the user of IVI.
Please consider #2 and #3, once there are some malicious/buggy program send CAN messages frequently, then it will be harmful to the whole CAN network.

And by this logic in my patch, I'm trying to just to prevent the abnormal TX rate from user space on IVI product in the worst case: either some bug in the user space programs; or some compromised program by some hackers with certain zero day attack.

For the ECUs on the CAN bus, I'm not sure why is it not possible to set different config for their case.
I mean, e.g. 
(a) On the bus, for IVI, I can use it as 5/s,
(b) For the cases mentioned by Austin, the config could be 10M for their saturation work.
It's just configurable for every nodes on the bus.

Please point out if any fallacies I made in my comments above.

Thanks!

Zhongjie


-----Original Message-----
From: Justin Delegard [mailto:justin@samsara.com] 
Sent: Friday, September 01, 2017 12:18 PM
To: Shi, Zhongjie <zhongjie.shi@intel.com>
Cc: Austin Schuh <austin@peloton-tech.com>; Oliver Hartkopp <socketcan@hartkopp.net>; Marc Kleine-Budde <mkl@pengutronix.de>; linux-can <linux-can@vger.kernel.org>
Subject: Re: [PATCH] Add TX limit for SocketCAN.

This strikes me as something that should be configured via a sysctl, not a kernel config option.

Isn't CAN itself robust against such a problem?  Low priority senders should be clobbered by higher priority ones.

5 packets per second is also extremely slow.  Even on slow buses (eg 250kbps in J1939) the bitrate supports thousands of packets per second.

What are you trying to protect with this change?  There's no guarantee any other device on the bus will respect this, and an attacker using linux could just revert this change.


Justin

On Thu, Aug 31, 2017 at 8:22 PM, Shi, Zhongjie <zhongjie.shi@intel.com> wrote:
> Hi Austin,
>
> It's appreciated for your review and let me know your use cases!
> Just a quick thought, for your case, is it OK to set the TX to some big value, eg. Some value as 10 * 1024 * 1024?
> Please point out if anything I missed in your comment/point.
>
> It will be also appreciated if you or Oliver can share with me the details of how to "use queuing discipline" to achieve this.
>
> Thanks!
>
> Zhongjie
>
>
> -----Original Message-----
> From: Austin Schuh [mailto:austin@peloton-tech.com]
> Sent: Friday, September 01, 2017 11:02 AM
> To: Shi, Zhongjie <zhongjie.shi@intel.com>
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>; Marc Kleine-Budde 
> <mkl@pengutronix.de>; linux-can <linux-can@vger.kernel.org>
> Subject: Re: [PATCH] Add TX limit for SocketCAN.
>
> On Thu, Aug 31, 2017 at 7:14 PM,  <zhongjie.shi@intel.com> wrote:
>> From: "Shi, Zhongjie" <zhongjie.shi@intel.com>
>>
>> This will be used to prevent malicious or unintentional flooding of 
>> messages via SocketCAN.
>>
>> Change-Id: I6c2e122e12a594d9ad7dc4bdeebe15a1929eb893
>> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-48496
>> Signed-off-by: Shi, Zhongjie <zhongjie.shi@intel.com>
>> ---
>>  net/can/Kconfig  | 13 +++++++++++++
>>  net/can/af_can.c | 15 +++++++++++++++  net/can/af_can.h |  5 +++++
>>  net/can/proc.c   | 13 +++++++++++++
>>  4 files changed, 46 insertions(+)
>>
>> diff --git a/net/can/Kconfig b/net/can/Kconfig index a15c0e0..aeba3d0
>> 100644
>> --- a/net/can/Kconfig
>> +++ b/net/can/Kconfig
>> @@ -51,6 +51,19 @@ config CAN_GW
>>           They can be modified with AND/OR/XOR/SET operations as configured
>>           by the netlink configuration interface known e.g. from iptables.
>>
>> +config CAN_TX_ATTEMPT_RATE_LIMIT
>> +       int "Tx attempt rate limit"
>> +       default "5"
>> +       ---help---
>> +         This TX attempt rate limit can be used to prevent flood of messages
>> +         from user space. It's TX "attempt" rate here instead of the "actual"
>> +         TX rate because we do the statistics for the TX attempt no matter if
>> +         the message TX is actually sent successful or not. The current
>> +         default value is "5" which means 5 messages per second that is proper
>> +         for an in-vehicle infotainment (IVI) system. For the use cases other
>> +         than IVI, this can be set to the value corresponding to the specific
>> +         requirement.
>> +
>>  source "drivers/net/can/Kconfig"
>>
>>  endif
>
> This default breaks our systems.  We send hundreds of messages/sec, and at worst case when pushing an update out over CAN to other ECUs, saturate the bus.  This should be opt in.
>
> I have vague recollections from Oliver long ago that you can use queueing disciplines to rate limit the interface, which should implement this without any kernel changes.
>
> Austin



--
Justin

Re: [PATCH] Add TX limit for SocketCAN.

[Kurt Van Dijck]

Hey,

I don't like the approach either.
See my comments below.

> Hi Justin,
> 
> Sorry for my late response. 
> First, I need to clarify that this limit is just for user space programs.
> And please let me try to clarify the requirement in more details on our side:
> 
> (1) The change will be used for some IVI product.

That's a choice, not a justification for the patch.

> (2) In IVI, there are hundreds of use space processes running which has network access which has more risk to be compromised.
> (3) And for the IVI system, the TX rate isn't (or shouldn't be) that high as you mentioned. Only possible trigger of TX is from human: the user of IVI.
> Please consider #2 and #3, once there are some malicious/buggy program send CAN messages frequently, then it will be harmful to the whole CAN network.

Do you consider your IVI product having this patch as a safe ECU then?
It's a very rough method, since packets are considered good or bad based
on the number of recent packets.
That's like protecting your email server by rate-limiting ethernet packets.
It doesn't protect against spam, does it? It's just a rate limiter.

> 
> And by this logic in my patch, I'm trying to just to prevent the abnormal TX rate from user space on IVI product in the worst case:
> either some bug in the user space programs;
> or some compromised program by some hackers with certain zero day attack.

It's up to userspace to decide when a tx rate is abnormal.

> 
> For the ECUs on the CAN bus, I'm not sure why is it not possible to set different config for their case.
> I mean, e.g. 
> (a) On the bus, for IVI, I can use it as 5/s,
> (b) For the cases mentioned by Austin, the config could be 10M for their saturation work.
> It's just configurable for every nodes on the bus.

I looks like you set up a simple 'firewall' to CAN, with 1 hardcoded
rule in kernel code.

Kind regards,
Kurt

RE: [PATCH] Add TX limit for SocketCAN.

[Shi, Zhongjie]

Hi Kurt,

It's appreciated to get your suggestions on this.

In short, I would say that this is a simple (or as you said rough) protection in the layer of kernel.
It intentionally to make it configurable only at build time and in the kernel rather configurable in the user space at run/build time as it's just for that kernel layer.

There would be multi layers protection: in the user space, in the kernel running on the application processor, in the ECU or some IO controller, in some hardware settings, etc. This could be part of the whole protection.

Besides, some comments inline with your previous email.
Again, it would be appreciated if some of you experts can share with the details of using queueing discipline to achieve this.

Thanks!


Zhongjie


-----Original Message-----
From: Kurt Van Dijck [mailto:dev.kurt@vandijck-laurijssen.be] 
Sent: Friday, September 01, 2017 3:53 PM
To: Shi, Zhongjie <zhongjie.shi@intel.com>
Cc: Justin Delegard <justin@samsara.com>; Austin Schuh <austin@peloton-tech.com>; Oliver Hartkopp <socketcan@hartkopp.net>; Marc Kleine-Budde <mkl@pengutronix.de>; linux-can <linux-can@vger.kernel.org>
Subject: Re: [PATCH] Add TX limit for SocketCAN.

Hey,

I don't like the approach either.
See my comments below.

> Hi Justin,
> 
> Sorry for my late response. 
> First, I need to clarify that this limit is just for user space programs.
> And please let me try to clarify the requirement in more details on our side:
> 
> (1) The change will be used for some IVI product.

That's a choice, not a justification for the patch. 
>>> Zhongjie: I'm sorry that I didn't mean to make it as justification. I put it here as a fact/context of this change.

> (2) In IVI, there are hundreds of use space processes running which has network access which has more risk to be compromised.
> (3) And for the IVI system, the TX rate isn't (or shouldn't be) that high as you mentioned. Only possible trigger of TX is from human: the user of IVI.
> Please consider #2 and #3, once there are some malicious/buggy program send CAN messages frequently, then it will be harmful to the whole CAN network.

Do you consider your IVI product having this patch as a safe ECU then?
>>> Zhongjie: No. It can only part of the whole protection. As I mentioned, we can have protection in multiple layer.
It's a very rough method, since packets are considered good or bad based on the number of recent packets.
That's like protecting your email server by rate-limiting ethernet packets.
It doesn't protect against spam, does it? It's just a rate limiter.
>>> Zhongjie: I agree it's rough/simple. To make it to identify the Good/Bad things, there will be more work to be done. I tried add some more complicated statistics in the /proc/net/can/stats and launch some user space watchdog to do the audit. I may share with that sometime later. It's doable that way but much more complicated. With this change, I just want to make it work as simple as possible while fulfill our requirement. The limit to the rate just works in the layer of kernel to do the protection against the worst case of denial of service attack.

> 
> And by this logic in my patch, I'm trying to just to prevent the abnormal TX rate from user space on IVI product in the worst case:
> either some bug in the user space programs; or some compromised 
> program by some hackers with certain zero day attack.

It's up to userspace to decide when a tx rate is abnormal.
>>> Zhongjie: Again, I would stress that this is another layer of protection in the kernel. We can have different layers of protection which include the user spaces and the kernel.

> 
> For the ECUs on the CAN bus, I'm not sure why is it not possible to set different config for their case.
> I mean, e.g. 
> (a) On the bus, for IVI, I can use it as 5/s,
> (b) For the cases mentioned by Austin, the config could be 10M for their saturation work.
> It's just configurable for every nodes on the bus.

I looks like you set up a simple 'firewall' to CAN, with 1 hardcoded rule in kernel code.
>>> Zhongjie: Yes :D. I would say implemented in a few line code changes with 1 configurable parameter.


Kind regards,
Kurt

Re: [PATCH] Add TX limit for SocketCAN.

[Kurt Van Dijck]

Hey,

Just 1 remark left ...

...
> > (2) In IVI, there are hundreds of use space processes running which has network access which has more risk to be compromised.
> > (3) And for the IVI system, the TX rate isn't (or shouldn't be) that high as you mentioned. Only possible trigger of TX is from human: the user of IVI.
> > Please consider #2 and #3, once there are some malicious/buggy program send CAN messages frequently, then it will be harmful to the whole CAN network.
> 
> Do you consider your IVI product having this patch as a safe ECU then?
> >>> Zhongjie: No. It can only part of the whole protection. As I mentioned, we can have protection in multiple layer.
> It's a very rough method, since packets are considered good or bad based on the number of recent packets.
> That's like protecting your email server by rate-limiting ethernet packets.
> It doesn't protect against spam, does it? It's just a rate limiter.
> >>> Zhongjie: I agree it's rough/simple. To make it to identify the Good/Bad things, there will be more work to be done. I tried add some more complicated statistics in the /proc/net/can/stats and launch some user space watchdog to do the audit. I may share with that sometime later. It's doable that way but much more complicated. With this change, I just want to make it work as simple as possible while fulfill our requirement. The limit to the rate just works in the layer of kernel to do the protection against the worst case of denial of service attack.
> 
> > 
> > And by this logic in my patch, I'm trying to just to prevent the abnormal TX rate from user space on IVI product in the worst case:
> > either some bug in the user space programs; or some compromised 
> > program by some hackers with certain zero day attack.
> 
> It's up to userspace to decide when a tx rate is abnormal.
> >>> Zhongjie: Again, I would stress that this is another layer of protection in the kernel.
> >>> We can have different layers of protection which include the user spaces and the kernel.

This will eventually the biggest problem to mainline it:
it's a part of your application policy that doesn't belong in a generic
kernel.

But I guess you're free to maintain the patch out-of-tree for your
devices.

> 
> > 
> > For the ECUs on the CAN bus, I'm not sure why is it not possible to set different config for their case.
> > I mean, e.g. 
> > (a) On the bus, for IVI, I can use it as 5/s,
> > (b) For the cases mentioned by Austin, the config could be 10M for their saturation work.
> > It's just configurable for every nodes on the bus.
> 
> I looks like you set up a simple 'firewall' to CAN, with 1 hardcoded rule in kernel code.
> >>> Zhongjie: Yes :D. I would say implemented in a few line code changes with 1 configurable parameter.
> 

Kind regards,
Kurt

RE: [PATCH] Add TX limit for SocketCAN.

[Shi, Zhongjie]

Hi Kurt,

I agree that: it should be very carefully considered to make decision about whether this change can be pulled by mainline.
And thank you for your candid remarks .

We have requirement which requires me upstream patch first and backporting later. Otherwise my patch will be legacy issue or technical debt.
I believe it's a right decision that stressing the importance of upstream. This way will definitely benefit us (should beyond my projects) in the long term.
I believe you, the maintainers in the community, who have the responsibility and sense on what is the right way.
Also, I'm pretty sure the use case (prevent DoS from user space attack) I mentioned is valid.

Now, one step or several steps further, it would be really appreciated if you experts can give some pointers on how to solve my problem:

    (A) If there is some existing mechanism, then what are they and how can I use that? E.g. Austin mentioned queueing discipline, although I can google try that, it would be much better if who is expert on that can share some details so I can avoid the wrong path.
    (B) If there is no existing mechanism, then what's your opinion on how to create that one? E.g. Is it sensible that I modify my patch to make it somewhat more generic?

Thanks!

Zhongjie



-----Original Message-----
From: linux-can-owner@vger.kernel.org [mailto:linux-can-owner@vger.kernel.org] On Behalf Of Kurt Van Dijck
Sent: Friday, September 01, 2017 5:58 PM
To: Shi, Zhongjie <zhongjie.shi@intel.com>
Cc: Justin Delegard <justin@samsara.com>; Austin Schuh <austin@peloton-tech.com>; Oliver Hartkopp <socketcan@hartkopp.net>; Marc Kleine-Budde <mkl@pengutronix.de>; linux-can <linux-can@vger.kernel.org>
Subject: Re: [PATCH] Add TX limit for SocketCAN.

Hey,

Just 1 remark left ...

...
> > (2) In IVI, there are hundreds of use space processes running which has network access which has more risk to be compromised.
> > (3) And for the IVI system, the TX rate isn't (or shouldn't be) that high as you mentioned. Only possible trigger of TX is from human: the user of IVI.
> > Please consider #2 and #3, once there are some malicious/buggy program send CAN messages frequently, then it will be harmful to the whole CAN network.
> 
> Do you consider your IVI product having this patch as a safe ECU then?
> >>> Zhongjie: No. It can only part of the whole protection. As I mentioned, we can have protection in multiple layer.
> It's a very rough method, since packets are considered good or bad based on the number of recent packets.
> That's like protecting your email server by rate-limiting ethernet packets.
> It doesn't protect against spam, does it? It's just a rate limiter.
> >>> Zhongjie: I agree it's rough/simple. To make it to identify the Good/Bad things, there will be more work to be done. I tried add some more complicated statistics in the /proc/net/can/stats and launch some user space watchdog to do the audit. I may share with that sometime later. It's doable that way but much more complicated. With this change, I just want to make it work as simple as possible while fulfill our requirement. The limit to the rate just works in the layer of kernel to do the protection against the worst case of denial of service attack.
> 
> > 
> > And by this logic in my patch, I'm trying to just to prevent the abnormal TX rate from user space on IVI product in the worst case:
> > either some bug in the user space programs; or some compromised 
> > program by some hackers with certain zero day attack.
> 
> It's up to userspace to decide when a tx rate is abnormal.
> >>> Zhongjie: Again, I would stress that this is another layer of protection in the kernel.
> >>> We can have different layers of protection which include the user spaces and the kernel.

This will eventually the biggest problem to mainline it:
it's a part of your application policy that doesn't belong in a generic kernel.

But I guess you're free to maintain the patch out-of-tree for your devices.

> 
> > 
> > For the ECUs on the CAN bus, I'm not sure why is it not possible to set different config for their case.
> > I mean, e.g. 
> > (a) On the bus, for IVI, I can use it as 5/s,
> > (b) For the cases mentioned by Austin, the config could be 10M for their saturation work.
> > It's just configurable for every nodes on the bus.
> 
> I looks like you set up a simple 'firewall' to CAN, with 1 hardcoded rule in kernel code.
> >>> Zhongjie: Yes :D. I would say implemented in a few line code changes with 1 configurable parameter.
> 

Kind regards,
Kurt
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add TX limit for SocketCAN.

[Brandon Martin]

On 08/31/2017 11:22 PM, Shi, Zhongjie wrote:
> It will be also appreciated if you or Oliver can share with me the details of how to "use queuing discipline" to achieve this.

Presumably it works (or is supposed to work) just like any other 
socket/packet interface.  The "tc" userspace command (friend of 
iproute2) would configure it.  Whether this is actually functional at 
this time is not something I can speak to.
-- 
Brandon Martin
Mothic Technologies
317-565-1357 x7000

Re: [PATCH] Add TX limit for SocketCAN.

[Oliver Hartkopp]

Hi all,

On 09/01/2017 03:54 PM, Brandon Martin wrote:
> On 08/31/2017 11:22 PM, Shi, Zhongjie wrote:
>> It will be also appreciated if you or Oliver can share with me the 
>> details of how to "use queuing discipline" to achieve this.
> 
> Presumably it works (or is supposed to work) just like any other 
> socket/packet interface.  The "tc" userspace command (friend of 
> iproute2) would configure it.  Whether this is actually functional at 
> this time is not something I can speak to.

Yes, in fact using queuing disciplines is the right way to do.

You wanted to create some 'kind of' firewall via bandwidth limitation 
which could be created with 'tc'.

The paper about using CAN with queuing disciplines can be found here:
http://rtime.felk.cvut.cz/can/

http://rtime.felk.cvut.cz/can/socketcan-qdisc-final.pdf

Especially on this level it would work with PF_PACKET sockets too.
Limiting CAN traffic in the CAN network layer in linux/net/can does not 
limit the access to CAN network devices e.g. via packet sockets or 
in-kernel traffic generators (I think there is a traffic generator - but 
I didn't check if it knows about CAN interfaces).

Regards,
Oliver

RE: [PATCH] Add TX limit for SocketCAN.

[Shi, Zhongjie]

Hi Oliver and Martin,

Many thanks for your knowledge sharing!
I'll look into it as your suggestions.

With regard to my patch, I'll hold on and may abandon it later when I learned how to use queuing discipline to resolve my problem.

Thanks again!


Zhongjie

-----Original Message-----
From: Oliver Hartkopp [mailto:socketcan@hartkopp.net] 
Sent: Friday, September 01, 2017 11:39 PM
To: Brandon Martin <martinbv@mothictech.com>; Shi, Zhongjie <zhongjie.shi@intel.com>; Austin Schuh <austin@peloton-tech.com>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>; linux-can <linux-can@vger.kernel.org>
Subject: Re: [PATCH] Add TX limit for SocketCAN.

Hi all,

On 09/01/2017 03:54 PM, Brandon Martin wrote:
> On 08/31/2017 11:22 PM, Shi, Zhongjie wrote:
>> It will be also appreciated if you or Oliver can share with me the 
>> details of how to "use queuing discipline" to achieve this.
> 
> Presumably it works (or is supposed to work) just like any other 
> socket/packet interface.  The "tc" userspace command (friend of
> iproute2) would configure it.  Whether this is actually functional at 
> this time is not something I can speak to.

Yes, in fact using queuing disciplines is the right way to do.

You wanted to create some 'kind of' firewall via bandwidth limitation which could be created with 'tc'.

The paper about using CAN with queuing disciplines can be found here:
http://rtime.felk.cvut.cz/can/

http://rtime.felk.cvut.cz/can/socketcan-qdisc-final.pdf

Especially on this level it would work with PF_PACKET sockets too.
Limiting CAN traffic in the CAN network layer in linux/net/can does not limit the access to CAN network devices e.g. via packet sockets or in-kernel traffic generators (I think there is a traffic generator - but I didn't check if it knows about CAN interfaces).

Regards,
Oliver