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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Wed,  8 Aug 2018 14:32:56 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     netdev@...r.kernel.org
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Vlad Buslov <vladbu@...lanox.com>
Subject: [Patch net-next] net_sched: fix a potential out-of-bound access

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@...lanox.com>
Cc: Vlad Buslov <vladbu@...lanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@...il.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