linux-kernel - Re: [PATCH] Fix array overflow in CFQ

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-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 Oct 2010 12:01:40 +0200
From:	Jens Axboe <axboe@...nel.dk>
To:	Andi Kleen <andi@...stfloor.org>
CC:	torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org,
	Andi Kleen <ak@...ux.intel.com>,
	Vivek Goyal <vgoyal@...hat.com>
Subject: Re: [PATCH] Fix array overflow in CFQ

On 2010-10-19 11:10, Andi Kleen wrote:
> From: Andi Kleen <ak@...ux.intel.com>
> 
> gcc 4.5 complains when compiling a recent rc with
> 
> linux/block/cfq-iosched.c: In function ‘cfq_dispatch_requests’:
> linux/block/cfq-iosched.c:2156:3: warning: array subscript is above array bounds
> 
> and it is right:
> 
>  slice = group_slice * count /
>                 max_t(unsigned, cfqg->busy_queues_avg[cfqd->serving_prio],
>                       cfq_group_busy_queues_wl(cfqd->serving_prio, cfqd, cfqg));
> 
> busy_queues_avg can be indexed by this enum
> 
> enum wl_prio_t {
>         BE_WORKLOAD = 0,
>         RT_WORKLOAD = 1,
>         IDLE_WORKLOAD = 2,
> };
> 
> in cfqd->serving_prio, but is only declared as
> 
> unsigned int busy_queues_avg[2];
> 
> which is clearly off by one. Fix this here.

Indeed, that is definitely buggy. ->service_trees[][] looks buggy, too.
WTF?!

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 9eba291..8ce9f52 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -160,6 +160,7 @@ enum wl_prio_t {
 	BE_WORKLOAD = 0,
 	RT_WORKLOAD = 1,
 	IDLE_WORKLOAD = 2,
+	CFQ_PRIO_NR,
 };
 
 /*
@@ -168,7 +169,8 @@ enum wl_prio_t {
 enum wl_type_t {
 	ASYNC_WORKLOAD = 0,
 	SYNC_NOIDLE_WORKLOAD = 1,
-	SYNC_WORKLOAD = 2
+	SYNC_WORKLOAD = 2,
+	CFQ_TYPE_NR,
 };
 
 /* This is per cgroup per device grouping structure */
@@ -185,12 +187,12 @@ struct cfq_group {
 	int nr_cfqq;
 
 	/* Per group busy queus average. Useful for workload slice calc. */
-	unsigned int busy_queues_avg[2];
+	unsigned int busy_queues_avg[CFQ_PRIO_NR];
 	/*
 	 * rr lists of queues with requests, onle rr for each priority class.
 	 * Counts are embedded in the cfq_rb_root
 	 */
-	struct cfq_rb_root service_trees[2][3];
+	struct cfq_rb_root service_trees[CFQ_PRIO_NR][CFQ_TYPE_NR];
 	struct cfq_rb_root service_tree_idle;
 
 	unsigned long saved_workload_slice;

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/