From mboxrd@z Thu Jan  1 00:00:00 1970
From: Cong Wang <xiyou.wangcong@gmail.com>
Subject: Re: [PATCH net-next v6 11/11] net: sched: change action API to use
 array of pointers to actions
Date: Wed, 8 Aug 2018 11:29:40 -0700
Message-ID: <CAM_iQpW9_hh8XJOJsMESbOKdGU_qAZ-tQw9n_HrdXTFLTDUCDg@mail.gmail.com>
References: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
 <1530800673-12280-12-git-send-email-vladbu@mellanox.com> <CAM_iQpWpxf=yRNadgN+q0rwhd_E0c7pAy7fG+A=asqgk9JRx0Q@mail.gmail.com>
 <vbfr2j92ii3.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
        David Miller <davem@davemloft.net>,
        Jamal Hadi Salim <jhs@mojatatu.com>,
        Jiri Pirko <jiri@resnulli.us>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Yevgeny Kliteynik <kliteyn@mellanox.com>
To: Vlad Buslov <vladbu@mellanox.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pf1-f196.google.com ([209.85.210.196]:41780 "EHLO
        mail-pf1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1727295AbeHHUrw (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 8 Aug 2018 16:47:52 -0400
Received: by mail-pf1-f196.google.com with SMTP id y10-v6so1540791pfn.8
        for <netdev@vger.kernel.org>; Wed, 08 Aug 2018 11:26:57 -0700 (PDT)
In-Reply-To: <vbfr2j92ii3.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Wed, Aug 8, 2018 at 4:41 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Tue 07 Aug 2018 at 23:26, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>         attr_size = tcf_action_full_attrs_size(attr_size);
> >>
> >>         if (event == RTM_GETACTION)
> >> -               ret = tcf_get_notify(net, portid, n, &actions, event, extack);
> >> +               ret = tcf_get_notify(net, portid, n, actions, event, extack);
> >>         else { /* delete */
> >> -               ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
> >> +               ret = tcf_del_notify(net, n, actions, &acts_deleted, portid,
> >> +                                    attr_size, extack);
> >>                 if (ret)
> >>                         goto err;
> >>                 return ret;
> >>         }
> >>  err:
> >> -       tcf_action_put_lst(&actions);
> >> +       tcf_action_put_many(&actions[acts_deleted]);
> >>         return ret;
> >
> > How does this even work?
> >
> > You save an index in 'acts_deleted', but you pass &actions[acts_deleted]
> > to tcf_action_put_many(), which seems you want to start from
> > where it fails, but inside tcf_action_put_many() it starts from 0
> > to TCA_ACT_MAX_PRIO, out-of-bound access at least?
>
> Actions array is declared to be TCA_ACT_MAX_PRIO+1 in size, and


Declaration doesn't matter at all, functions see it as a pure pointer
once you pass it as an argument.


> initialized to NULL pointers. In loop inside tcf_action_put_many() there
> are two checks: One is that index is less than TCA_ACT_MAX_PRIO and
> another one that pointer is not NULL. In this case I rely on extra NULL
> pointer at the end of actions array to prevent out-of-bound access.

True, but you pass &actions[acts_deleted] as the start of the array,
so inside it would be:

&actions[acts_deleted][0]...&actions[acts_deleted][MAX_PRIO]

So, the overall of the result is:

actions[acts_deleted]...actions[acts_deleted + MAX_PRIO]

You have out-of-bound access when acts_deleted > 1.

And if acts_deleted == MAX_PRIO-1, then you don't have any
NULL pointer to rely on.