Re: [PATCH net-next v6 11/11] net: sched: change action API to use array of pointers to actions

[Cong Wang <xiyou.wangcong@gmail.com>]

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.