WARNING in can_rcv

[syzbot]

Hello,

syzkaller hit the following crash on
a8750ddca918032d6349adbf9a4b6555e7db20da
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
C reproducer is attached
syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+438670...@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed. See footer for
details.
If you forward the report, please keep this part and the footer.

device eql entered promiscuous mode
------------[ cut here ]------------
PF_CAN: dropped non conform CAN skbuf: dev type 65534, len 42, datalen 0
WARNING: CPU: 0 PID: 3650 at net/can/af_can.c:729 can_rcv+0x1c5/0x200
net/can/af_can.c:724
Kernel panic - not syncing: panic_on_warn set ...

CPU: 0 PID: 3650 Comm: syzkaller340557 Not tainted 4.15.0-rc8+ #263
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:17 [inline]
dump_stack+0x194/0x257 lib/dump_stack.c:53
panic+0x1e4/0x41c kernel/panic.c:183
__warn+0x1dc/0x200 kernel/panic.c:547
report_bug+0x211/0x2d0 lib/bug.c:184
fixup_bug.part.11+0x37/0x80 arch/x86/kernel/traps.c:178
fixup_bug arch/x86/kernel/traps.c:247 [inline]
do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:296
do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:315
invalid_op+0x22/0x40 arch/x86/entry/entry_64.S:1085
RIP: 0010:can_rcv+0x1c5/0x200 net/can/af_can.c:724
RSP: 0018:ffff8801bbe57068 EFLAGS: 00010286
RAX: dffffc0000000008 RBX: ffff8801d947ae40 RCX: ffffffff8159dade
RDX: 0000000000000000 RSI: 1ffff100377cadc8 RDI: ffff8801bbe56d70
RBP: ffff8801bbe57090 R08: 0000000000000000 R09: 0000000000000000
R10: ffff8801bbe575b0 R11: 0000000000000000 R12: ffff8801d99f8380
R13: ffff8801d9a3d040 R14: 000000000000fffe R15: ffffffff8704d800
__netif_receive_skb_core+0x1a41/0x3460 net/core/dev.c:4473
__netif_receive_skb+0x2c/0x1b0 net/core/dev.c:4538
netif_receive_skb_internal+0x10b/0x670 net/core/dev.c:4611
netif_receive_skb+0xae/0x390 net/core/dev.c:4635
tun_rx_batched.isra.50+0x5ee/0x870 drivers/net/tun.c:1378
tun_get_user+0x24ac/0x3710 drivers/net/tun.c:1774
tun_chr_write_iter+0xb9/0x160 drivers/net/tun.c:1800
call_write_iter include/linux/fs.h:1772 [inline]
new_sync_write fs/read_write.c:469 [inline]
__vfs_write+0x684/0x970 fs/read_write.c:482
vfs_write+0x189/0x510 fs/read_write.c:544
SYSC_write fs/read_write.c:589 [inline]
SyS_write+0xef/0x220 fs/read_write.c:581
entry_SYSCALL_64_fastpath+0x29/0xa0
RIP: 0033:0x4445d9
RSP: 002b:00007fffbeafb128 EFLAGS: 00000217 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: ffffffffffffffff RCX: 00000000004445d9
RDX: 000000000000002e RSI: 0000000020b61fd2 RDI: 0000000000000004
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000217 R12: 00000000006c7165
R13: 0000000000401fb0 R14: 0000000000000000 R15: 0000000000000000
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
This bug is generated by a dumb bot. It may contain errors.
See https://goo.gl/tpsmEJ for details.
Direct all questions to syzk...@googlegroups.com.

syzbot will keep track of this bug report.
If you forgot to add the Reported-by tag, once the fix for this bug is
merged
into any tree, please reply to this email with:
#syz fix: exact-commit-title
If you want to test a patch for this bug, please reply with:
#syz test: git://repo/address.git branch
and provide the patch inline or as an attachment.
To mark this as a duplicate of another syzbot report, please reply with:
#syz dup: exact-subject-of-another-report
If it's a one-off invalid bug report, please reply with:
#syz invalid
Note: if the crash happens again, it will cause creation of a new bug
report.
Note: all commands must start from beginning of the line in the email body.

[Marc Kleine-Budde]

On 01/16/2018 06:58 PM, syzbot wrote:
> Hello,
>
> syzkaller hit the following crash on
> a8750ddca918032d6349adbf9a4b6555e7db20da
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers
>
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+438670...@syzkaller.appspotmail.com
> It will help syzbot understand when the bug is fixed. See footer for
> details.
> If you forward the report, please keep this part and the footer.
>
> device eql entered promiscuous mode
> ------------[ cut here ]------------
> PF_CAN: dropped non conform CAN skbuf: dev type 65534, len 42, datalen 0
> WARNING: CPU: 0 PID: 3650 at net/can/af_can.c:729 can_rcv+0x1c5/0x200
> net/can/af_can.c:724
> Kernel panic - not syncing: panic_on_warn set ...

Invalid packages generate a warning (WARN_ONCE()), and you have
panic_on_warn active. Should we better silently drop these CAN packages?

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |

[Dmitry Vyukov]

Hi,

pr_warn_once() will be more appropriate. It prints a single line.

[Marc Kleine-Budde]

On 01/16/2018 07:11 PM, Dmitry Vyukov wrote:
>>> Kernel panic - not syncing: panic_on_warn set ...
>>
>> Invalid packages generate a warning (WARN_ONCE()), and you have
>> panic_on_warn active. Should we better silently drop these CAN packages?
>

> pr_warn_once() will be more appropriate. It prints a single line.

Thanks - just posted the patches, will send a pull request to David
tomorrow.

regards,

[Oliver Hartkopp]

On 01/16/2018 07:11 PM, Dmitry Vyukov wrote:

The idea behind this WARN() is to detect really bad things that might
have happen on network driver level:

The CAN subsystem registers with dev_add_pack() for ETH_P_CAN and
ETH_P_CANFD only. These ETH_P_ types are only allowed to be created by
CAN network devices (like vcan, vxcan, and real CAN drivers).

I don't have any strong opinion on using WARN() or pr_warn_once().
Is this detected violation worth using WARN(), as something already must
have gone really wrong to trigger this issue?

Best regards,
Oliver

[Eric Biggers]

WARN() indicates a kernel bug. If it's instead "userspace did something
stupid", or "someone sent some unexpected network packet", it needs to be
pr_warn_once(), pr_warn_ratelimited(), or removed entirely.

Eric

[Dmitry Vyukov]

The packet comes from tun device. We could change tun to filter out
such packages earlier. However, in the context of "syzkaller support
for AF_CAN" discussion, it would actually be useful for fuzzer to be
able emit can packets for testing purposes. For example, for tcp it
can not just emit random packets, it can build complex user<->network
interactions, for example, open a listening socket, connect to it
"from outside", accept the connection, and then exchange some data
over the active connection. It could do the same for can.
Is it possible to allow can packets via tun? Then we could leave this
WARNING in place. tun/vcan are contained within a net namespace, so
this should not be a security problem, right?
Or is there a way to do the same with vcan? If yes, then fuzzer could
use vcan. But then we need some fix for this WARNING: either change it
to pr_warn or change tun (I don't have strong preference which one).

[Oliver Hartkopp]

Ok. Thanks for the explanation!
It is "some bogus network driver sent something unexpected" - but that
does not harm the entire system.

pr_warn_once() seems the right way to go then.

Thanks,
Oliver

[Oliver Hartkopp]

Yes - definitely! It's a safer process to check the reception side
instead of maintaining thousands of potential transmitters.

> For example, for tcp it
> can not just emit random packets, it can build complex user<->network
> interactions, for example, open a listening socket, connect to it
> "from outside", accept the connection, and then exchange some data
> over the active connection. It could do the same for can.

Yes.

> Is it possible to allow can packets via tun?

Hm - didn't even think about it.
CAN frames have a fixed data structure (struct can_frame) so the tunnel
would need to be capable to process SOCK_SEQPACKET (?!?) traffic.

Right now there has been no work to 'tunnel' CAN traffic.

> Then we could leave this
> WARNING in place.

Yes.

> tun/vcan are contained within a net namespace, so
> this should not be a security problem, right?

vcan can be created in or moved into a namespace. vxcan can bridge
namespaces similar to veth. This is all local traffic then.

What kind of security problem would you have in mind there?

> Or is there a way to do the same with vcan? If yes, then fuzzer could
> use vcan.

Yes. This would be my idea too. Unfortunately I'm very busy @work this
week - so I would like to dig deeper into your mail some days ago at the
beginning of next week.

> But then we need some fix for this WARNING: either change it
> to pr_warn or change tun (I don't have strong preference which one).

From the discussions (also with Eric) I think going with pr_warn is the
right way for now.

Tnx & best regards,
Oliver

[Dmitry Vyukov]

But it all seems to be working already (as proven by this report).
syzkaller emits ethernet packets with ETH_P_CAN and they are routed to
can_rcv. It's just that all packed are dropped on this check. So
perhaps if we relax this check, it will all work.

>> Then we could leave this
>> WARNING in place.
>
>
> Yes.
>
>> tun/vcan are contained within a net namespace, so
>> this should not be a security problem, right?
>
>
> vcan can be created in or moved into a namespace. vxcan can bridge
> namespaces similar to veth. This is all local traffic then.
>
> What kind of security problem would you have in mind there?

Something along the following lines:
A machine has can network attached and an eithernet cable. Attacker
sends ethernet packets with ETH_P_CAN and they are emitted into can
stack. The system thinks these packets come from can network, but they
actually come from ethernet.
But it we allow only tun, then I think it should not be a problem as
tun requires admin rights in the net namespace.

[Marc Kleine-Budde]

Is this an Acked-by for both patches?

[Oliver Hartkopp]

Yes :-)

Acked-by: Oliver Hartkopp <sock...@hartkopp.net>

I just did not expect that you wanted to update the patches before
sending ...

Thanks,
Oliver

[Marc Kleine-Budde]

On 01/17/2018 05:29 PM, Oliver Hartkopp wrote:
>> Is this an Acked-by for both patches?
>>
>
> Yes :-)
>
> Acked-by: Oliver Hartkopp <sock...@hartkopp.net>

Tnx

> I just did not expect that you wanted to update the patches before
> sending ...

Why not :)