Re: [syzbot] kernel BUG in pskb_expand_head

[Oliver Hartkopp <socketcan@xxxxxxxxxxxx>]

On 05.01.22 12:44, Marc Kleine-Budde wrote:
> On 19.12.2021 16:19:20, syzbot wrote:
> >   skb_over_panic net/core/skbuff.c:118 [inline]
> >   skb_over_panic net/core/skbuff.c:118 [inline] net/core/skbuff.c:1986
> >   skb_put.cold+0x24/0x24 net/core/skbuff.c:1986 net/core/skbuff.c:1986
> >   isotp_rcv_cf net/can/isotp.c:570 [inline]
> >   isotp_rcv_cf net/can/isotp.c:570 [inline] net/can/isotp.c:668
> >   isotp_rcv+0xa38/0x1e30 net/can/isotp.c:668 net/can/isotp.c:668
>
> > struct tpcon {
> > 	int idx;
> > 	int len;
>          ^^^
> > 	u32 state;
> > 	u8 bs;
> > 	u8 sn;
> > 	u8 ll_dl;
> > 	u8 buf[MAX_MSG_LENGTH + 1];
> > };
> >
> > static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)
> > {
>
> [...]
>
> > 	/* Check for FF_DL escape sequence supporting 32 bit PDU length */
> > 	if (so->rx.len) {
> > 		ff_pci_sz = FF_PCI_SZ12;
> > 	} else {
> > 		/* 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];
> > 		ff_pci_sz = FF_PCI_SZ32;
> > 	}
>
> Full 32 Bit PDUs don't work with struct tpcon::len being an "int". I
> think converting it to "unsigned int" should be done.
>
> [...]
>
> > }
> >
> > static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
> > 			struct sk_buff *skb)
> > {
> > 	struct isotp_sock *so = isotp_sk(sk);
> > 	struct sk_buff *nskb;
> > 	int i;
> >
> > 	if (so->rx.state != ISOTP_WAIT_DATA)
> > 		return 0;
> >
> > 	/* drop if timestamp gap is less than force_rx_stmin nano secs */
> > 	if (so->opt.flags & CAN_ISOTP_FORCE_RXSTMIN) {
> > 		if (ktime_to_ns(ktime_sub(skb->tstamp, so->lastrxcf_tstamp)) <
> > 		    so->force_rx_stmin)
> > 			return 0;
> >
> > 		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;
> >
> > 	/* CFs have usually the LL_DL length */
> > 	if (cf->len < so->rx.ll_dl) {
> > 		/* this is only allowed for the last CF */
> > 		if (so->rx.len - so->rx.idx > so->rx.ll_dl - ae - N_PCI_SZ)
> > 			return 1;
> > 	}
> >
> > 	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;
> > 	}
> > 	so->rx.sn++;
> > 	so->rx.sn %= 16;
> >
> > 	for (i = ae + N_PCI_SZ; i < cf->len; i++) {
> > 		so->rx.buf[so->rx.idx++] = cf->data[i];
> > 		if (so->rx.idx >= so->rx.len)
> > 			break;
> > 	}
> >
> > 	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;
> > 		}
> >
> > 		nskb = alloc_skb(so->rx.len, gfp_any());
> > 		if (!nskb)
> > 			return 1;
> >
> > 		memcpy(skb_put(nskb, so->rx.len), so->rx.buf,
>                         ^^^^^^^
> > 		       so->rx.len);
>
> This is where the skb_over_panic() happens.

Thanks Marc!

Yes I went to this piece of code too - but was not able to find anything 
wrong, as the values at this point should be far(!!) away from INT_MAX.

Due to this check in isotp_rcv_ff():

if (so->rx.len > MAX_MSG_LENGTH) { ... exit

And MAX_MSG_LENGTH is define as 8200.

Btw. making tpcon:len an unsigned int is really the solution to this! 
Which makes the above if-statement act correctly also with values like 
0x80001234.

m(

Thanks for the finding!

Best regards,
Oliver