netdev - [PATCH net-next] net: sched: fix use before alloc of per cpu stats

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]

Message-Id: <20180115055222.4248-1-bhole_prashant_q7@lab.ntt.co.jp>
Date:   Mon, 15 Jan 2018 14:52:22 +0900
From:   Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
To:     "David S . Miller" <davem@...emloft.net>
Cc:     Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org
Subject: [PATCH net-next] net: sched: fix use before alloc of per cpu stats

About bug:
During init of clsact/ingress, it links qdisc's cpu_bstats,cpu_qstats
with mini qdisc. TCQ_F_CPUSTATS is set in qdisc->flags during init and
this flag is checked after init to allocate memory for stats.

Hence mini qdisc points to null per-cpu-stats. The problem isn't caught
while updating stats via mini qdisc because per_cpu_ptr(NULL, cpu_num)
or this_cpu_ptr(NULL) gives a valid pointer.

About fix:
Currently stats memory is allocated at two places.
- in qdisc_alloc() if TCQ_F_CPUSTATS is set in Qdisc_ops->static_flags
- in qdisc_create() if TCQ_F_CPUSTATS is set in Qdisc->flags

Qdisc_ops->static_flags is propagated to Qdisc->flags. So to fix this bug,
we set TCQ_F_CPUSTATS in static flags and additional condition to avoid
allocation after init.

Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath")
Signed-off-by: Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
---
 net/sched/sch_api.c     | 3 ++-
 net/sched/sch_ingress.c | 6 ++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 8a04c36e579f..de99a5e80944 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1094,7 +1094,8 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
 			goto err_out5;
 	}
 
-	if (qdisc_is_percpu_stats(sch)) {
+	if (!(ops->static_flags & TCQ_F_CPUSTATS) &&
+	    qdisc_is_percpu_stats(sch)) {
 		sch->cpu_bstats =
 			netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu);
 		if (!sch->cpu_bstats)
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 7ca2be20dd6f..0a3fba46dfd3 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -82,8 +82,6 @@ static int ingress_init(struct Qdisc *sch, struct nlattr *opt,
 	if (err)
 		return err;
 
-	sch->flags |= TCQ_F_CPUSTATS;
-
 	return 0;
 }
 
@@ -127,6 +125,7 @@ static struct Qdisc_ops ingress_qdisc_ops __read_mostly = {
 	.destroy	=	ingress_destroy,
 	.dump		=	ingress_dump,
 	.owner		=	THIS_MODULE,
+	.static_flags	=	TCQ_F_CPUSTATS,
 };
 
 struct clsact_sched_data {
@@ -202,8 +201,6 @@ static int clsact_init(struct Qdisc *sch, struct nlattr *opt,
 	if (err)
 		return err;
 
-	sch->flags |= TCQ_F_CPUSTATS;
-
 	return 0;
 }
 
@@ -235,6 +232,7 @@ static struct Qdisc_ops clsact_qdisc_ops __read_mostly = {
 	.destroy	=	clsact_destroy,
 	.dump		=	ingress_dump,
 	.owner		=	THIS_MODULE,
+	.static_flags	=	TCQ_F_CPUSTATS,
 };
 
 static int __init ingress_module_init(void)
-- 
2.13.6