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]

Message-ID: <20101019114933.GA857@redhat.com>
Date:	Tue, 19 Oct 2010 07:49:33 -0400
From:	Vivek Goyal <vgoyal@...hat.com>
To:	Jens Axboe <axboe@...nel.dk>
Cc:	Andi Kleen <andi@...stfloor.org>, torvalds@...ux-foundation.org,
	linux-kernel@...r.kernel.org, Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH] Fix array overflow in CFQ

On Tue, Oct 19, 2010 at 12:01:40PM +0200, Jens Axboe wrote:
> 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?!

Hi Jens,

busy_queues_avg[] definitely looks buggy. Looks like I introduced this bug
while converting corrado's logic to group logic. I will fix it in a while.
Sorry for the goof up here.

->service_trees[][] is not buggy. We maintain workload subclassification
only for RT and BE class. For IDLE class, there are no ASYNC_WORKLOAD,
SYNC_NOIDLE_WORKLOAD or SYNC_WORKLOAD. All the type of idle queues
go onto a separate service tree, service_tree_idle.

Thanks
Vivek

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