linux-kernel - Re: [PATCH 5.10 105/299] crypto: stm32/cryp - Fix PM reference leak on stm32-cryp.c

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 11 May 2021 11:55:48 +0800
From:   Liu Shixin <liushixin2@...wei.com>
To:     Pavel Machek <pavel@...x.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     <linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 5.10 105/299] crypto: stm32/cryp - Fix PM reference leak
 on stm32-cryp.c

On 2021/5/10 20:07, Pavel Machek wrote:
> On Mon 2021-05-10 12:18:22, Greg Kroah-Hartman wrote:
>> From: Shixin Liu <liushixin2@...wei.com>
>>
>> [ Upstream commit 747bf30fd944f02f341b5f3bc7d97a13f2ae2fbe ]
>>
>> pm_runtime_get_sync will increment pm usage counter even it failed.
>> Forgetting to putting operation will result in reference leak here.
>> Fix it by replacing it with pm_runtime_resume_and_get to keep usage
>> counter balanced.
> Yes, but that only works when code checks the return value.
Thank you for the correction. Yes, You are right that if we carry out runtime resume
failed on the path where the return value is not checked, the pm usage counter will
be put in later path.

But I have another question. Why don't we check the return values in these path?
Does that mean these resumes will never fail?
>> +++ b/drivers/crypto/stm32/stm32-cryp.c
>> @@ -542,7 +542,7 @@ static int stm32_cryp_hw_init(struct stm32_cryp *cryp)
>>  	int ret;
>>  	u32 cfg, hw_mode;
>>  
>> -	pm_runtime_get_sync(cryp->dev);
>> +	pm_runtime_resume_and_get(cryp->dev);
>>  
>>  	/* Disable interrupt */
>>  	stm32_cryp_write(cryp, CRYP_IMSCR, 0);
> Again, this is wrong.
>
>> @@ -2043,7 +2043,7 @@ static int stm32_cryp_remove(struct platform_device *pdev)
>>  	if (!cryp)
>>  		return -ENODEV;
>>  
>> -	ret = pm_runtime_get_sync(cryp->dev);
>> +	ret = pm_runtime_resume_and_get(cryp->dev);
>>  	if (ret < 0)
>>  		return ret;
>>  
> But this may be right.
>
> Best regards,
> 								Pavel