> 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: >> 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? It receives packets from network, but unexpected packets order make server panic. > >> The following syz problem is one of the scenarios. so->rx.len is >> changed by isotp_rcv_ff() during isotp_rcv_cf(), so->rx.len equals >> 0 before alloc_skb() and equals 4096 after alloc_skb(). That will >> trigger skb_over_panic() in skb_put(). >> >> ======================================================= >> CPU: 1 PID: 19 Comm: ksoftirqd/1 Not tainted 5.16.0-rc8-syzkaller #0 >> RIP: 0010:skb_panic+0x16c/0x16e net/core/skbuff.c:113 >> Call Trace: >> <TASK> >> skb_over_panic net/core/skbuff.c:118 [inline] >> skb_put.cold+0x24/0x24 net/core/skbuff.c:1990 >> isotp_rcv_cf net/can/isotp.c:570 [inline] >> isotp_rcv+0xa38/0x1e30 net/can/isotp.c:668 >> deliver net/can/af_can.c:574 [inline] >> can_rcv_filter+0x445/0x8d0 net/can/af_can.c:635 >> can_receive+0x31d/0x580 net/can/af_can.c:665 >> can_rcv+0x120/0x1c0 net/can/af_can.c:696 >> __netif_receive_skb_one_core+0x114/0x180 net/core/dev.c:5465 >> __netif_receive_skb+0x24/0x1b0 net/core/dev.c:5579 >> >> Check so->rx.state equals ISOTP_IDLE firstly in isotp_rcv_ff(). >> Make sure so->rx idle when receive another new packet. And set >> so->rx.state to ISOTP_IDLE after whole packet being received. >> >> Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") >> Reported-by: syzbot+4c63f36709a642f801c5@xxxxxxxxxxxxxxxxxxxxxxxxx >> Signed-off-by: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx> >> --- >> net/can/isotp.c | 28 +++++++++++++++++----------- >> 1 file changed, 17 insertions(+), 11 deletions(-) >> >> diff --git a/net/can/isotp.c b/net/can/isotp.c >> index df6968b28bf4..a4b174f860f3 100644 >> --- a/net/can/isotp.c >> +++ b/net/can/isotp.c >> @@ -443,8 +443,10 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae) >> int off; >> int ff_pci_sz; >> + if (so->rx.state != ISOTP_IDLE) >> + return 0; >> + >> hrtimer_cancel(&so->rxtimer); >> - so->rx.state = ISOTP_IDLE; > > No matter in which receiving state we receive a first frame (FF) we are required to start a fresh reception process and/or terminate the current attempt. > > Best regards, > Oliver > >> /* get the used sender LL_DL from the (first) CAN frame data length */ >> so->rx.ll_dl = padlen(cf->len); >> @@ -518,8 +520,6 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, >> so->lastrxcf_tstamp = skb->tstamp; >> } >> - hrtimer_cancel(&so->rxtimer); >> - >> /* CFs are never longer than the FF */ >> if (cf->len > so->rx.ll_dl) >> return 1; >> @@ -531,15 +531,15 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, >> return 1; >> } >> + hrtimer_cancel(&so->rxtimer); >> + >> if ((cf->data[ae] & 0x0F) != so->rx.sn) { >> /* wrong sn detected - report 'illegal byte sequence' */ >> sk->sk_err = EILSEQ; >> if (!sock_flag(sk, SOCK_DEAD)) >> sk_error_report(sk); >> - /* reset rx state */ >> - so->rx.state = ISOTP_IDLE; >> - return 1; >> + goto err_out; >> } >> so->rx.sn++; >> so->rx.sn %= 16; >> @@ -551,21 +551,18 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, >> } >> if (so->rx.idx >= so->rx.len) { >> - /* we are done */ >> - so->rx.state = ISOTP_IDLE; >> - >> if ((so->opt.flags & ISOTP_CHECK_PADDING) && >> check_pad(so, cf, i + 1, so->opt.rxpad_content)) { >> /* malformed PDU - report 'not a data message' */ >> sk->sk_err = EBADMSG; >> if (!sock_flag(sk, SOCK_DEAD)) >> sk_error_report(sk); >> - return 1; >> + goto err_out; >> } >> nskb = alloc_skb(so->rx.len, gfp_any()); >> if (!nskb) >> - return 1; >> + goto err_out; >> memcpy(skb_put(nskb, so->rx.len), so->rx.buf, >> so->rx.len); >> @@ -573,6 +570,10 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, >> nskb->tstamp = skb->tstamp; >> nskb->dev = skb->dev; >> isotp_rcv_skb(nskb, sk); >> + >> + /* we are done */ >> + so->rx.state = ISOTP_IDLE; >> + >> return 0; >> } >> @@ -591,6 +592,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, >> /* we reached the specified blocksize so->rxfc.bs */ >> isotp_send_fc(sk, ae, ISOTP_FC_CTS); >> return 0; >> + >> +err_out: >> + /* reset rx state */ >> + so->rx.state = ISOTP_IDLE; >> + return 1; >> } >> static void isotp_rcv(struct sk_buff *skb, void *data) > .
