Re: [PATCH] platform/x86: intel_pmc_core: Fix uninitialized pmc/map in pmc_core_send_ltr_ignore

[Purva Yeshi <purvayeshi550@gmail.com>]

On 17/04/25 18:43, Ilpo Järvinen wrote:
> On Thu, 17 Apr 2025, Purva Yeshi wrote:
> 
>> Fix Smatch-detected issue:
>>
>> drivers/platform/x86/intel/pmc/core.c:501 pmc_core_send_ltr_ignore()
>> error: uninitialized symbol 'pmc'.
>>
>> drivers/platform/x86/intel/pmc/core.c:501 pmc_core_send_ltr_ignore()
>> error: uninitialized symbol 'map'.
>>
>> drivers/platform/x86/intel/pmc/core.c:501 pmc_core_send_ltr_ignore()
>> error: we previously assumed 'pmc' could be null (see line 479)
>>
>>
>> Prevents uninitialized symbol warnings detected by smatch.
>>
>> Ensures map is not accessed if pmc is NULL, preventing dereferencing
>> of uninitialized pointers
>>
>> Add defensive check for pmc and map to catch any unexpected edge cases
>> and ensure all required pointers are valid.
>>
>> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
>> ---
>>   drivers/platform/x86/intel/pmc/core.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
>> index 7a1d11f2914f..e674b940e29e 100644
>> --- a/drivers/platform/x86/intel/pmc/core.c
>> +++ b/drivers/platform/x86/intel/pmc/core.c
>> @@ -462,8 +462,8 @@ DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
>>   
>>   int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore)
>>   {
>> -	struct pmc *pmc;
>> -	const struct pmc_reg_map *map;
>> +	struct pmc *pmc = NULL;
>> +	const struct pmc_reg_map *map = NULL;
>>   	u32 reg;
>>   	unsigned int pmc_index;
>>   	int ltr_index;
>> @@ -480,6 +480,9 @@ int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore)

>>   			continue;
>>   
>>   		map = pmc->map;
>> +		if (!map)
>> +			continue;
> 
> How can this happen?? If pmc is created, it should have a valid ->map
> AFAICT. Did you even read that code at all???

Hi,

Thanks for the feedback.

Yes, I did read through the code and I understand your point.

The motivation behind the patch was a Smatch warning about possible 
uninitialized use of map and pmc, even though they are logically 
guarded. I now see that these checks may not be necessary given the 
existing control flow.

> 
>> +
>>   		if (ltr_index <= map->ltr_ignore_max)
>>   			break;
>>   
>> @@ -491,7 +494,7 @@ int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore)
>>   		ltr_index = ltr_index - (map->ltr_ignore_max + 2) - 1;
>>   	}
>>   
>> -	if (pmc_index >= ARRAY_SIZE(pmcdev->pmcs) || ltr_index < 0)
>> +	if (pmc_index >= ARRAY_SIZE(pmcdev->pmcs) || ltr_index < 0 || !pmc || !map)
> 
> What are the situations pmc_index >= ARRAY_SIZE(pmcdev->pmcs) check
> didn't catch where these new checks do something useful??
> 
> Lots of noise but little real substance in this patch?

You're right, if pmc is non-NULL, then map should also be valid, and the 
bounds check on pmc_index already prevents out-of-bounds access. Adding 
further checks might just add noise.

I'll drop the patch unless there's a cleaner way to restructure the 
logic to make Smatch silence without redundant checks.

Thanks again for the clarification!

Best regards,
Purva

>