linux-kernel - Re: [PATCH] platform/x86: intel/pmc: Fix iounmap call for valid addresses

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: <1405ee5c-de05-4f67-a747-98412e4b0017@linux.intel.com>
Date: Mon, 24 Mar 2025 13:54:38 -0700
From: Xi Pardee <xi.pardee@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: irenic.rajneesh@...il.com, david.e.box@...ux.intel.com,
 Hans de Goede <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org,
 LKML <linux-kernel@...r.kernel.org>, linux-pm@...r.kernel.org
Subject: Re: [PATCH] platform/x86: intel/pmc: Fix iounmap call for valid
 addresses


On 3/21/2025 7:49 AM, Ilpo Järvinen wrote:
> On Wed, 19 Mar 2025, Xi Pardee wrote:
>
>> pmc_core_clean_structure() is called when generic_core_init() fails.
>> generic_core_init() could fail before ioremap() is called to get
>> a valid regbase for pmc structure. The current code does not check
>> regbase before calling iounmap(). Add a check to fix it.
> Hi,
>
> The approach that calls the same "full cleanup" function as deinit uses
> when init function fails midway is very error prone as once again is
> demonstrated. Is this the only error handling problem? Are you 100% sure?
>
> Think about it, init is x% (<100%) done when it fails, then it calls a
> function that tries to undo 100%. One needs to add lots of special logic
> to handle 0-100% rollback into that cleanup function. The init function,
> on the other hand, knows exactly where it was so it can rollback just what
> is needed and not even try to rollback for more.
>
> It's also very inconsistent to rollback ssram_pcidev in this file as ssram
> code was moved into core_ssram so I think the ssram deinit should be moved
> there too.
>
> I think these init functions should be converted to do proper rollback
> within the init function(s) to avoid very hard to track error handling.
> I tried to check the error handling now in the pmc driver and after I
> would have needed to jump between the files, I gave up.

Hi,

Thanks for the comments! Will move the error handing code for init 
function to init function in next cycle.

I will send out a new version to separate SSRAM device handling code to 
a new PCI driver and remove ssram_pcidev field from pmcdev in next cycle.


Thanks!

Xi

>> Fixes: 1b8c7b843c00 ("platform/x86:intel/pmc: Discover PMC devices")
>> Signed-off-by: Xi Pardee <xi.pardee@...ux.intel.com>
>> ---
>>   drivers/platform/x86/intel/pmc/core.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
>> index 7a1d11f2914f..de5fc06232e5 100644
>> --- a/drivers/platform/x86/intel/pmc/core.c
>> +++ b/drivers/platform/x86/intel/pmc/core.c
>> @@ -1471,7 +1471,7 @@ static void pmc_core_clean_structure(struct platform_device *pdev)
>>   	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
>>   		struct pmc *pmc = pmcdev->pmcs[i];
>>   
>> -		if (pmc)
>> +		if (pmc && pmc->regbase)
>>   			iounmap(pmc->regbase);