On Wed, Aug 8, 2018 at 4:41 AM Vlad Buslov <vla...@mellanox.com> wrote:
>
>
> On Tue 07 Aug 2018 at 23:26, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> > On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov <vla...@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.
