linux-kernel - Re: [PATCH] platform/x86: intel_pmc_core: Fix uninitialized pmc/map in pmc_core_send_ltr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <309074de-9f12-45c9-8eec-8a116ee6abf8@gmail.com>
Date: Fri, 18 Apr 2025 14:36:53 +0530
From: Purva Yeshi <purvayeshi550@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: irenic.rajneesh@...il.com, david.e.box@...el.com,
 Hans de Goede <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org,
 LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: intel_pmc_core: Fix uninitialized pmc/map
 in pmc_core_send_ltr_ignore

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@...il.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

>