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

[Mario Limonciello <superm1@kernel.org>]

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().

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.

> 
>> 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
>>
>