Re: [PATCH net] can: isotp: isotp_rcv_cf(): fix so->rx race problem

On 18.01.22 13:46, Ziyang Xuan (William) wrote:

quoted

Hi,

the referenced syzbot issue has already been fixed in upstream here:

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=5f33a09e769a9da0482f20a6770a342842443776

("can: isotp: convert struct tpcon::{idx,len} to unsigned int")

Additionally this fix changes some behaviour that is required by the ISO 16765-2 specification (see below).

On 17.01.22 13:01, Ziyang Xuan wrote:

quoted

When receive a FF, the current code logic does not consider the real
so->rx.state but set so->rx.state to ISOTP_IDLE directly. That will
make so->rx accessed by multiple receiving processes concurrently.

This is intentionally. "multiple receiving processes" are not allowed resp. specified by ISO 15765-2.

Does it can be a network attack?

Yes. You can see it like this. The ISO 15765-2 protocol is an unreliable 
UDP-like datagram protocol and the session layer takes care about 
timeouts and packet lost.

If you want to disturb that protocol you can also send PDUs with 
out-of-sync packet counters which will make the receiver drop the 
communication attempt.

This is 'CAN-style' ... as usually the bus is very reliable. Security 
and reliable communication is done on top of these protocols.

It receives packets from network, but unexpected packets order make server panic.

Haha, no :-)

Unexpected packets should not make the server panic but only drop the 
communication process.

In the case pointed out by syzbot the unsigned 32 bit length information 
was stored in a signed 32 bit integer which caused a sanity check to fail.

This is now fixed with the patch from Marc.

Regards,
Oliver