Re: [PATCH] sched/psi: Bounds-check state iterator against NR_PSI_STATES

[Kees Cook <keescook@chromium.org>]

On Mon, May 23, 2022 at 10:19:22AM -0400, Johannes Weiner wrote:
> On Fri, May 20, 2022 at 09:58:26AM -0700, Kees Cook wrote:
> > GCC 12 cannot tell that "t" will be bounded by NR_PSI_STATES, which could
> > lead to walking off the end of the tasks array, which is NR_PSI_STATES in
> > size. Explicitly bounds-check "t" as part of the loop.
> > 
> > In file included from ../kernel/sched/build_utility.c:97:
> > ../kernel/sched/psi.c: In function 'psi_group_change':
> > ../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds]
> >   730 |                         groupc->tasks[t]++;
> >       |                         ~~~~~~~~~~~~~^~~
> > In file included from ../include/linux/psi.h:6,
> >                  from ../kernel/sched/build_utility.c:36:
> > ../include/linux/psi_types.h:84:22: note: while referencing 'tasks'
> >    84 |         unsigned int tasks[NR_PSI_TASK_COUNTS];
> >       |                      ^~~~~
> > ../kernel/sched/psi.c:730:38: warning: array subscript 32 is above array bounds of 'unsigned int[5]' [-Warray-bounds]
> >   730 |                         groupc->tasks[t]++;
> >       |                         ~~~~~~~~~~~~~^~~
> > ../include/linux/psi_types.h:84:22: note: while referencing 'tasks'
> >    84 |         unsigned int tasks[NR_PSI_TASK_COUNTS];
> >       |                      ^~~~~
> > 
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Juri Lelli <juri.lelli@redhat.com>
> > Cc: Vincent Guittot <vincent.guittot@linaro.org>
> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Ben Segall <bsegall@google.com>
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> > Cc: Valentin Schneider <vschneid@redhat.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  kernel/sched/psi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> > index a337f3e35997..827f16a79936 100644
> > --- a/kernel/sched/psi.c
> > +++ b/kernel/sched/psi.c
> > @@ -725,7 +725,7 @@ static void psi_group_change(struct psi_group *group, int cpu,
> >  		}
> >  	}
> >  
> > -	for (t = 0; set; set &= ~(1 << t), t++)
> > +	for (t = 0; set && t < ARRAY_SIZE(groupc->tasks); set &= ~(1 << t), t++)
> >  		if (set & (1 << t))
> >  			groupc->tasks[t]++;
> 
> This is a very hot path, it runs for every nested cgroup on every task
> switch, wakeup and sleep. We should avoid unnecessary instructions and
> branches if we can help it at all.
> 
> Does the below patch address the warning for you? I can't test it
> myself, because I'm not getting it with gcc version 12.1.0. It's also
> odd that it didn't warn you about the loop over `clear' a few lines
> up, which ostensibly has the same "problem".

It seems this is a GCC 12 bug. Array indexes that have been used with a
shift end up miscalculated when building also with -fsanitize=shift
(i.e. allmodconfig):

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679


> 
> ---
> 
> diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
> index c7fe7c089718..113861343733 100644
> --- a/include/linux/psi_types.h
> +++ b/include/linux/psi_types.h
> @@ -41,6 +41,7 @@ enum psi_task_count {
>  #define TSK_RUNNING	(1 << NR_RUNNING)
>  #define TSK_ONCPU	(1 << NR_ONCPU)
>  #define TSK_MEMSTALL_RUNNING	(1 << NR_MEMSTALL_RUNNING)
> +#define TSK_MASK	((1U << NR_PSI_TASK_COUNTS) - 1)
>  
>  /* Resources that workloads could be stalled on */
>  enum psi_res {
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index a4fa3aadfcba..fb7fd40af337 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -804,6 +804,8 @@ void psi_task_change(struct task_struct *task, int clear, int set)
>  	void *iter = NULL;
>  	u64 now;
>  
> +	WARN_ON_ONCE((clear|set) & ~TSK_MASK);
> +
>  	if (!task->pid)
>  		return;

I'll give this a spin.

Thanks!

-Kees

-- 
Kees Cook