On Thu, Apr 4, 2019 at 1:32 PM Davide Caratti <dcara...@redhat.com> wrote:
>
> the control path of 'sample' action does not validate the value of 'rate'
> provided by the user, but then it uses it as divisor in the traffic path.
> Validate it in tcf_sample_init(), and return -EINVAL with a proper extack
> message in case that value is zero, to fix a splat with the script below:
>
> # tc f a dev test0 egress matchall action sample rate 0 group 1 index 2
> # tc -s a s action sample
> total acts 1
>
> action order 0: sample rate 1/0 group 1 pipe
> index 2 ref 1 bind 1 installed 19 sec used 19 sec
> Action statistics:
> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> # ping 192.0.2.1 -I test0 -c1 -q
>
> divide error: 0000 [#1] SMP PTI
> CPU: 1 PID: 6192 Comm: ping Not tainted 5.1.0-rc2.diag2+ #591
> Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> RIP: 0010:tcf_sample_act+0x9e/0x1e0 [act_sample]
> Code: 6a f1 85 c0 74 0d 80 3d 83 1a 00 00 00 0f 84 9c 00 00 00 4d 85 e4 0f
> 84 85 00 00 00 e8 9b d7 9c f1 44 8b 8b e0 00 00 00 31 d2 <41> f7 f1 85 d2 75
> 70 f6 85 83 00 00 00 10 48 8b 45 10 8b 88 08 01
> RSP: 0018:ffffae320190ba30 EFLAGS: 00010246
> RAX: 00000000b0677d21 RBX: ffff8af1ed9ec000 RCX: 0000000059a9fe49
> RDX: 0000000000000000 RSI: 000000000c7e33b7 RDI: ffff8af23daa0af0
> RBP: ffff8af1ee11b200 R08: 0000000074fcaf7e R09: 0000000000000000
> R10: 0000000000000050 R11: ffffffffb3088680 R12: ffff8af232307f80
> R13: 0000000000000003 R14: ffff8af1ed9ec000 R15: 0000000000000000
> FS: 00007fe9c6d2f740(0000) GS:ffff8af23da80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fff6772f000 CR3: 00000000746a2004 CR4: 00000000001606e0
> Call Trace:
> tcf_action_exec+0x7c/0x1c0
> tcf_classify+0x57/0x160
> __dev_queue_xmit+0x3dc/0xd10
> ip_finish_output2+0x257/0x6d0
> ip_output+0x75/0x280
> ip_send_skb+0x15/0x40
> raw_sendmsg+0xae3/0x1410
> sock_sendmsg+0x36/0x40
> __sys_sendto+0x10e/0x140
> __x64_sys_sendto+0x24/0x30
> do_syscall_64+0x60/0x210
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [...]
> Kernel panic - not syncing: Fatal exception in interrupt
>
> Add a TDC selftest to document that 'rate' is now being validated.
>
> Reported-by: Matteo Croce <mcr...@redhat.com>
> Fixes: 5c5670fae430 ("net/sched: Introduce sample tc action")
> Signed-off-by: Davide Caratti <dcara...@redhat.com>
Acked-by: Yotam Gigi <yotam...@gmail.com>
Thanks!
> ---
> net/sched/act_sample.c | 10 ++++++--
> .../tc-testing/tc-tests/actions/sample.json | 24 +++++++++++++++++++
> 2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
> index 4060b0955c97..0f82d50ea232 100644
> --- a/net/sched/act_sample.c
> +++ b/net/sched/act_sample.c
> @@ -45,8 +45,8 @@ static int tcf_sample_init(struct net *net, struct nlattr
> *nla,
> struct nlattr *tb[TCA_SAMPLE_MAX + 1];
> struct psample_group *psample_group;
> struct tcf_chain *goto_ch = NULL;
> + u32 psample_group_num, rate;
> struct tc_sample *parm;
> - u32 psample_group_num;
> struct tcf_sample *s;
> bool exists = false;
> int ret, err;
> @@ -85,6 +85,12 @@ static int tcf_sample_init(struct net *net, struct nlattr
> *nla,
> if (err < 0)
> goto release_idr;
>
> + rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
> + if (!rate) {
> + NL_SET_ERR_MSG(extack, "invalid sample rate");
> + err = -EINVAL;
> + goto put_chain;
> + }
> psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
> psample_group = psample_group_get(net, psample_group_num);
> if (!psample_group) {
> @@ -96,7 +102,7 @@ static int tcf_sample_init(struct net *net, struct nlattr
> *nla,
>
> spin_lock_bh(&s->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> - s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
> + s->rate = rate;
> s->psample_group_num = psample_group_num;
> RCU_INIT_POINTER(s->psample_group, psample_group);
>
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/sample.json
> b/tools/testing/selftests/tc-testing/tc-tests/actions/sample.json
> index 27f0acaed880..ddabb160a11b 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/actions/sample.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/sample.json
> @@ -143,6 +143,30 @@
> "$TC actions flush action sample"
> ]
> },
> + {
> + "id": "7571",
> + "name": "Add sample action with invalid rate",
> + "category": [
> + "actions",
> + "sample"
> + ],
> + "setup": [
> + [
> + "$TC actions flush action sample",
> + 0,
> + 1,
> + 255
> + ]
> + ],
> + "cmdUnderTest": "$TC actions add action sample rate 0 group 1 index
> 2",
> + "expExitCode": "255",
> + "verifyCmd": "$TC actions get action sample index 2",
> + "matchPattern": "action order [0-9]+: sample rate 1/0 group 1.*index
> 2 ref",
> + "matchCount": "0",
> + "teardown": [
> + "$TC actions flush action sample"
> + ]
> + },
> {
> "id": "b6d4",
> "name": "Add sample action with mandatory arguments and invalid
> control action",
> --
> 2.20.1
>
