Re: [PATCH net] net/sched: act_sample: fix divide by zero in the traffic path

[yotam gigi <yotam.gi@gmail.com>]

On Thu, Apr 4, 2019 at 1:32 PM Davide Caratti <dcaratti@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 <mcroce@redhat.com>
> Fixes: 5c5670fae430 ("net/sched: Introduce sample tc action")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>

Acked-by: Yotam Gigi <yotam.gi@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
>