Re: [PATCH] arm64: cacheinfo: Avoid out-of-bounds write to cacheinfo array

[Radu Rendec <rrendec@redhat.com>]

On Thu, 2025-02-06 at 13:02 +0000, Will Deacon wrote:
> On Tue, Feb 04, 2025 at 10:55:53AM -0500, Radu Rendec wrote:
> > On Tue, 2025-02-04 at 12:39 +0000, Will Deacon wrote:
> > > On Thu, Jan 23, 2025 at 01:11:59PM -0500, Radu Rendec wrote:
> > > > diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
> > > > index d9c9218fa1fdd..77ffda7284754 100644
> > > > --- a/arch/arm64/kernel/cacheinfo.c
> > > > +++ b/arch/arm64/kernel/cacheinfo.c
> > > > @@ -101,16 +101,18 @@ int populate_cache_leaves(unsigned int cpu)
> > > >  	unsigned int level, idx;
> > > >  	enum cache_type type;
> > > >  	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> > > > -	struct cacheinfo *this_leaf = this_cpu_ci->info_list;
> > > > +	struct cacheinfo *infos = this_cpu_ci->info_list;
> > > >  
> > > >  	for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
> > > > -	     idx < this_cpu_ci->num_leaves; idx++, level++) {
> > > > +	     idx < this_cpu_ci->num_leaves; level++) {
> > > >  		type = get_cache_type(level);
> > > >  		if (type == CACHE_TYPE_SEPARATE) {
> > > > -			ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
> > > > -			ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> > > > +			if (idx + 2 > this_cpu_ci->num_leaves)
> > > > +				break;
> > > 
> > > Why are you checking 'idx + 2' rather than 'idx + 1'?
> > 
> > I don't like "magic constants", and I thought "idx + 2" would be more
> > suggestive since 2 elements were added.
> > 
> > The check is correct though. For example, if this_cpu_ci->num_leaves = 3
> > (the array size is 3) and idx = 2, then 2 + 2 > 3 is true, so you can't
> > add two more elements. On the other hand, if idx = 1, then 1 + 2 > 3 is
> > false, so you can add the two elements (at indices 1 and 2).
> > 
> > If there's a strong preference for "idx + 1", I can change it. But then
> > of course ">" will need to change to ">=" as well.
> 
> Might just be me, but I'd personally find that clearer given that we're
> assigning to infos[idx] and infos[idx + 1].

Fair enough. I don't feel strongly about it, and since nobody else has
chimed in, I will make the change and post v2.

-- 
Thanks,
Radu