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

[Dan Carpenter <dan.carpenter@linaro.org>]

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?
My reading of the code says that this bug can only occur if
amd_pmf_register_input_device() fails, right?

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