[PATCH] perf/x86/uncore: avoid null-ptr-deref on error in pmu_alloc_topology

[PATCH] perf/x86/uncore: avoid null-ptr-deref on error in pmu_alloc_topology

[Fedor Pchelkin]

If topology[die] array allocation fails then topology[die][idx] elements
can't be accessed on error path.

Checking this on the error path probably looks more readable than
decrementing the counter in the allocation loop.

Found by Linux Verification Center (linuxtesting.org).

Fixes: 4d13be8ab5d4 ("perf/x86/intel/uncore: Generalize IIO topology support")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 arch/x86/events/intel/uncore_snbep.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index a96496bef678..7d4deb9126be 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -3831,9 +3831,11 @@ static int pmu_alloc_topology(struct intel_uncore_type *type, int topology_type)
 	return 0;
 clear:
 	for (; die >= 0; die--) {
-		for (idx = 0; idx < type->num_boxes; idx++)
-			kfree(topology[die][idx].untyped);
-		kfree(topology[die]);
+		if (topology[die]) {
+			for (idx = 0; idx < type->num_boxes; idx++)
+				kfree(topology[die][idx].untyped);
+			kfree(topology[die]);
+		}
 	}
 	kfree(topology);
 err:
-- 
2.39.2

Re: [PATCH] perf/x86/uncore: avoid null-ptr-deref on error in pmu_alloc_topology

[Liang, Kan]

On 2024-02-04 8:48 a.m., Fedor Pchelkin wrote:
> If topology[die] array allocation fails then topology[die][idx] elements
> can't be accessed on error path.
> 
> Checking this on the error path probably looks more readable than
> decrementing the counter in the allocation loop.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Fixes: 4d13be8ab5d4 ("perf/x86/intel/uncore: Generalize IIO topology support")
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---

It seems the code just jumps to the wrong kfree on the error path.
Does the below patch work?

diff --git a/arch/x86/events/intel/uncore_snbep.c
b/arch/x86/events/intel/uncore_snbep.c
index 8250f0f59c2b..5481fd00d861 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -3808,7 +3808,7 @@ static int pmu_alloc_topology(struct
intel_uncore_type *type, int topology_type)
 	for (die = 0; die < uncore_max_dies(); die++) {
 		topology[die] = kcalloc(type->num_boxes, sizeof(**topology), GFP_KERNEL);
 		if (!topology[die])
-			goto clear;
+			goto free_topology;
 		for (idx = 0; idx < type->num_boxes; idx++) {
 			topology[die][idx].untyped = kcalloc(type->num_boxes,
 							     topology_size[topology_type],
@@ -3827,6 +3827,7 @@ static int pmu_alloc_topology(struct
intel_uncore_type *type, int topology_type)
 			kfree(topology[die][idx].untyped);
 		kfree(topology[die]);
 	}
+free_topology:
 	kfree(topology);
 err:
 	return -ENOMEM;

Thanks,
Kan

>  arch/x86/events/intel/uncore_snbep.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index a96496bef678..7d4deb9126be 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -3831,9 +3831,11 @@ static int pmu_alloc_topology(struct intel_uncore_type *type, int topology_type)
>  	return 0;
>  clear:
>  	for (; die >= 0; die--) {
> -		for (idx = 0; idx < type->num_boxes; idx++)
> -			kfree(topology[die][idx].untyped);
> -		kfree(topology[die]);
> +		if (topology[die]) {
> +			for (idx = 0; idx < type->num_boxes; idx++)
> +				kfree(topology[die][idx].untyped);
> +			kfree(topology[die]);
> +		}
>  	}
>  	kfree(topology);
>  err:

Re: Re: [PATCH] perf/x86/uncore: avoid null-ptr-deref on error in pmu_alloc_topology

[Fedor Pchelkin]

Hello,

On 24/02/05 10:08AM, Liang, Kan wrote:
> 
> 
> On 2024-02-04 8:48 a.m., Fedor Pchelkin wrote:
> > If topology[die] array allocation fails then topology[die][idx] elements
> > can't be accessed on error path.
> > 
> > Checking this on the error path probably looks more readable than
> > decrementing the counter in the allocation loop.
> > 
> > Found by Linux Verification Center (linuxtesting.org).
> > 
> > Fixes: 4d13be8ab5d4 ("perf/x86/intel/uncore: Generalize IIO topology support")
> > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> > ---
> 
> It seems the code just jumps to the wrong kfree on the error path.
> Does the below patch work?
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c
> b/arch/x86/events/intel/uncore_snbep.c
> index 8250f0f59c2b..5481fd00d861 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -3808,7 +3808,7 @@ static int pmu_alloc_topology(struct
> intel_uncore_type *type, int topology_type)
>  	for (die = 0; die < uncore_max_dies(); die++) {
>  		topology[die] = kcalloc(type->num_boxes, sizeof(**topology), GFP_KERNEL);
>  		if (!topology[die])
> -			goto clear;
> +			goto free_topology;
>  		for (idx = 0; idx < type->num_boxes; idx++) {
>  			topology[die][idx].untyped = kcalloc(type->num_boxes,
>  							     topology_size[topology_type],
> @@ -3827,6 +3827,7 @@ static int pmu_alloc_topology(struct
> intel_uncore_type *type, int topology_type)
>  			kfree(topology[die][idx].untyped);
>  		kfree(topology[die]);
>  	}
> +free_topology:
>  	kfree(topology);
>  err:
>  	return -ENOMEM;
> 
> Thanks,
> Kan
> 

In this way the already allocated topology[die] elements won't be freed.

--
Fedor

Re: [PATCH] perf/x86/uncore: avoid null-ptr-deref on error in pmu_alloc_topology

[Liang, Kan]

On 2024-02-05 10:18 a.m., Fedor Pchelkin wrote:
> Hello,
> 
> On 24/02/05 10:08AM, Liang, Kan wrote:
>>
>>
>> On 2024-02-04 8:48 a.m., Fedor Pchelkin wrote:
>>> If topology[die] array allocation fails then topology[die][idx] elements
>>> can't be accessed on error path.
>>>
>>> Checking this on the error path probably looks more readable than
>>> decrementing the counter in the allocation loop.
>>>
>>> Found by Linux Verification Center (linuxtesting.org).
>>>
>>> Fixes: 4d13be8ab5d4 ("perf/x86/intel/uncore: Generalize IIO topology support")
>>> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
>>> ---
>>
>> It seems the code just jumps to the wrong kfree on the error path.
>> Does the below patch work?
>>
>> diff --git a/arch/x86/events/intel/uncore_snbep.c
>> b/arch/x86/events/intel/uncore_snbep.c
>> index 8250f0f59c2b..5481fd00d861 100644
>> --- a/arch/x86/events/intel/uncore_snbep.c
>> +++ b/arch/x86/events/intel/uncore_snbep.c
>> @@ -3808,7 +3808,7 @@ static int pmu_alloc_topology(struct
>> intel_uncore_type *type, int topology_type)
>>  	for (die = 0; die < uncore_max_dies(); die++) {
>>  		topology[die] = kcalloc(type->num_boxes, sizeof(**topology), GFP_KERNEL);
>>  		if (!topology[die])
>> -			goto clear;
>> +			goto free_topology;
>>  		for (idx = 0; idx < type->num_boxes; idx++) {
>>  			topology[die][idx].untyped = kcalloc(type->num_boxes,
>>  							     topology_size[topology_type],
>> @@ -3827,6 +3827,7 @@ static int pmu_alloc_topology(struct
>> intel_uncore_type *type, int topology_type)
>>  			kfree(topology[die][idx].untyped);
>>  		kfree(topology[die]);
>>  	}
>> +free_topology:
>>  	kfree(topology);
>>  err:
>>  	return -ENOMEM;
>>
>> Thanks,
>> Kan
>>
> 
> In this way the already allocated topology[die] elements won't be freed.
>

Ah, right. The patch looks good to me.

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan
> --
> Fedor
> 

Re: [PATCH] perf/x86/uncore: avoid null-ptr-deref on error in pmu_alloc_topology

[Fedor Pchelkin]

On Sun, 04. Feb 16:48, Fedor Pchelkin wrote:
> If topology[die] array allocation fails then topology[die][idx] elements
> can't be accessed on error path.
> 
> Checking this on the error path probably looks more readable than
> decrementing the counter in the allocation loop.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Fixes: 4d13be8ab5d4 ("perf/x86/intel/uncore: Generalize IIO topology support")
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

Hi,

ping as the patch seems to have been missed. FWIW, it's still applicable to
the mainline.

--
Thanks,
Fedor