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: 6710 bytes --]

On Fri, 16 May 2025, Mario Limonciello wrote:

> On 5/16/2025 1:36 AM, Ilpo Järvinen wrote:
> > 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().
> 
> Because the policy is coming in from userspace and you need to have somewhere
> that is the right size to copy it to.
> 
> As you mentioned wanting to remove the double malloc in the earlier version
> this is the way to do it.
> 
> IE when using copy_from_user() instead of memdup_user() the memory must
> "already" be allocated.  That's what is done now with devm_kzalloc().

Hi Mario,

Why can't you do:

	/* re-alloc to the new buffer length of the policy binary */
	new_policy_buf = devm_kzalloc(dev->dev, length, GFP_KERNEL);
	if (!new_policy_buf)
		return -ENOMEM;

	if (copy_from_user(new_policy_buf, buf, length)) {
		devm_kfree(new_policy_buf);
		return -EFAULT;
	}

	devm_kfree(dev->policy_buf);
	ev->policy_buf = new_policy_buf;

?

That follows pretty much what the old code did with new_policy_buf 
variable, but allocation related calls are now devm_*().

> Are you suggesting some sort of way to keep it in the same order and subvert
> device managed allocations and "steal" the pointer?  Glib has a concept like
> this, but I wasn't aware of a way to do in the kernel.
> 
> > 
> > > 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.
> 
> I guess if there's a way to do this without changing the order I will do it,
> but I don't see one RN.


-- 
 i.