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

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?

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

quoted

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@syzkaller.appspotmail.com
Signed-off-by: Ziyang Xuan <redacted>
---
  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

quoted

        /* 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)

.