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

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

[Cong Wang]

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;
 }
 
-- 
2.14.4

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

[Vlad Buslov]

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.

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

[Cong Wang]

On Thu, Aug 9, 2018 at 12:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
> 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.

Interesting, I didn't notice you call it for tcf_del_notify()'s failure too.

But this is easy to resolve, we can just set succeeded ones to NULL
and teach tcf_action_put_many() to scan the whole array but
skip NULL's.


>
> 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.

Oh, I see, this is not how we use C, you can at least rollback
by passing acts_deleted as a parameter as the start of the array.
You picked the most confusing way to handle it.

I will send an updated patch.

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

[Vlad Buslov]

On Thu 09 Aug 2018 at 21:43, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Aug 9, 2018 at 12:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> 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.
>
> Interesting, I didn't notice you call it for tcf_del_notify()'s failure too.
>
> But this is easy to resolve, we can just set succeeded ones to NULL
> and teach tcf_action_put_many() to scan the whole array but
> skip NULL's.

Both approaches have their own tradeoffs. I considered just skipping
NULLs, but in my experience such approach might introduce hard to debug
bugs when all callers are expected to have either valid pointers or
NULLs in whole array. Then someone forgets to zero unused elements, but
it continues to work because stack memory just happens to be 0. Until
some code path uses same stack space before and leaves some junk there
instead of convenient zeroes...

Also, it becomes more computationally expensive to always scan whole
array for non-NULL pointers, instead of exiting when first NULL is
encountered. I don't think it is a major concert for current
implementation with MAX_PRIO==32, just a general preference of mine.

>
>
>>
>> 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.
>
> Oh, I see, this is not how we use C, you can at least rollback
> by passing acts_deleted as a parameter as the start of the array.
> You picked the most confusing way to handle it.

Noted. I will refrain from passing pointers to arbitrary array elements
as beginning-of-array from now on.

>
> I will send an updated patch.