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

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

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

On Fri, 16 May 2025, Mario Limonciello wrote:
> On 5/16/25 01:04, Ilpo Järvinen wrote:
> > On Thu, 15 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()")
> > > Link:
> > > https://lore.kernel.org/r/20250512211154.2510397-2-superm1@kernel.org
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > v4:
> > >   * Handle failures from memory allocation on sideload (Ilpo)
> > >   * Allocate memory before copying from user (Ilpo)
> > > ---
> > >   drivers/platform/x86/amd/pmf/core.c   |  3 +-
> > >   drivers/platform/x86/amd/pmf/tee-if.c | 58 +++++++++------------------
> > >   2 files changed, 20 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/amd/pmf/core.c
> > > b/drivers/platform/x86/amd/pmf/core.c
> > > index 96821101ec773..395c011e837f1 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 d3bd12ad036ae..6d85601812225 100644
> > > --- a/drivers/platform/x86/amd/pmf/tee-if.c
> > > +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> > > @@ -350,38 +350,30 @@ static ssize_t amd_pmf_get_pb_data(struct file
> > > *filp, const char __user *buf,
> > >   				   size_t length, loff_t *pos)
> > >   {
> > >   	struct amd_pmf_dev *dev = filp->private_data;
> > > -	unsigned char *new_policy_buf;
> > >   	int ret;
> > >     	/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
> > >   	if (length > POLICY_BUF_MAX_SZ || length == 0)
> > >   		return -EINVAL;
> > >   -	/* re-alloc to the new buffer length of the policy binary */
> > > -	new_policy_buf = memdup_user(buf, length);
> > > -	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_kzalloc(dev->dev, length, GFP_KERNEL);
> > > +	if (IS_ERR(dev->policy_buf))
> > > +		return -ENOMEM;
> > >   	dev->policy_sz = length;
> > >   -	if (!amd_pmf_pb_valid(dev)) {
> > > -		ret = -EINVAL;
> > > -		goto cleanup;
> > > -	}
> > > +	if (copy_from_user(dev->policy_buf, buf, length))
> > > +		return -EFAULT;
> > 
> > Previously, if anything failed here, the old buffer was left in place.
> > I always assumed it was intentional. But after your change, first thing
> > that happens is freeing the old policy_buf.
> 
> Yeah; in order to do devm without a double malloc it needs to be cleared
> immediately.

I'm feeling like I must be missing something here, but I just fail to see 
why the order _has to be_ changed when changing kfree() -> devm_kfree().

> But this is a debugfs sideloading interface.  If you send a bad
> binary you can just try again with a good one.
> 
> > 
> > We're long past the point where I've started to lose confidence in this
> > patch :-(. Could we like just make the minimal changes here to convert
> > into devm_*() and nothing more? If you want to make any other changes, be
> > it reordering logic, removal of the local variable, or whatever, please
> > put those into own patch(es) and properly justify them.
> > 
> 
> If we're aiming for a total minimal patch that just fixes the most immediate
> issue that's v1 of this series [1].

As spelled out very clearly in the above comment, I'm aiming to a patch 
which converts this to devm_*() without other changes. If you want to do 
other changes, they should be in their own patch.

> Through the course of the discussion obviously there were more things raised
> by Dan, and I feel that v4 is more robust.
> 
> Maybe the right answer is to just pick up v1 for 6.15-rc, and this series for
> 6.16?  Or if you want this to have more time in -next I can just resubmit it
> after 6.16-rc1 and we aim for 6.17 with it.
> 
> [1]
> https://lore.kernel.org/platform-driver-x86/20250506131130.1446262-1-superm1@kernel.org/#t
> 

-- 
 i.