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:49:12PM -0500, Mario Limonciello wrote:
> 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.

Great!  There should be no need to call kfree(dev->buf) there because
amd_pmf_deinit_features() will do it.  Could you add something like
the following to your diff?

The changes to amd_pmf_ta_open_session() are unrelated actually?  Do
that in another patch?

Setting dev->tee_ctx to NULL will prevent amd_pmf_tee_deinit() from
being run twice.

We only need to cancel the work once it has been scheduled.  We
scheduled it for 3 seconds into the future so it will be canceled and
that bug won't trigger in real life.  But freeing the pointers before
canceling is ugly.

regards,
dan carpenter

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 96821101ec77..0a494d6e8fcf 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -493,7 +493,6 @@ static void amd_pmf_remove(struct platform_device *pdev)
 	mutex_destroy(&dev->lock);
 	mutex_destroy(&dev->update_mutex);
 	mutex_destroy(&dev->cb_mutex);
-	kfree(dev->buf);
 }
 
 static const struct attribute_group *amd_pmf_driver_groups[] = {
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 14b99d8b63d2..b332b5470398 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -407,12 +407,12 @@ static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id, const uuid_
 	rc = tee_client_open_session(ctx, &sess_arg, NULL);
 	if (rc < 0 || sess_arg.ret != 0) {
 		pr_err("Failed to open TEE session err:%#x, rc:%d\n", sess_arg.ret, rc);
-		return rc;
+		return rc ?: -EINVAL;
 	}
 
 	*id = sess_arg.session;
 
-	return rc;
+	return 0;
 }
 
 static int amd_pmf_register_input_device(struct amd_pmf_dev *dev)
@@ -447,7 +447,9 @@ static int amd_pmf_tee_init(struct amd_pmf_dev *dev, const uuid_t *uuid)
 	dev->tee_ctx = tee_client_open_context(NULL, amd_pmf_amdtee_ta_match, NULL, NULL);
 	if (IS_ERR(dev->tee_ctx)) {
 		dev_err(dev->dev, "Failed to open TEE context\n");
-		return PTR_ERR(dev->tee_ctx);
+		ret = PTR_ERR(dev->tee_ctx);
+		dev->tee_ctx = NULL;
+		return ret;
 	}
 
 	ret = amd_pmf_ta_open_session(dev->tee_ctx, &dev->session_id, uuid);
@@ -487,9 +489,12 @@ static int amd_pmf_tee_init(struct amd_pmf_dev *dev, const uuid_t *uuid)
 
 static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
 {
+	if (!dev->tee_ctx)
+		return;
 	tee_shm_free(dev->fw_shm_pool);
 	tee_client_close_session(dev->tee_ctx, dev->session_id);
 	tee_client_close_context(dev->tee_ctx);
+	dev->tee_ctx = NULL;
 }
 
 int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
@@ -512,7 +517,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 
 	ret = amd_pmf_set_dram_addr(dev, true);
 	if (ret)
-		goto err_cancel_work;
+		return ret;
 
 	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
 	if (IS_ERR(dev->policy_base)) {
@@ -576,6 +581,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	return 0;
 
 err_pmf_remove_pb:
+	cancel_delayed_work_sync(&dev->pb_work);
 	if (pb_side_load && dev->esbin)
 		amd_pmf_remove_pb(dev);
 	amd_pmf_tee_deinit(dev);
@@ -585,8 +591,6 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	kfree(dev->policy_buf);
 err_free_dram_buf:
 	kfree(dev->buf);
-err_cancel_work:
-	cancel_delayed_work_sync(&dev->pb_work);
 
 	return ret;
 }