lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 12 Oct 2021 12:41:32 +0200
From:   Kurt Van Dijck <dev.kurt@...dijck-laurijssen.be>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Zhang Changzhong <zhangchangzhong@...wei.com>,
        Oliver Hartkopp <socketcan@...tkopp.net>,
        Maxime Jayat <maxime.jayat@...ile-devices.fr>,
        Robin van der Gracht <robin@...tonic.nl>,
        linux-kernel@...r.kernel.org,
        Oleksij Rempel <linux@...pel-privat.de>,
        Marc Kleine-Budde <mkl@...gutronix.de>, kernel@...gutronix.de,
        netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        linux-can@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] can: j1939: j1939_xtp_rx_dat_one(): cancel session
 if receive TP.DT with error length

On Tue, 12 Oct 2021 12:21:31 +0200, Oleksij Rempel wrote:
> On Mon, Oct 11, 2021 at 06:40:15PM +0800, Zhang Changzhong wrote:
> > On 2021/10/11 14:35, Oleksij Rempel wrote:
> > > On Sat, Oct 09, 2021 at 04:43:56PM +0800, Zhang Changzhong wrote:
> > >> On 2021/10/8 19:00, Oleksij Rempel wrote:
> > >>> On Fri, Oct 08, 2021 at 05:22:12PM +0800, Zhang Changzhong wrote:
> > >>>> Hi Kurt,
> > >>>> Sorry for the late reply.
> > >>>>
> > >>>> On 2021/9/30 15:42, Kurt Van Dijck wrote:
> > >>>>> On Thu, 30 Sep 2021 11:33:20 +0800, Zhang Changzhong wrote:
> > >>>>>> According to SAE-J1939-21, the data length of TP.DT must be 8 bytes, so
> > >>>>>> cancel session when receive unexpected TP.DT message.
> > >>>>>
> > >>>>> SAE-j1939-21 indeed says that all TP.DT must be 8 bytes.
> > >>>>> However, the last TP.DT may contain up to 6 stuff bytes, which have no meaning.
> > >>>>> If I remember well, they are even not 'reserved'.
> > >>>>
> > >>>> Agree, these bytes are meaningless for last TP.DT.
> > >>>>
> > >>>>>
> > >>>>>>
> > >>>>>> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> > >>>>>> Signed-off-by: Zhang Changzhong <zhangchangzhong@...wei.com>
> > >>>>>> ---
> > >>>>>>  net/can/j1939/transport.c | 7 +++++--
> > >>>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> > >>>>>> index bb5c4b8..eedaeaf 100644
> > >>>>>> --- a/net/can/j1939/transport.c
> > >>>>>> +++ b/net/can/j1939/transport.c
> > >>>>>> @@ -1789,6 +1789,7 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb,
> > >>>>>>  static void j1939_xtp_rx_dat_one(struct j1939_session *session,
> > >>>>>>  				 struct sk_buff *skb)
> > >>>>>>  {
> > >>>>>> +	enum j1939_xtp_abort abort = J1939_XTP_ABORT_FAULT;
> > >>>>>>  	struct j1939_priv *priv = session->priv;
> > >>>>>>  	struct j1939_sk_buff_cb *skcb, *se_skcb;
> > >>>>>>  	struct sk_buff *se_skb = NULL;
> > >>>>>> @@ -1803,9 +1804,11 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
> > >>>>>>  
> > >>>>>>  	skcb = j1939_skb_to_cb(skb);
> > >>>>>>  	dat = skb->data;
> > >>>>>> -	if (skb->len <= 1)
> > >>>>>> +	if (skb->len != 8) {
> > >>>>>>  		/* makes no sense */
> > >>>>>> +		abort = J1939_XTP_ABORT_UNEXPECTED_DATA;
> > >>>>>>  		goto out_session_cancel;
> > >>>>>
> > >>>>> I think this is a situation of
> > >>>>> "be strict on what you send, be tolerant on what you receive".
> > >>>>>
> > >>>>> Did you find a technical reason to abort a session because the last frame didn't
> > >>>>> bring overhead that you don't use?
> > >>>>
> > >>>> No technical reason. The only reason is that SAE-J1939-82 requires responder
> > >>>> to abort session if any TP.DT less than 8 bytes (section A.3.4, Row 7).
> > >>>
> > >>> Do you mean: "BAM Transport: Ensure DUT discards BAM transport when
> > >>> TP.DT data packets are not correct size" ... "Verify DUT discards the
> > >>> BAM transport if any TP.DT data packet has less than 8 bytes"?
> > >>
> > >> Yes.
> > > 
> > > OK, then I have some problems to understand this part:
> > > - 5.10.2.4 Connection Closure
> > >   The “connection abort” message is not allowed to be used by responders in the
> > >   case of a global destination (i.e. BAM).
> > > 
> > > My assumption would be: In case of broadcast transfer, multiple MCU are
> > > receivers. If one of MCU was not able to get complete TP.DT, it should
> > > not abort BAM for all.
> > > 
> > > So, "DUT discards the BAM transport" sounds for me as local action.
> > > Complete TP would be dropped locally.
> > 
> > Yeah, you are right. With this patch receivers drop BAM transport locally
> > because j1939_session_cancel() only send abort message in RTS/CTS transport.
> > 
> > For RTS/CTS transport, SAE-J1939-82 also has similar requirements:
> > "RTS/CTS Transport: Data field size of Transport Data packets for RTS/CTS
> > (DUT as Responder)"..."Verify DUT behavior, e.g., sends a TP.CM_CTS to have
> > packets resent or sends a TP.Conn_Abort, when it receives TP.DT data packets
> > with less than 8 bytes" (section A.3.6, Row 18)
> 
> You are right. Sounds plausible. If we find some device in the field
> which will need a workaround to support less than 8byte, then we will
> need to add some UAPI to configure it. By default we should follow the
> spec. @Kurt, do you have anything against it?

Zhang Changzhong suggested that this is part of compliance testing nowadays.
That obsoletes all technical arguments, and you have no choice than to adapt.

Kurt

Powered by blists - more mailing lists