On Wed 08 Aug 2018 at 21:32, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> In tca_action_gd(), when tcf_action_get_1() fails in the middle
> of the loop, tcf_action_put_many(&actions[acts_deleted]) is
> called to cleanup.
>
> But inside tcf_action_put_many() it still iterates from
> 0 to TCA_ACT_MAX_PRIO, so inside it would be:
>
> &actions[acts_deleted][0]...&actions[acts_deleted][MAX_PRIO]
>
> Then the overall of the result is:
>
> actions[acts_deleted]...actions[acts_deleted + MAX_PRIO]
>
> We have a potential out-of-bound access when acts_deleted > 1.
>
> acts_deleted is completely unnecessary since tcf_action_put_many()
> checks against NULL pointer.
>
> Fixes: 90b73b77d08e ("net: sched: change action API to use array of pointers
> to actions")
> Cc: Jiri Pirko <j...@mellanox.com>
> Cc: Vlad Buslov <vla...@mellanox.com>
> Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
> ---
> net/sched/act_api.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 229d63c99be2..36549bc7ce78 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1176,7 +1176,7 @@ static int tca_action_flush(struct net *net, struct
> nlattr *nla,
> }
>
> static int tcf_action_delete(struct net *net, struct tc_action *actions[],
> - int *acts_deleted, struct netlink_ext_ack *extack)
> + struct netlink_ext_ack *extack)
> {
> u32 act_index;
> int ret, i;
> @@ -1196,20 +1196,16 @@ static int tcf_action_delete(struct net *net, struct
> tc_action *actions[],
> } else {
> /* now do the delete */
> ret = ops->delete(net, act_index);
> - if (ret < 0) {
> - *acts_deleted = i + 1;
> + if (ret < 0)
> return ret;
> - }
> }
> }
> - *acts_deleted = i;
> return 0;
> }
>
> static int
> tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action
> *actions[],
> - int *acts_deleted, u32 portid, size_t attr_size,
> - struct netlink_ext_ack *extack)
> + u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
> {
> int ret;
> struct sk_buff *skb;
> @@ -1227,7 +1223,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n,
> struct tc_action *actions[],
> }
>
> /* now do the delete */
> - ret = tcf_action_delete(net, actions, acts_deleted, extack);
> + ret = tcf_action_delete(net, actions, extack);
> if (ret < 0) {
> NL_SET_ERR_MSG(extack, "Failed to delete TC action");
> kfree_skb(skb);
> @@ -1250,7 +1246,6 @@ tca_action_gd(struct net *net, struct nlattr *nla,
> struct nlmsghdr *n,
> struct tc_action *act;
> size_t attr_size = 0;
> struct tc_action *actions[TCA_ACT_MAX_PRIO + 1] = {};
> - int acts_deleted = 0;
>
> ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
> if (ret < 0)
> @@ -1280,14 +1275,14 @@ tca_action_gd(struct net *net, struct nlattr *nla,
> struct nlmsghdr *n,
> if (event == RTM_GETACTION)
> ret = tcf_get_notify(net, portid, n, actions, event, extack);
> else { /* delete */
> - ret = tcf_del_notify(net, n, actions, &acts_deleted, portid,
> + ret = tcf_del_notify(net, n, actions, portid,
> attr_size, extack);
> if (ret)
> goto err;
> return ret;
> }
> err:
> - tcf_action_put_many(&actions[acts_deleted]);
> + tcf_action_put_many(actions);
> return ret;
> }
Hello Cong,
Before version V5 of my action API patchset this functionality was
implemented in exactly the same way as in your patch. Unfortunately, it
has a double-free bug. The problem is that if you have multiple
actions(N) being deleted, and deleted succeeded for first K actions,
this implementation will try to delete all N actions second time
(including first K actions that were already deleted). That is why I
added 'acts_deleted' variable that tracks actual amount of actions that
were deleted successfully, and only delete last N-K actions in case of
error.
In order to fix that issue I did following code changes in V5:
- Added 'acts_deleted' variable to delete only actions [K, N) in case of
error.
- Extended 'actions' array size by one to ensure that it always ends
with NULL pointer.
