Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752279AbeAQIq7 (ORCPT + 1 other); Wed, 17 Jan 2018 03:46:59 -0500 Received: from mail-pf0-f195.google.com ([209.85.192.195]:44281 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750794AbeAQIq4 (ORCPT ); Wed, 17 Jan 2018 03:46:56 -0500 X-Google-Smtp-Source: ACJfBospGfuKgyzxv6rYPC2DhFGbPSaiw10ZDtrLXl6MOyI3z7umqwa/1lGihG6juON0q/Z+pQrqvfm0cd6ViJ+rU1Q= MIME-Version: 1.0 In-Reply-To: <25021468-cd11-b44d-b9f3-0afd987d1d11@hartkopp.net> References: <001a11c16b40d795350562e878cc@google.com> <48c5623e-de78-5cfa-b537-2acf9a44ae51@hartkopp.net> <20180117071204.GG15527@zzz.localdomain> <25021468-cd11-b44d-b9f3-0afd987d1d11@hartkopp.net> From: Dmitry Vyukov Date: Wed, 17 Jan 2018 09:46:35 +0100 Message-ID: Subject: Re: WARNING in can_rcv To: Oliver Hartkopp Cc: Eric Biggers , Marc Kleine-Budde , syzbot , David Miller , linux-can@vger.kernel.org, LKML , netdev , syzkaller-bugs@googlegroups.com Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Wed, Jan 17, 2018 at 9:22 AM, Oliver Hartkopp wrote: > > > On 01/17/2018 08:39 AM, Dmitry Vyukov wrote: >> >> On Wed, Jan 17, 2018 at 8:12 AM, Eric Biggers wrote: >>> >>> On Wed, Jan 17, 2018 at 07:39:24AM +0100, Oliver Hartkopp wrote: >>>> >>>> >>>> >>>> On 01/16/2018 07:11 PM, Dmitry Vyukov wrote: >>>>> >>>>> On Tue, Jan 16, 2018 at 7:07 PM, Marc Kleine-Budde >>>>> wrote: >>>>>> >>>>>> 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+4386709c0c1284dca827@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? >>>>> >>>>> >>>>> Hi, >>>>> >>>>> pr_warn_once() will be more appropriate. It prints a single line. >>>>> >>>> >>>> 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? >>>> >>> >>> 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. >> >> >> >> 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. > > > 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. 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. >> 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