From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B3F8AC43381 for ; Fri, 1 Mar 2019 16:36:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6E1DC20850 for ; Fri, 1 Mar 2019 16:36:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=Mellanox.com header.i=@Mellanox.com header.b="Upk1R8Di" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2389073AbfCAQgC (ORCPT ); Fri, 1 Mar 2019 11:36:02 -0500 Received: from mail-eopbgr00064.outbound.protection.outlook.com ([40.107.0.64]:9694 "EHLO EUR02-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S2389033AbfCAQgA (ORCPT ); Fri, 1 Mar 2019 11:36:00 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=lAGnjEq6xWhTXjfo7gLT0z6IP70RjMhbimiF8NHqqOw=; b=Upk1R8DiDAAILNminBKz1J53NziuQ8l4fBDwfIeZkKwL0t/6t3qWGmbOzOfRDCuy4wAFq8vUFUXzhB2gj45CjQoiyZLh88ikMjugYncGhxVoxnE462pHcYXgtnN2h6cLU/0O118Jc1h/uAXIEEteQGTDI6h00ehVCb4Ix9uoTP8= Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com (10.167.127.11) by HE1PR0502MB3098.eurprd05.prod.outlook.com (10.175.29.144) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1665.15; Fri, 1 Mar 2019 16:35:55 +0000 Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::b03d:8cd4:d259:f749]) by HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::b03d:8cd4:d259:f749%6]) with mapi id 15.20.1665.017; Fri, 1 Mar 2019 16:35:55 +0000 From: Vlad Buslov To: Davide Caratti CC: "David S. Miller" , Jamal Hadi Salim , Cong Wang , Jiri Pirko , "pabeni@redhat.com" , "netdev@vger.kernel.org" Subject: Re: [PATCH net 07/16] net/sched: act_connmark: validate the control action inside init() Thread-Topic: [PATCH net 07/16] net/sched: act_connmark: validate the control action inside init() Thread-Index: AQHU0EzXjCMtxrbUCUaeDh+fp7ESFg== Date: Fri, 1 Mar 2019 16:35:55 +0000 Message-ID: References: <89f95fccc16101df5d1b01c00957751e412439a5.1551288982.git.dcaratti@redhat.com> In-Reply-To: <89f95fccc16101df5d1b01c00957751e412439a5.1551288982.git.dcaratti@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: LO2P265CA0380.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:a3::32) To HE1PR0502MB3641.eurprd05.prod.outlook.com (2603:10a6:7:85::11) authentication-results: spf=none (sender IP is ) smtp.mailfrom=vladbu@mellanox.com; x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [37.142.13.130] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 60e1d0c0-db91-46af-06e8-08d69e63f9cd x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600127)(711020)(4605104)(4618075)(2017052603328)(7153060)(7193020);SRVR:HE1PR0502MB3098; x-ms-traffictypediagnostic: HE1PR0502MB3098: x-microsoft-exchange-diagnostics: =?iso-8859-1?Q?1;HE1PR0502MB3098;23:bAuAMuEm75vTq2g5faII/5BaZeIbA0tEhzBJO?= =?iso-8859-1?Q?xNZTVAPf+pbCbiNxghNB1gEk9byw4HvfU0TaH9XsZnzeKARQ69fcxOWY2D?= =?iso-8859-1?Q?YLhffxFt8387dMvNLEkzRsJYGnML536rTsaVLciOc3ZNepdETCAzPdQBgi?= =?iso-8859-1?Q?5Lhi7he1MeKCb63nbnKbfOYTxdpW4ZPZj+jLLPm3BUUsITe05hH4uTgOMN?= =?iso-8859-1?Q?Ytu6GmGhZwUo86Bt6F7wGu3AVQniAzJN6+/cIyVLHfBTxayMP3082/gphY?= =?iso-8859-1?Q?igpZiuPpIjt+UzXcsY2yIukS5jg5NvZEiA4cnYQuLwKx8ABKSLL1h/wmF7?= =?iso-8859-1?Q?BsAMGk92Oop44DCovk5E27UPnnJdphfzwe+cxAixrgwv7SwcIgagKGJFJF?= =?iso-8859-1?Q?0U2+56sD97Y7qUJlDmGWJStk0F7eOYW7qt01AwRyKkuyQ/ge1HAZ7cD7kt?= =?iso-8859-1?Q?HbTx1DoCAJgeX0dmPOfO6JTv/4kNPBbb1qFMMUZio9dxBCxHwZq+fOfc/n?= =?iso-8859-1?Q?fdLQuWYMsFuDsV1NVmcytMsNuDHnnq+aGR99Bncmg0MDpgGk+16z8CqSVT?= =?iso-8859-1?Q?kyP4kjLMB453ioDS37JL9o6ivngEcCeeniNK7zIWTlhzLQ9Ctn+x+8lI/m?= =?iso-8859-1?Q?FKj25c6JUsRcN288mEwYhP3jgMzTJnByhjy4RXX85I35nT3Do622OKlM1u?= =?iso-8859-1?Q?+v7TMdxvSq57hMVTEvcfD3TjKHbcGjKY51RSfdhSiJM1q4E7FCgwLICf2/?= =?iso-8859-1?Q?qJedVJP5Brj1K0XFunZWMTy76jgJ+yvv8NeQq6LBDsLWpJFuanUyxct1MR?= =?iso-8859-1?Q?SuHDO+5wFKHB2gVUynx99egRm55rVzWztPiSeuYbfet0/0JThN5wskFehY?= =?iso-8859-1?Q?aMoWJFXpQsGBKVPdX9TUIo/1pThkw2KnJBFgSQFj4ynXZI7jcOrDDTUlqF?= =?iso-8859-1?Q?bPGpvd/yV7OEPAHYx45hNd9YGYgANNegZ1cMsTgkLMNQfeNSw2ld/eznMt?= =?iso-8859-1?Q?X4/K3/aI2i1Zq6lGxfWL/nspBHbaMVSfHpXtF64xghOr23cdMesrlSppNJ?= =?iso-8859-1?Q?HvhdvvAz7mWbvh1lUFtwgdXjIdmB5sGH8hKOGzGRsa8glOdH5pMlktLV2o?= =?iso-8859-1?Q?K93XjYBxhEGw2fJRZ0QcLo+CiHt4Jl1qV+ytyFZ9+ei6FpTeT3s08cup3M?= =?iso-8859-1?Q?5y7aLAsHnQQjAs786E8Vbd2h9ywELzSUply/FL8N6iX/PrEpm9CmXOrRsd?= =?iso-8859-1?Q?vonOSpI41zVYLOco6OZY7Q5FjpVCmq+mCd3KHkWAYyJspIZzDcr1HrofAo?= =?iso-8859-1?Q?u/opsUFrwiJkT68dVbHuBgz07KhrO8boqlswBz18+D2KZXKufszswwyFWx?= =?iso-8859-1?Q?ZibhsigQpc=3D?= x-microsoft-antispam-prvs: x-forefront-prvs: 09634B1196 x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(376002)(136003)(396003)(366004)(346002)(39860400002)(189003)(199004)(51234002)(68736007)(5660300002)(229853002)(52116002)(76176011)(99286004)(8676002)(6436002)(478600001)(15650500001)(14454004)(36756003)(6916009)(102836004)(316002)(81166006)(105586002)(86362001)(71200400001)(256004)(81156014)(386003)(14444005)(6486002)(6506007)(54906003)(71190400001)(8936002)(106356001)(476003)(486006)(2616005)(11346002)(446003)(305945005)(186003)(53936002)(4326008)(6246003)(6512007)(26005)(45080400002)(6116002)(3846002)(97736004)(66066001)(25786009)(2906002)(7736002);DIR:OUT;SFP:1101;SCL:1;SRVR:HE1PR0502MB3098;H:HE1PR0502MB3641.eurprd05.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: MPSeuKcauX/8lfD8zPnFkDSci4cep3oTDb1xXvIvPeKTror7obUMI9qcfwHJXDhzI3IdhnldyiPlz+a5reE8VWh5kKMgT0DcEy11sYsJUSoEsXSHUB/mTId/FTBweGNc5ikbyzK60NveuIVTT9EzTNqyneVa2dGPwlyltOJD6aNfU0Bi0wye+O4fHIyNrswLLNXB0FhyrA93ZmVo4IIP2I6pcjDOBvsFchhG/8ek47osCZdw0a3iDMzqrxew5+8BaUyhaaO5zCFxGWpZ4FaMqZpscwdJDNnVu3WAdlVuh4yS5Z+atIPCwV18vASFFLY2+p1krWzBYgR71orF4O6n54hTpRNdgdsXQySMROYaPSvrq7Sq+IKeWvMMLvLZOPKCv9BLZk1EH/vUDlfKXpY7JOofgPJ7oPDserWngEwmfQQ= Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 60e1d0c0-db91-46af-06e8-08d69e63f9cd X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Mar 2019 16:35:55.3341 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0502MB3098 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed 27 Feb 2019 at 17:40, Davide Caratti wrote: > the following script: > > # tc qdisc add dev crash0 clsact > # tc filter add dev crash0 egress matchall \ > > action connmark pass index 90 > # tc actions replace action connmark \ > > goto chain 42 index 90 cookie c1a0c1a0 > # tc actions show action connmark > > had the following output: > > Error: Failed to init TC action chain. > We have an error talking to the kernel > total acts 1 > > action order 0: connmark zone 0 goto chain 42 > index 90 ref 2 bind 1 > cookie c1a0c1a0 > > Then, the first packet transmitted by crash0 made the kernel crash: > > BUG: unable to handle kernel NULL pointer dereference at 000000000000000= 0 > #PF error: [normal kernel read fault] > PGD 0 P4D 0 > Oops: 0000 [#1] SMP PTI > CPU: 0 PID: 302 Comm: kworker/0:2 Not tainted 5.0.0-rc4.gotochain_crash+= #533 > Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011 > Workqueue: ipv6_addrconf addrconf_dad_work > RIP: 0010:tcf_action_exec+0xb8/0x100 > Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 = c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 8= 9 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 > RSP: 0018:ffff9bea406c3ad0 EFLAGS: 00010246 > RAX: 000000002000002a RBX: ffff8c5dfc009f00 RCX: 0000000000000000 > RDX: 0000000000000000 RSI: ffff9bea406c3a80 RDI: ffff8c5dfb9d6ec0 > RBP: ffff9bea406c3b70 R08: ffff8c5dfda222a0 R09: ffffffff90933c3c > R10: 0000000000000000 R11: 0000000092793f7d R12: ffff8c5df48b3c00 > R13: ffff8c5df48b3c08 R14: 0000000000000001 R15: ffff8c5dfb9d6e40 > FS: 0000000000000000(0000) GS:ffff8c5dfda00000(0000) knlGS:000000000000= 0000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000000000000 CR3: 0000000062e0e006 CR4: 00000000001606f0 > Call Trace: > tcf_classify+0x58/0x120 > __dev_queue_xmit+0x40a/0x890 > ? ndisc_next_option+0x50/0x50 > ? ___neigh_create+0x4d5/0x680 > ? ip6_finish_output2+0x1b5/0x590 > ip6_finish_output2+0x1b5/0x590 > ? ip6_output+0x68/0x110 > ip6_output+0x68/0x110 > ? nf_hook.constprop.28+0x79/0xc0 > ndisc_send_skb+0x248/0x2e0 > ndisc_send_ns+0xf8/0x200 > ? addrconf_dad_work+0x389/0x4b0 > addrconf_dad_work+0x389/0x4b0 > ? __switch_to_asm+0x34/0x70 > ? process_one_work+0x195/0x380 > ? addrconf_dad_completed+0x370/0x370 > process_one_work+0x195/0x380 > worker_thread+0x30/0x390 > ? process_one_work+0x380/0x380 > kthread+0x113/0x130 > ? kthread_park+0x90/0x90 > ret_from_fork+0x35/0x40 > Modules linked in: act_connmark nf_conntrack nf_defrag_ipv6 nf_defrag_ip= v4 veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 crct10di= f_pclmul mbcache crc32_pclmul jbd2 snd_hda_codec_generic ghash_clmulni_inte= l snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device= snd_pcm aesni_intel snd_timer crypto_simd cryptd snd glue_helper joydev vi= rtio_balloon pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grac= e sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper virtio_net = net_failover syscopyarea virtio_blk failover virtio_console sysfillrect sys= imgblt fb_sys_fops ttm drm ata_piix crc32c_intel serio_raw libata virtio_pc= i virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod > CR2: 0000000000000000 > > Validating the control action within tcf_connmark_init() proved to fix th= e > above issue. A TDC selftest is added to verify the correct behavior. > > Fixes: db50514f9a9c ("net: sched: add termination action to allow goto ch= ain") > Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values") > Signed-off-by: Davide Caratti > --- > net/sched/act_connmark.c | 24 +++++++++++++++++- > .../tc-testing/tc-tests/actions/connmark.json | 25 +++++++++++++++++++ > 2 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c > index 30c4c109c80c..5e02f9f6850a 100644 > --- a/net/sched/act_connmark.c > +++ b/net/sched/act_connmark.c > @@ -21,6 +21,7 @@ > #include > #include > #include > +#include > #include > #include > =20 > @@ -101,10 +102,11 @@ static int tcf_connmark_init(struct net *net, struc= t nlattr *nla, > struct netlink_ext_ack *extack) > { > struct tc_action_net *tn =3D net_generic(net, connmark_net_id); > + struct tcf_chain *newchain =3D NULL, *oldchain; > struct nlattr *tb[TCA_CONNMARK_MAX + 1]; > struct tcf_connmark_info *ci; > struct tc_connmark *parm; > - int ret =3D 0; > + int ret =3D 0, err; > =20 > if (!nla) > return -EINVAL; > @@ -129,6 +131,12 @@ static int tcf_connmark_init(struct net *net, struct= nlattr *nla, > } > =20 > ci =3D to_connmark(*a); > + err =3D tcf_action_check_ctrlact(parm->action, tp, &newchain, > + extack); > + if (err) > + goto release_idr; > + oldchain =3D ci->tcf_goto_chain; > + ci->tcf_goto_chain =3D newchain; > ci->tcf_action =3D parm->action; > ci->net =3D net; > ci->zone =3D parm->zone; > @@ -143,15 +151,29 @@ static int tcf_connmark_init(struct net *net, struc= t nlattr *nla, > tcf_idr_release(*a, bind); > return -EEXIST; > } > + err =3D tcf_action_check_ctrlact(parm->action, tp, &newchain, > + extack); > + if (err) > + goto release_idr; > /* replacing action and zone */ > spin_lock_bh(&ci->tcf_lock); > + oldchain =3D ci->tcf_goto_chain; > + ci->tcf_goto_chain =3D newchain; > ci->tcf_action =3D parm->action; > ci->zone =3D parm->zone; > spin_unlock_bh(&ci->tcf_lock); > ret =3D 0; > + } else { > + goto end; Maybe just initialize oldchain to NULL to avoid this goto? > } > =20 > + if (oldchain) > + tcf_chain_put_by_act(oldchain); > +end: > return ret; > +release_idr: > + tcf_idr_release(*a, bind); > + return err; > } > =20 > static inline int tcf_connmark_dump(struct sk_buff *skb, struct tc_actio= n *a, > diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/connmark= .json b/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json > index 13147a1f5731..cadde8f41fcd 100644 > --- a/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json > +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json > @@ -287,5 +287,30 @@ > "teardown": [ > "$TC actions flush action connmark" > ] > + }, > + { > + "id": "c506", > + "name": "Replace connmark with invalid goto chain control", > + "category": [ > + "actions", > + "connmark" > + ], > + "setup": [ > + [ > + "$TC actions flush action connmark", > + 0, > + 1, > + 255 > + ], > + "$TC actions add action connmark pass index 90" > + ], > + "cmdUnderTest": "$TC actions replace action connmark goto chain = 42 index 90 cookie c1a0c1a0", > + "expExitCode": "255", > + "verifyCmd": "$TC actions get action connmark index 90", > + "matchPattern": "action order [0-9]+: connmark zone 0 pass.*inde= x 90 ref", > + "matchCount": "1", > + "teardown": [ > + "$TC actions flush action connmark" > + ] > } > ]