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

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

[Radu Rendec]

The (percpu) cacheinfo array is allocated based on the number of cache
leaves that is determined by fetch_cache_info(). When the device tree or
the ACPI tables provide information about the cache, that information is
used to determine the number of leaves. However, for arm64 systems, the
cacheinfo array is filled later in populate_cache_leaves() based on the
information from the CLIDR_EL1 register. If this information happens to
be inconsistent with the device tree or ACPI tables, and if the number
of cache leaves exposed by CLIDR_EL1 is larger than the pre-determined
number, populate_cache_leaves() happily writes past the allocated struct
cacheinfo array.

An example is the Renesas R-Car S4 Spider board, where the cache
information is described in arch/arm64/boot/dts/renesas/r8a779f0.dtsi.
The CPU nodes contain no explicit info (other than `next-level-cache`),
so of_count_cache_leaves() defaults to 2. The next cache level has an
explicit `cache-level` property equal to 3 and `cache-unified`, so at
the first iteration in init_of_cache_level(), the level is bumped to 3
(from 1) and the leaves count is incremented to 3. So, it ends up with
a total of 3 levels and 3 leaves. Essentially, the device tree describes
a split L1 cache, no L2 cache and a unified L3 cache, and that's
consistent with the technical reference manual. However, according to
CLIDR_EL1, the unified cache is at L2, and there is no L3. So, the code
in populate_cache_leaves() populates the 3 leaves for L1 and L2, but it
also examines L3 (for which it finds CACHE_TYPE_NOCACHE) and attempts to
populate a 4th leaf, which hasn't been allocated.

This is easily observed on boot if KASAN is enabled:
8<----------------------------------------------------------------------
BUG: KASAN: slab-out-of-bounds in populate_cache_leaves+0x314/0x388
Write of size 4 at addr ffff0004402e16c4 by task swapper/0/1

CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.13.0-rc6+ #14
Hardware name: Renesas Spider CPU and Breakout boards based on r8a779f0 (DT)
Call trace:
 show_stack+0x34/0x98 (C)
 dump_stack_lvl+0x7c/0xa0
 print_address_description.constprop.0+0x90/0x320
 print_report+0x108/0x1f0
 kasan_report+0xb4/0x100
 __asan_report_store4_noabort+0x20/0x30
 populate_cache_leaves+0x314/0x388
 detect_cache_attributes+0x2f0/0x440
 update_siblings_masks+0x30/0x5b0
 store_cpu_topology+0x154/0x1d8
 smp_prepare_cpus+0x34/0x180
 kernel_init_freeable+0x338/0x480
 kernel_init+0x28/0x170
 ret_from_fork+0x10/0x20

Allocated by task 1:
 kasan_save_stack+0x2c/0x58
 kasan_save_track+0x20/0x40
 kasan_save_alloc_info+0x40/0x60
 __kasan_kmalloc+0xd4/0xd8
 __kmalloc_noprof+0x190/0x4b0
 allocate_cache_info+0x90/0x188
 fetch_cache_info+0x320/0x408
 init_cpu_topology+0x318/0x398
 smp_prepare_cpus+0x28/0x180
 kernel_init_freeable+0x338/0x480
 kernel_init+0x28/0x170
 ret_from_fork+0x10/0x20
8<----------------------------------------------------------------------

The loop that detects/populates cache information already has a bounds
check on the array size but does not account for cache levels with
separate data/instructions cache. Fix this by incrementing the index
for any populated leaf (instead of any populated level).

Fixes: 5d425c186537 ("arm64: kernel: add support for cpu cache information")

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 arch/arm64/kernel/cacheinfo.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

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;
+			ci_leaf_init(&infos[idx++], CACHE_TYPE_DATA, level);
+			ci_leaf_init(&infos[idx++], CACHE_TYPE_INST, level);
 		} else {
-			ci_leaf_init(this_leaf++, type, level);
+			ci_leaf_init(&infos[idx++], type, level);
 		}
 	}
 	return 0;
-- 
2.47.1

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

[Will Deacon]

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'?

Will

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

[Radu Rendec]

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.

-- 
Thanks,
Radu

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

[Will Deacon]

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].

Thanks,

Will

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

[Radu Rendec]

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