netdev - Re: [PATCH v1] can: j1939: transport: j1939_simple_recv(): ignore local J1939 messages send not by J1939 stack

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] [day] [month] [year] [list]

Message-ID: <20191218095540.f2vo7mh6jist525h@pengutronix.de>
Date:   Wed, 18 Dec 2019 10:55:40 +0100
From:   Oleksij Rempel <o.rempel@...gutronix.de>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     dev.kurt@...dijck-laurijssen.be, mkl@...gutronix.de,
        wg@...ndegger.com, netdev@...r.kernel.org, kernel@...gutronix.de,
        linux-can@...r.kernel.org
Subject: Re: [PATCH v1] can: j1939: transport: j1939_simple_recv(): ignore
 local J1939 messages send not by J1939 stack

On Wed, Dec 18, 2019 at 10:03:27AM +0100, Oliver Hartkopp wrote:
> Hi Oleksij,
> 
> On 18/12/2019 09.43, Oleksij Rempel wrote:
> > In current J1939 stack implementation, we process all locally send
> > messages as own messages. Even if it was send by CAN_RAW socket.
> > 
> > To reproduce it use following commands:
> > testj1939 -P -r can0:0x80 &
> > cansend can0 18238040#0123
> > 
> > This step will trigger false positive not critical warning:
> > j1939_simple_recv: Received already invalidated message
> > 
> > With this patch we add additional check to make sure, related skb is own
> > echo message.
> 
> in net/can/raw.c we check whether the CAN has been sent from that socket (an
> by default suppress our own transmitted data):
> 
> https://elixir.bootlin.com/linux/v5.4.3/source/net/can/raw.c#L124
> 
> would checking against the 'sk' work for you too?

The J1939 stack work per interface. On each interface we have J1939
privat data with session list.
For each new transfer we register a new session and wait for echo
package to confirm the state of this session.
On recv, if skb->sk is set, we assume, it is echo message, so we searching for
related session to complete or continue with it. Since each session created on
sendmsg path is bound to one of sockets, we won't be able to say if this skb->sk
refers to valid but released socket. Or there is some kind of bug which need to
be fixed.

Searching for the session make sense only if message was handled/send by
the local kernel j1939 stack. If there are other, for example user space j1939
implementations, we should handle them as remote not local stacks. So,
there should be no difference to other j1939 devices on the CAN bus.

J1939 kernel stack cares about echo messages to guarantee proper
ordering of data and control packets on the bus. 

> 
> What happens if someone runs a J1939 implementation on a CAN_RAW socket in
> addition to the in-kernel implementation? Can they talk to each other?

Yes. This patch addressing exactly this use case. It is just eliminate
false positive echo handling.

Regards,
Oleksij
 
> > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> > Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> > ---
> >   net/can/j1939/socket.c    | 1 +
> >   net/can/j1939/transport.c | 4 ++++
> >   2 files changed, 5 insertions(+)
> > 
> > diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> > index f7587428febd..b9a17c2ee16f 100644
> > --- a/net/can/j1939/socket.c
> > +++ b/net/can/j1939/socket.c
> > @@ -398,6 +398,7 @@ static int j1939_sk_init(struct sock *sk)
> >   	spin_lock_init(&jsk->sk_session_queue_lock);
> >   	INIT_LIST_HEAD(&jsk->sk_session_queue);
> >   	sk->sk_destruct = j1939_sk_sock_destruct;
> > +	sk->sk_protocol = CAN_J1939;
> >   	return 0;
> >   }
> > diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> > index 9f99af5b0b11..b135c5e2a86e 100644
> > --- a/net/can/j1939/transport.c
> > +++ b/net/can/j1939/transport.c
> > @@ -2017,6 +2017,10 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb)
> >   	if (!skb->sk)
> >   		return;
> > +	if (skb->sk->sk_family != AF_CAN ||
> > +	    skb->sk->sk_protocol != CAN_J1939)
> > +		return;
> > +
> >   	j1939_session_list_lock(priv);
> >   	session = j1939_session_get_simple(priv, skb);
> >   	j1939_session_list_unlock(priv);
> > 
> 
> 

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)