[PATCH net 0/3] can: j1939: fix some standard conformance problems

This patchset fixes 3 standard conformance problems in the j1939 stack.

Zhang Changzhong (3):
  can: j1939: j1939_tp_cmd_recv(): ignore abort message in the BAM
    transport
  can: j1939: j1939_can_recv(): ignore messages with invalid source
    address
  can: j1939: j1939_tp_cmd_recv(): check the dst address of TP.CM_BAM

 net/can/j1939/main.c      | 4 ++++
 net/can/j1939/transport.c | 5 +++++
 2 files changed, 9 insertions(+)

-- 
2.9.5

According to SAE-J1939-82 2015 (A.3.6 Row 2), a receiver should never
send TP.CM_CTS to the global address, so we can add a check in
j1939_can_recv() to drop messages with invalid source address.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>
---
 net/can/j1939/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 08c8606..4f1e4bb 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c

@@ -75,6 +75,10 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
 	skcb->addr.pgn = (cf->can_id >> 8) & J1939_PGN_MAX;
 	/* set default message type */
 	skcb->addr.type = J1939_TP;
+	if (!j1939_address_is_valid(skcb->addr.sa))
+		/* ignore messages whose sa is broadcast address */
+		goto done;
+
 	if (j1939_pgn_is_pdu1(skcb->addr.pgn)) {
 		/* Type 1: with destination address */
 		skcb->addr.da = skcb->addr.pgn;

-- 
2.9.5

The TP.CM_BAM message must be sent to the global address [1], so add a
check to drop TP.CM_BAM sent to a non-global address.

Without this patch, the receiver will treat the following packets as
normal RTS/CTS tranport:
18EC0102#20090002FF002301
18EB0102#0100000000000000
18EB0102#020000FFFFFFFFFF

[1] SAE-J1939-82 2015 A.3.3 Row 1.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>
---
 net/can/j1939/transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 2efa606..93ec0c3 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c

@@ -2019,6 +2019,8 @@ static void j1939_tp_cmd_recv(struct j1939_priv *priv, struct sk_buff *skb)
 		extd = J1939_ETP;
 		fallthrough;
 	case J1939_TP_CMD_BAM:
+		if (!j1939_cb_is_broadcast(skcb))
+			return;
 		fallthrough;
 	case J1939_TP_CMD_RTS:
 		if (skcb->addr.type != extd)

-- 
2.9.5

This patch prevents BAM transport from being closed by receiving abort
message, as specified in SAE-J1939-82 2015 (A.3.3 Row 4).

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>
---
 net/can/j1939/transport.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index bb5c4b8..2efa606 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c

@@ -2081,6 +2081,9 @@ static void j1939_tp_cmd_recv(struct j1939_priv *priv, struct sk_buff *skb)
 		break;
 
 	case J1939_ETP_CMD_ABORT: /* && J1939_TP_CMD_ABORT */
+		if (j1939_cb_is_broadcast(skcb))
+			return;
+
 		if (j1939_tp_im_transmitter(skcb))
 			j1939_xtp_rx_abort(priv, skb, true);
 

-- 
2.9.5

On Thu, Oct 21, 2021 at 10:04:15PM +0800, Zhang Changzhong wrote:

This patch prevents BAM transport from being closed by receiving abort
message, as specified in SAE-J1939-82 2015 (A.3.3 Row 4).

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Thu, Oct 21, 2021 at 10:04:16PM +0800, Zhang Changzhong wrote:

According to SAE-J1939-82 2015 (A.3.6 Row 2), a receiver should never
send TP.CM_CTS to the global address, so we can add a check in
j1939_can_recv() to drop messages with invalid source address.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>

NACK. This will break Address Claiming, where first message is SA == 0xff

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Thu, Oct 21, 2021 at 10:04:17PM +0800, Zhang Changzhong wrote:

Please add here netdev_err_once("with some message"), we need to know if
some MCU makes bad things.

+			return;
 		fallthrough;
 	case J1939_TP_CMD_RTS:
 		if (skcb->addr.type != extd)
-- 
2.9.5

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Thu, Oct 21, 2021 at 10:04:15PM +0800, Zhang Changzhong wrote:

Please add here netdev_err_once("Abort to broadcast, ignoring!"), or
something like this.

+			return;
+
 		if (j1939_tp_im_transmitter(skcb))
 			j1939_xtp_rx_abort(priv, skb, true);
 
-- 
2.9.5

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On 2021/10/22 18:28, Oleksij Rempel wrote:

On Thu, Oct 21, 2021 at 10:04:17PM +0800, Zhang Changzhong wrote:

quoted

The TP.CM_BAM message must be sent to the global address [1], so add a
check to drop TP.CM_BAM sent to a non-global address.

Without this patch, the receiver will treat the following packets as
normal RTS/CTS tranport:
18EC0102#20090002FF002301
18EB0102#0100000000000000
18EB0102#020000FFFFFFFFFF

[1] SAE-J1939-82 2015 A.3.3 Row 1.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>
---
 net/can/j1939/transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 2efa606..93ec0c3 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c

@@ -2019,6 +2019,8 @@ static void j1939_tp_cmd_recv(struct j1939_priv *priv, struct sk_buff *skb)
 		extd = J1939_ETP;
 		fallthrough;
 	case J1939_TP_CMD_BAM:
+		if (!j1939_cb_is_broadcast(skcb))

Please add here netdev_err_once("with some message"), we need to know if
some MCU makes bad things.

Ok, will do.

The current patch breaks ETP functionality as the J1939_ETP_CMD_RTS fallthrough
to J1939_TP_CMD_BAM, this will also be fixed in v2.

quoted

+			return;
 		fallthrough;
 	case J1939_TP_CMD_RTS:
 		if (skcb->addr.type != extd)
-- 
2.9.5

On 2021/10/22 18:23, Oleksij Rempel wrote:

On Thu, Oct 21, 2021 at 10:04:16PM +0800, Zhang Changzhong wrote:

quoted

According to SAE-J1939-82 2015 (A.3.6 Row 2), a receiver should never
send TP.CM_CTS to the global address, so we can add a check in
j1939_can_recv() to drop messages with invalid source address.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>

NACK. This will break Address Claiming, where first message is SA == 0xff

I know that 0xfe can be used as a source address, but which message has a source
address of 0xff?

According to SAE-J1939-81 2017 4.2.2.8：

  The network address 255, also known as the Global address, is permitted in the
  Destination Address field of the SAE J1939 message identifier but never in the
  Source Address field.

quoted

---
 net/can/j1939/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 08c8606..4f1e4bb 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c

@@ -75,6 +75,10 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
 	skcb->addr.pgn = (cf->can_id >> 8) & J1939_PGN_MAX;
 	/* set default message type */
 	skcb->addr.type = J1939_TP;
+	if (!j1939_address_is_valid(skcb->addr.sa))
+		/* ignore messages whose sa is broadcast address */
+		goto done;
+
 	if (j1939_pgn_is_pdu1(skcb->addr.pgn)) {
 		/* Type 1: with destination address */
 		skcb->addr.da = skcb->addr.pgn;

-- 
2.9.5

Hi,

On Mon, Oct 25, 2021 at 03:30:57PM +0800, Zhang Changzhong wrote:

On 2021/10/22 18:23, Oleksij Rempel wrote:

quoted

On Thu, Oct 21, 2021 at 10:04:16PM +0800, Zhang Changzhong wrote:

quoted

According to SAE-J1939-82 2015 (A.3.6 Row 2), a receiver should never
send TP.CM_CTS to the global address, so we can add a check in
j1939_can_recv() to drop messages with invalid source address.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>

NACK. This will break Address Claiming, where first message is SA == 0xff

I know that 0xfe can be used as a source address, but which message has a source
address of 0xff?

According to SAE-J1939-81 2017 4.2.2.8：

  The network address 255, also known as the Global address, is permitted in the
  Destination Address field of the SAE J1939 message identifier but never in the
  Source Address field.

You are right. Thx!

Are you using any testing frameworks?
Can you please take a look here:
https://github.com/linux-can/can-tests/tree/master/j1939

We are using this scripts for regression testing of some know bugs.

quoted

quoted

---
 net/can/j1939/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 08c8606..4f1e4bb 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c

@@ -75,6 +75,10 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
 	skcb->addr.pgn = (cf->can_id >> 8) & J1939_PGN_MAX;
 	/* set default message type */
 	skcb->addr.type = J1939_TP;
+	if (!j1939_address_is_valid(skcb->addr.sa))
+		/* ignore messages whose sa is broadcast address */
+		goto done;

Please add some warning once message here. We wont to know if something bad
is happening on the bus.

quoted

quoted

+
 	if (j1939_pgn_is_pdu1(skcb->addr.pgn)) {
 		/* Type 1: with destination address */
 		skcb->addr.da = skcb->addr.pgn;
-- 
2.9.5

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On 2021/10/28 14:51, Oleksij Rempel wrote:

Hi,

On Mon, Oct 25, 2021 at 03:30:57PM +0800, Zhang Changzhong wrote:

quoted

On 2021/10/22 18:23, Oleksij Rempel wrote:

quoted

On Thu, Oct 21, 2021 at 10:04:16PM +0800, Zhang Changzhong wrote:

quoted

According to SAE-J1939-82 2015 (A.3.6 Row 2), a receiver should never
send TP.CM_CTS to the global address, so we can add a check in
j1939_can_recv() to drop messages with invalid source address.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>

NACK. This will break Address Claiming, where first message is SA == 0xff

I know that 0xfe can be used as a source address, but which message has a source
address of 0xff?

According to SAE-J1939-81 2017 4.2.2.8：

  The network address 255, also known as the Global address, is permitted in the
  Destination Address field of the SAE J1939 message identifier but never in the
  Source Address field.

You are right. Thx!

Are you using any testing frameworks?
Can you please take a look here:
https://github.com/linux-can/can-tests/tree/master/j1939

We are using this scripts for regression testing of some know bugs.

Great! I'll run these scripts before posting patches.

quoted

quoted

quoted

---
 net/can/j1939/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 08c8606..4f1e4bb 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c

@@ -75,6 +75,10 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
 	skcb->addr.pgn = (cf->can_id >> 8) & J1939_PGN_MAX;
 	/* set default message type */
 	skcb->addr.type = J1939_TP;
+	if (!j1939_address_is_valid(skcb->addr.sa))
+		/* ignore messages whose sa is broadcast address */
+		goto done;

Please add some warning once message here. We wont to know if something bad
is happening on the bus.

Will do. Thanks for your suggestions.

Regards,
Changzhong

quoted

quoted

quoted

+
 	if (j1939_pgn_is_pdu1(skcb->addr.pgn)) {
 		/* Type 1: with destination address */
 		skcb->addr.da = skcb->addr.pgn;
-- 
2.9.5

Regards,
Oleksij

On Thu, Oct 28, 2021 at 03:33:21PM +0800, Zhang Changzhong wrote:

On 2021/10/28 14:51, Oleksij Rempel wrote:

quoted

Hi,

On Mon, Oct 25, 2021 at 03:30:57PM +0800, Zhang Changzhong wrote:

quoted

On 2021/10/22 18:23, Oleksij Rempel wrote:

quoted

On Thu, Oct 21, 2021 at 10:04:16PM +0800, Zhang Changzhong wrote:

quoted

According to SAE-J1939-82 2015 (A.3.6 Row 2), a receiver should never
send TP.CM_CTS to the global address, so we can add a check in
j1939_can_recv() to drop messages with invalid source address.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <redacted>

NACK. This will break Address Claiming, where first message is SA == 0xff

I know that 0xfe can be used as a source address, but which message has a source
address of 0xff?

According to SAE-J1939-81 2017 4.2.2.8：

  The network address 255, also known as the Global address, is permitted in the
  Destination Address field of the SAE J1939 message identifier but never in the
  Source Address field.

You are right. Thx!

Are you using any testing frameworks?
Can you please take a look here:
https://github.com/linux-can/can-tests/tree/master/j1939

We are using this scripts for regression testing of some know bugs.

Great! I'll run these scripts before posting patches.

 
You are welcome to extend this tests :)

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |