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

[Mario Limonciello <mario.limonciello@amd.com>]

On 5/6/2025 11:11 AM, Mario Limonciello wrote:
> 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()].

Ah I think I found the actual callpath.

It's amd_pmf_remove() that has kfree(dev->buf) - that's probably what's 
actually tripping it.