From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Buslov Subject: Re: [Patch net-next] net_sched: fix a potential out-of-bound access Date: Thu, 09 Aug 2018 10:32:22 +0300 Message-ID: References: <20180808213256.16425-1-xiyou.wangcong@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Cc: netdev@vger.kernel.org, Jiri Pirko To: Cong Wang Return-path: Received: from mail-eopbgr70047.outbound.protection.outlook.com ([40.107.7.47]:8736 "EHLO EUR04-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727237AbeHIJ4I (ORCPT ); Thu, 9 Aug 2018 05:56:08 -0400 In-reply-to: <20180808213256.16425-1-xiyou.wangcong@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Wed 08 Aug 2018 at 21:32, Cong Wang 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 > Cc: Vlad Buslov > Signed-off-by: Cong Wang > --- > 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.