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

[Mario Limonciello <superm1@kernel.org>]

On 5/20/2025 10:19 AM, Ilpo Järvinen wrote:
> 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_*().

Thanks for the suggestion!  I hadn't thought about using a second 
pointer like that.

Technically the devm_kfree(new_policy_buf) under the copy_from_user() 
failure shouldn't be needed though, right?  Because it will still be 
freed when the device is freed.

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