lkml.org 
[lkml]   [2022]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [syzbot] kernel BUG in pskb_expand_head
From
Date


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

\
 
 \ /
  Last update: 2022-01-05 13:59    [W:0.046 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site