From: Vlad Buslov <vladbu@mellanox.com>
To: Davide Caratti <dcaratti@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Jamal Hadi Salim <jhs@mojatatu.com>,
Cong Wang <xiyou.wangcong@gmail.com>,
Jiri Pirko <jiri@resnulli.us>,
"pabeni@redhat.com" <pabeni@redhat.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH net 05/16] net/sched: act_ife: validate the control action inside init()
Date: Fri, 1 Mar 2019 16:33:29 +0000 [thread overview]
Message-ID: <vbfk1hid06k.fsf@mellanox.com> (raw)
In-Reply-To: <a51604fe9879237dad3f2417bec2b7c12061dba7.1551288982.git.dcaratti@redhat.com>
On Wed 27 Feb 2019 at 17:40, Davide Caratti <dcaratti@redhat.com> wrote:
> the following script:
>
> # tc qdisc add dev crash0 clsact
> # tc filter add dev crash0 egress matchall \
> > action ife encode allow mark pass index 90
> # tc actions replace action ife \
> > encode allow mark goto chain 42 index 90 cookie c1a0c1a0
> # tc action show action ife
>
> had the following output:
>
> IFE type 0xED3E
> IFE type 0xED3E
> Error: Failed to init TC action chain.
> We have an error talking to the kernel
> total acts 1
>
> action order 0: ife encode action goto chain 42 type 0XED3E
> allow mark
> 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 0000000000000000
> #PF error: [normal kernel read fault]
> PGD 800000007b4e7067 P4D 800000007b4e7067 PUD 7b4e6067 PMD 0
> Oops: 0000 [#1] SMP PTI
> CPU: 2 PID: 164 Comm: kworker/2:1 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 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
> RSP: 0018:ffffa6a7c0553ad0 EFLAGS: 00010246
> RAX: 000000002000002a RBX: ffff9796ee1bbd00 RCX: 0000000000000001
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: ffffa6a7c0553b70 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: ffff9797385bb038 R12: ffff9796ead9d700
> R13: ffff9796ead9d708 R14: 0000000000000001 R15: ffff9796ead9d800
> FS: 0000000000000000(0000) GS:ffff97973db00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 000000007c41e006 CR4: 00000000001606e0
> 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_gact act_meta_mark act_ife dummy veth ip6table_filter ip6_tables iptable_filter binfmt_misc snd_hda_codec_generic ext4 snd_hda_intel snd_hda_codec crct10dif_pclmul mbcache crc32_pclmul jbd2 snd_hwdep snd_hda_core ghash_clmulni_intel snd_seq snd_seq_device snd_pcm snd_timer aesni_intel crypto_simd snd cryptd glue_helper virtio_balloon joydev pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl virtio_net drm_kms_helper virtio_blk net_failover syscopyarea failover sysfillrect virtio_console sysimgblt fb_sys_fops ttm drm crc32c_intel serio_raw ata_piix virtio_pci virtio_ring libata virtio floppy dm_mirror dm_region_hash dm_log dm_mod [last unloaded: act_ife]
> CR2: 0000000000000000
>
> Validating the control action within tcf_ife_init() proved to fix the
> above issue. A TDC selftest is added to verify the correct behavior.
>
> Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
> Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/sched/act_ife.c | 32 ++++++++++++-------
> .../tc-testing/tc-tests/actions/ife.json | 25 +++++++++++++++
> 2 files changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
> index 9b2eb941e093..4480edb386d7 100644
> --- a/net/sched/act_ife.c
> +++ b/net/sched/act_ife.c
> @@ -29,6 +29,7 @@
> #include <net/net_namespace.h>
> #include <net/netlink.h>
> #include <net/pkt_sched.h>
> +#include <net/pkt_cls.h>
> #include <uapi/linux/tc_act/tc_ife.h>
> #include <net/tc_act/tc_ife.h>
> #include <linux/etherdevice.h>
> @@ -472,6 +473,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> struct tcf_proto *tp, struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, ife_net_id);
> + struct tcf_chain *newchain = NULL, *oldchain;
> struct nlattr *tb[TCA_IFE_MAX + 1];
> struct nlattr *tb2[IFE_META_MAX + 1];
> struct tcf_ife_params *p;
> @@ -531,6 +533,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> }
>
> ife = to_ife(*a);
> + err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
> + if (err)
> + goto release_idr;
> +
> p->flags = parm->flags;
>
> if (parm->flags & IFE_ENCODE) {
> @@ -563,13 +569,8 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> if (tb[TCA_IFE_METALST]) {
> err = nla_parse_nested(tb2, IFE_META_MAX, tb[TCA_IFE_METALST],
> NULL, NULL);
> - if (err) {
> -metadata_parse_err:
> - tcf_idr_release(*a, bind);
> - kfree(p);
> - return err;
> - }
> -
> + if (err)
> + goto metadata_parse_err;
> err = populate_metalist(ife, tb2, exists, rtnl_held);
> if (err)
> goto metadata_parse_err;
> @@ -581,15 +582,14 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> * going to bail out
> */
> err = use_all_metadata(ife, exists);
> - if (err) {
> - tcf_idr_release(*a, bind);
> - kfree(p);
> - return err;
> - }
> + if (err)
> + goto metadata_parse_err;
> }
>
> if (exists)
> spin_lock_bh(&ife->tcf_lock);
> + oldchain = ife->tcf_goto_chain;
> + ife->tcf_goto_chain = newchain;
> ife->tcf_action = parm->action;
> /* protected by tcf_lock when modifying existing action */
> rcu_swap_protected(ife->params, p, 1);
> @@ -603,6 +603,14 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> tcf_idr_insert(tn, *a);
>
oldchain is not released before return.
> return ret;
> +
> +metadata_parse_err:
> + if (newchain)
> + tcf_chain_put_by_act(newchain);
> +release_idr:
> + kfree(p);
> + tcf_idr_release(*a, bind);
> + return err;
> }
>
> static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json b/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json
> index 0da3545cabdb..c13a68b98fc7 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json
> @@ -1060,5 +1060,30 @@
> "matchPattern": "action order [0-9]*: ife encode action pipe.*allow prio.*index 4",
> "matchCount": "0",
> "teardown": []
> + },
> + {
> + "id": "a0e2",
> + "name": "Replace ife encode action with invalid goto chain control",
> + "category": [
> + "actions",
> + "ife"
> + ],
> + "setup": [
> + [
> + "$TC actions flush action ife",
> + 0,
> + 1,
> + 255
> + ],
> + "$TC actions add action ife encode allow mark pass index 90"
> + ],
> + "cmdUnderTest": "$TC actions replace action ife encode allow mark goto chain 42 index 90 cookie c1a0c1a0",
> + "expExitCode": "255",
> + "verifyCmd": "$TC actions get action ife index 90",
> + "matchPattern": "action order [0-9]*: ife encode action pass.*type 0[xX]ED3E .*allow mark.*index 90 ref",
> + "matchCount": "1",
> + "teardown": [
> + "$TC actions flush action ife"
> + ]
> }
> ]
next prev parent reply other threads:[~2019-03-01 16:33 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
2019-02-27 17:40 ` [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action Davide Caratti
2019-02-28 1:28 ` Cong Wang
2019-03-01 17:59 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 02/16] net/sched: act_bpf: validate the control action inside init() Davide Caratti
2019-02-27 17:40 ` [PATCH net 03/16] net/sched: act_csum: " Davide Caratti
2019-02-28 1:50 ` Cong Wang
2019-03-01 18:02 ` Davide Caratti
2019-03-02 0:04 ` Cong Wang
2019-03-07 13:56 ` Davide Caratti
2019-03-07 14:51 ` Vlad Buslov
2019-03-07 16:56 ` Davide Caratti
2019-03-08 17:47 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 04/16] net/sched: act_gact: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 05/16] net/sched: act_ife: " Davide Caratti
2019-03-01 16:33 ` Vlad Buslov [this message]
2019-02-27 17:40 ` [PATCH net 06/16] net/sched: act_mirred: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 07/16] net/sched: act_connmark: " Davide Caratti
2019-03-01 16:35 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 08/16] net/sched: act_nat: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 09/16] net/sched: act_pedit: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 10/16] net/sched: act_police: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 11/16] net/sched: act_sample: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 12/16] net/sched: act_simple: " Davide Caratti
2019-03-01 16:39 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 13/16] net/sched: act_skbedit: " Davide Caratti
2019-03-01 16:43 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 14/16] net/sched: act_skbmod: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 15/16] net/sched: act_tunnel_key: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 16/16] net/sched: act_vlan: " Davide Caratti
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=vbfk1hid06k.fsf@mellanox.com \
--to=vladbu@mellanox.com \
--cc=davem@davemloft.net \
--cc=dcaratti@redhat.com \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=xiyou.wangcong@gmail.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.