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

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

quoted

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.

quoted

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.

I have reproduced the syz problem with Marc's commit, the commit can not fix the panic problem.
So I tried to find the root cause for panic and gave my solution.

Marc's commit just fix the condition that packet size bigger than INT_MAX which trigger
tpcon::{idx,len} integer overflow, but the packet size is 4096 in the syz problem.

so->rx.len is 0 after the following logic in isotp_rcv_ff():

/* get the FF_DL */
so->rx.len = (cf->data[ae] & 0x0F) << 8;
so->rx.len += cf->data[ae + 1];

so->rx.len is 4096 after the following logic in isotp_rcv_ff():

/* FF_DL = 0 => get real length from next 4 bytes */
so->rx.len = cf->data[ae + 2] << 24;
so->rx.len += cf->data[ae + 3] << 16;
so->rx.len += cf->data[ae + 4] << 8;
so->rx.len += cf->data[ae + 5];

so->rx.len is 0 before alloc_skb() and is 4096 after alloc_skb() in isotp_rcv_cf(). The following
skb_put() will trigger panic.

The following log is my reproducing log with Marc's commit and my debug modification in isotp_rcv_cf().

[  150.605776][    C6] isotp_rcv_cf: before alloc_skb so->rc.len: 0, after alloc_skb so->rx.len: 4096
[  150.611477][    C6] skbuff: skb_over_panic: text:ffffffff881ff7be len:4096 put:4096 head:ffff88807f93a800 data:ffff88807f93a800 tail:0x1000 end:0xc0 dev:<NULL>
[  150.615837][    C6] ------------[ cut here ]------------
[  150.617238][    C6] kernel BUG at net/core/skbuff.c:113!

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
.