Re: [PATCH] drivers/platform/x86/amd: pmf: Fix a double free on module unload

[Mario Limonciello <superm1@kernel.org>]

On 5/6/2025 10:53 AM, Dan Carpenter wrote:
> On Tue, May 06, 2025 at 08:11:29AM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> If setting up smart PC fails for any reason then this can lead to
>> a double free when unloading amd-pmf.  This is because dev->buf was
>> freed but never set to NULL and is again freed in
>> amd_pmf_deinit_smart_pc().
>>
>> Explicitly set pointers to NULL after freeing them to avoid the
>> double free.
>>
>> Fixes: 5b1122fc4995f ("platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc()")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/platform/x86/amd/pmf/tee-if.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>> index a1e43873a07b0..48902f1c767c6 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -579,10 +579,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>   	amd_pmf_tee_deinit(dev);
>          ^^^^^^^^^^^^^^^^^^^^^^^
> 
>>   err_free_prev_data:
>>   	kfree(dev->prev_data);
>> +	dev->prev_data = NULL;
>>   err_free_policy:
>>   	kfree(dev->policy_buf);
>> +	dev->policy_buf = NULL;
>>   err_free_dram_buf:
>>   	kfree(dev->buf);
>> +	dev->buf = NULL;
>>   err_cancel_work:
>>   	cancel_delayed_work_sync(&dev->pb_work);
>>   
> 
> This is a real bug.  Did you find it from testing or reading the code?

I found it from testing.  I was testing some other unrelated changes and 
found that unloading/reloading the module eventually lead to problems. 
I tracked it down to your change.

> My reading of the code says that this bug can only occur if
> amd_pmf_register_input_device() fails, right?

No; it was happening from a failure where the system didn't have a 
policy or had a "bad" policy.

> 
> We can only call amd_pmf_deinit_smart_pc() if amd_pmf_start_policy_engine()
> succeeds because that's where we set:
> 
> 	dev->smart_pc_enabled = true;
> 
> This patch doesn't totally fix the problem because we would still call
> amd_pmf_tee_deinit().  That's why I suspect you found this by auditing
> the code because I think that remaining bug would trigger a stack trace.
> I also worry that there is a small race window where we could trigger
> amd_pmf_tee_deinit() before amd_pmf_init_smart_pc() has finished
> running.
> 
> Another bug is that we should cancel the work before freeing all the
> pointers.  This looks like the more serious bug.
> 
> What about if we only set dev->smart_pc_enabled = true if the whole
> amd_pmf_init_smart_pc() has succeeded?
> 
> regards,
> dan carpenter
> 

Right; it's only set when amd_pmf_start_policy_engine() succeeds which 
was not the case for me.  This makes me wonder how exactly this was 
happening [amd_pmf_deinit_smart_pc() would only be called from 
amd_pmf_deinit_features()].