Re: [PATCH v3 1/3] platform/x86/amd: pmf: Use device managed allocations

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7164 bytes --]

On Wed, 14 May 2025, Mario Limonciello wrote:
> On 5/14/2025 5:21 AM, Ilpo Järvinen wrote:
> > On Mon, 12 May 2025, 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_remove().
> > > 
> > > To avoid subtle allocation bugs in failures leading to a double free
> > > change all allocations into device managed allocations.
> > > 
> > > 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/core.c   |  3 +-
> > >   drivers/platform/x86/amd/pmf/tee-if.c | 48 +++++++++------------------
> > >   2 files changed, 16 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/amd/pmf/core.c
> > > b/drivers/platform/x86/amd/pmf/core.c
> > > index 96821101ec77..395c011e837f 100644
> > > --- a/drivers/platform/x86/amd/pmf/core.c
> > > +++ b/drivers/platform/x86/amd/pmf/core.c
> > > @@ -280,7 +280,7 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev,
> > > bool alloc_buffer)
> > >   			dev_err(dev->dev, "Invalid CPU id: 0x%x",
> > > dev->cpu_id);
> > >   		}
> > >   -		dev->buf = kzalloc(dev->mtable_size, GFP_KERNEL);
> > > +		dev->buf = devm_kzalloc(dev->dev, dev->mtable_size,
> > > GFP_KERNEL);
> > >   		if (!dev->buf)
> > >   			return -ENOMEM;
> > >   	}
> > > @@ -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 d3bd12ad036a..513dbbe3f214 100644
> > > --- a/drivers/platform/x86/amd/pmf/tee-if.c
> > > +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> > > @@ -362,26 +362,20 @@ static ssize_t amd_pmf_get_pb_data(struct file
> > > *filp, const char __user *buf,
> > >   	if (IS_ERR(new_policy_buf))
> > >   		return PTR_ERR(new_policy_buf);
> > >   -	kfree(dev->policy_buf);
> > > -	dev->policy_buf = new_policy_buf;
> > > +	devm_kfree(dev->dev, dev->policy_buf);
> > > +	dev->policy_buf = devm_kmemdup(dev->dev, new_policy_buf,
> > > dev->policy_sz, GFP_KERNEL);
> > 
> > So now there are two memdups?? 
> 
> Is there a safe way to do it without the double memdups that I'm missing?

Why doing it in two stages, first devm_kmalloc() then copy from user, 
wouldn't be safe?

> I didn't see a helper for device managed memory to copy from user like
> memdup_user() does.

There seemingly isn't. I'm not sure how common use case this is as devm is 
typically an init thing, not something called from fops, so it might not 
be worth adding devm_kmemdup_user() for this. But I haven't looked if 
there are other cases in the kernel sources that could use it (it's not
a one-liner with grep and both patterns are extremely frequent, maybe 
coccinelle could find the relevant ones easily if there are others in 
case you want to look).

> > And there's no error handling either?!
> 
> Whoops; will fix.
> 
> > 
> > >   	dev->policy_sz = length;
> > > +	kfree(new_policy_buf);
> > >   -	if (!amd_pmf_pb_valid(dev)) {
> > > -		ret = -EINVAL;
> > > -		goto cleanup;
> > > -	}
> > > +	if (!amd_pmf_pb_valid(dev))
> > 
> > Due to lack of error handling, this can deref NULL.
> > 
> > > +		return -EINVAL;
> > >     	amd_pmf_hex_dump_pb(dev);
> > >   	ret = amd_pmf_start_policy_engine(dev);
> > >   	if (ret < 0)
> > > -		goto cleanup;
> > > +		return ret;
> > >     	return length;
> > > -
> > > -cleanup:
> > > -	kfree(dev->policy_buf);
> > > -	dev->policy_buf = NULL;
> > > -	return ret;
> > >   }
> > >     static const struct file_operations pb_fops = {
> > > @@ -532,13 +526,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >   	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
> > >   	if (IS_ERR(dev->policy_base)) {
> > >   		ret = PTR_ERR(dev->policy_base);
> > > -		goto err_free_dram_buf;
> > > +		goto err_cancel_work;
> > >   	}
> > >   -	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> > > +	dev->policy_buf = devm_kzalloc(dev->dev, dev->policy_sz, GFP_KERNEL);
> > >   	if (!dev->policy_buf) {
> > >   		ret = -ENOMEM;
> > > -		goto err_free_dram_buf;
> > > +		goto err_cancel_work;
> > >   	}
> > >     	memcpy_fromio(dev->policy_buf, dev->policy_base,
> > > dev->policy_sz);
> > > @@ -546,21 +540,21 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >   	if (!amd_pmf_pb_valid(dev)) {
> > >   		dev_info(dev->dev, "No Smart PC policy present\n");
> > >   		ret = -EINVAL;
> > > -		goto err_free_policy;
> > > +		goto err_cancel_work;
> > >   	}
> > >     	amd_pmf_hex_dump_pb(dev);
> > >   -	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
> > > +	dev->prev_data = devm_kzalloc(dev->dev, sizeof(*dev->prev_data),
> > > GFP_KERNEL);
> > >   	if (!dev->prev_data) {
> > >   		ret = -ENOMEM;
> > > -		goto err_free_policy;
> > > +		goto err_cancel_work;
> > >   	}
> > >     	for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) {
> > >   		ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]);
> > >   		if (ret)
> > > -			goto err_free_prev_data;
> > > +			goto err_cancel_work;
> > >     		ret = amd_pmf_start_policy_engine(dev);
> > >   		switch (ret) {
> > > @@ -575,7 +569,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >   		default:
> > >   			ret = -EINVAL;
> > >   			amd_pmf_tee_deinit(dev);
> > > -			goto err_free_prev_data;
> > > +			goto err_cancel_work;
> > >   		}
> > >     		if (status)
> > > @@ -584,7 +578,7 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >     	if (!status && !pb_side_load) {
> > >   		ret = -EINVAL;
> > > -		goto err_free_prev_data;
> > > +		goto err_cancel_work;
> > >   	}
> > >     	if (pb_side_load)
> > > @@ -600,12 +594,6 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> > >   	if (pb_side_load && dev->esbin)
> > >   		amd_pmf_remove_pb(dev);
> > >   	amd_pmf_tee_deinit(dev);
> > > -err_free_prev_data:
> > > -	kfree(dev->prev_data);
> > > -err_free_policy:
> > > -	kfree(dev->policy_buf);
> > > -err_free_dram_buf:
> > > -	kfree(dev->buf);
> > >   err_cancel_work:
> > >   	cancel_delayed_work_sync(&dev->pb_work);
> > >   @@ -621,11 +609,5 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev
> > > *dev)
> > >   		amd_pmf_remove_pb(dev);
> > >     	cancel_delayed_work_sync(&dev->pb_work);
> > > -	kfree(dev->prev_data);
> > > -	dev->prev_data = NULL;
> > > -	kfree(dev->policy_buf);
> > > -	dev->policy_buf = NULL;
> > > -	kfree(dev->buf);
> > > -	dev->buf = NULL;
> > >   	amd_pmf_tee_deinit(dev);
> > >   }
> > > 
> > 
> 

-- 
 i.