From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Marc Kleine-Budde <mkl@pengutronix.de>,
syzbot <syzbot+4c63f36709a642f801c5@syzkaller.appspotmail.com>
Cc: anthony.l.nguyen@intel.com, davem@davemloft.net,
eric.dumazet@gmail.com, hawk@kernel.org,
intel-wired-lan-owner@osuosl.org,
intel-wired-lan@lists.osuosl.org, jesse.brandeburg@intel.com,
kuba@kernel.org, linux-can@vger.kernel.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
syzkaller-bugs@googlegroups.com
Subject: Re: [syzbot] kernel BUG in pskb_expand_head
Date: Wed, 5 Jan 2022 13:46:52 +0100 [thread overview]
Message-ID: <c936a5e5-3c16-9060-f2fc-cb315a4c03e1@hartkopp.net> (raw)
In-Reply-To: <20220105114410.brzea3f5flgn5nl2@pengutronix.de>
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
WARNING: multiple messages have this Message-ID (diff)
From: Oliver Hartkopp <socketcan@hartkopp.net>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [syzbot] kernel BUG in pskb_expand_head
Date: Wed, 5 Jan 2022 13:46:52 +0100 [thread overview]
Message-ID: <c936a5e5-3c16-9060-f2fc-cb315a4c03e1@hartkopp.net> (raw)
In-Reply-To: <20220105114410.brzea3f5flgn5nl2@pengutronix.de>
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
next prev parent reply other threads:[~2022-01-05 12:47 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-15 8:38 [syzbot] kernel BUG in pskb_expand_head syzbot
2021-11-15 8:38 ` [Intel-wired-lan] " syzbot
2021-12-20 0:19 ` syzbot
2021-12-20 0:19 ` [Intel-wired-lan] " syzbot
2022-01-05 11:44 ` Marc Kleine-Budde
2022-01-05 11:44 ` [Intel-wired-lan] " Marc Kleine-Budde
2022-01-05 12:46 ` Oliver Hartkopp [this message]
2022-01-05 12:46 ` Oliver Hartkopp
2021-12-20 4:15 ` syzbot
2021-12-20 4:15 ` [Intel-wired-lan] " syzbot
2022-01-05 11:20 ` syzbot
2022-01-05 11:20 ` [Intel-wired-lan] " syzbot
2022-01-05 13:59 ` Eric Dumazet
2022-01-05 13:59 ` [Intel-wired-lan] " Eric Dumazet
2022-01-05 14:04 ` Marc Kleine-Budde
2022-01-05 14:04 ` [Intel-wired-lan] " Marc Kleine-Budde
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c936a5e5-3c16-9060-f2fc-cb315a4c03e1@hartkopp.net \
--to=socketcan@hartkopp.net \
--cc=anthony.l.nguyen@intel.com \
--cc=davem@davemloft.net \
--cc=eric.dumazet@gmail.com \
--cc=hawk@kernel.org \
--cc=intel-wired-lan-owner@osuosl.org \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jesse.brandeburg@intel.com \
--cc=kuba@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=syzbot+4c63f36709a642f801c5@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.