Re: [Patch net-next] net_sched: fix a potential out-of-bound access

[Vlad Buslov <vladbu@mellanox.com>]

On Wed 08 Aug 2018 at 21:32, Cong Wang <xiyou.wangcong@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 <jiri@mellanox.com>
> Cc: Vlad Buslov <vladbu@mellanox.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@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.