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

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

[Zhang Changzhong]

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

[PATCH net 1/3] can: j1939: j1939_tp_cmd_recv(): ignore abort message in the BAM transport

[Zhang Changzhong]

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 <zhangchangzhong@huawei.com>
---
 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

Re: [PATCH net 1/3] can: j1939: j1939_tp_cmd_recv(): ignore abort message in the BAM transport

[Oleksij Rempel]

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 <zhangchangzhong@huawei.com>

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

Thank you!

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

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

Re: [PATCH net 1/3] can: j1939: j1939_tp_cmd_recv(): ignore abort message in the BAM transport

[Oleksij Rempel]

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 <zhangchangzhong@huawei.com>
> ---
>  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))

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 |

[PATCH net 2/3] can: j1939: j1939_can_recv(): ignore messages with invalid source address

[Zhang Changzhong]

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 <zhangchangzhong@huawei.com>
---
 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

Re: [PATCH net 2/3] can: j1939: j1939_can_recv(): ignore messages with invalid source address

[Oleksij Rempel]

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 <zhangchangzhong@huawei.com>

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

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

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

Re: [PATCH net 2/3] can: j1939: j1939_can_recv(): ignore messages with invalid source address

[Zhang Changzhong]

On 2021/10/22 18:23, Oleksij Rempel wrote:
> 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 <zhangchangzhong@huawei.com>
> 
> 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.

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

Re: [PATCH net 2/3] can: j1939: j1939_can_recv(): ignore messages with invalid source address

[Oleksij Rempel]

Hi,

On Mon, Oct 25, 2021 at 03:30:57PM +0800, Zhang Changzhong wrote:
> On 2021/10/22 18:23, Oleksij Rempel wrote:
> > 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 <zhangchangzhong@huawei.com>
> > 
> > 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.

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

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

Re: [PATCH net 2/3] can: j1939: j1939_can_recv(): ignore messages with invalid source address

[Zhang Changzhong]

On 2021/10/28 14:51, Oleksij Rempel wrote:
> Hi,
> 
> On Mon, Oct 25, 2021 at 03:30:57PM +0800, Zhang Changzhong wrote:
>> On 2021/10/22 18:23, Oleksij Rempel wrote:
>>> 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 <zhangchangzhong@huawei.com>
>>>
>>> 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.

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

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

Re: [PATCH net 2/3] can: j1939: j1939_can_recv(): ignore messages with invalid source address

[Oleksij Rempel]

On Thu, Oct 28, 2021 at 03:33:21PM +0800, Zhang Changzhong wrote:
> On 2021/10/28 14:51, Oleksij Rempel wrote:
> > Hi,
> > 
> > On Mon, Oct 25, 2021 at 03:30:57PM +0800, Zhang Changzhong wrote:
> >> On 2021/10/22 18:23, Oleksij Rempel wrote:
> >>> 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 <zhangchangzhong@huawei.com>
> >>>
> >>> 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 |

[PATCH net 3/3] can: j1939: j1939_tp_cmd_recv(): check the dst address of TP.CM_BAM

[Zhang Changzhong]

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 <zhangchangzhong@huawei.com>
---
 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

Re: [PATCH net 3/3] can: j1939: j1939_tp_cmd_recv(): check the dst address of TP.CM_BAM

[Oleksij Rempel]

On Thu, Oct 21, 2021 at 10:04:17PM +0800, Zhang Changzhong wrote:
> 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 <zhangchangzhong@huawei.com>
> ---
>  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.

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

Re: [PATCH net 3/3] can: j1939: j1939_tp_cmd_recv(): check the dst address of TP.CM_BAM

[Zhang Changzhong]

On 2021/10/22 18:28, Oleksij Rempel wrote:
> On Thu, Oct 21, 2021 at 10:04:17PM +0800, Zhang Changzhong wrote:
>> 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 <zhangchangzhong@huawei.com>
>> ---
>>  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.

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