[PATCH] platform/x86/amd/pmf: Fix memory leak in amd_pmf_get_pb_data()

[PATCH] platform/x86/amd/pmf: Fix memory leak in amd_pmf_get_pb_data()

[Cong Liu]

amd_pmf_get_pb_data() will allocate memory for the policy buffer,
but does not free it if copy_from_user() fails. This leads to a memory
leak.

Signed-off-by: Cong Liu <liucong2@kylinos.cn>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 502ce93d5cdd..f8c0177afb0d 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -298,8 +298,10 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
 	if (!new_policy_buf)
 		return -ENOMEM;
 
-	if (copy_from_user(new_policy_buf, buf, length))
+	if (copy_from_user(new_policy_buf, buf, length)) {
+		kfree(new_policy_buf);
 		return -EFAULT;
+	}
 
 	kfree(dev->policy_buf);
 	dev->policy_buf = new_policy_buf;
-- 
2.34.1

Re: [PATCH] platform/x86/amd/pmf: Fix memory leak in amd_pmf_get_pb_data()

[Shyam Sundar S K]

Hi,

On 1/23/2024 06:41, Cong Liu wrote:
> amd_pmf_get_pb_data() will allocate memory for the policy buffer,
> but does not free it if copy_from_user() fails. This leads to a memory
> leak.

Thank you for the fix and looks good to me (just a valid Fixes tag is
missing.)

Fixes: 10817f28e533 ("platform/x86/amd/pmf: Add capability to sideload
of policy binary")
Reviewed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Thanks,
Shyam

> 
> Signed-off-by: Cong Liu <liucong2@kylinos.cn>
> ---
>  drivers/platform/x86/amd/pmf/tee-if.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 502ce93d5cdd..f8c0177afb0d 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -298,8 +298,10 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>  	if (!new_policy_buf)
>  		return -ENOMEM;
>  
> -	if (copy_from_user(new_policy_buf, buf, length))
> +	if (copy_from_user(new_policy_buf, buf, length)) {
> +		kfree(new_policy_buf);
>  		return -EFAULT;
> +	}
>  
>  	kfree(dev->policy_buf);
>  	dev->policy_buf = new_policy_buf;

[PATCH v2] platform/x86/amd/pmf: Fix memory leak in amd_pmf_get_pb_data()

[Cong Liu]

amd_pmf_get_pb_data() will allocate memory for the policy buffer,
but does not free it if copy_from_user() fails. This leads to a memory
leak.

Fixes: 10817f28e533 ("platform/x86/amd/pmf: Add capability to sideload of policy binary")
Reviewed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Signed-off-by: Cong Liu <liucong2@kylinos.cn>
---
 drivers/platform/x86/amd/pmf/tee-if.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 502ce93d5cdd..f8c0177afb0d 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -298,8 +298,10 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
 	if (!new_policy_buf)
 		return -ENOMEM;
 
-	if (copy_from_user(new_policy_buf, buf, length))
+	if (copy_from_user(new_policy_buf, buf, length)) {
+		kfree(new_policy_buf);
 		return -EFAULT;
+	}
 
 	kfree(dev->policy_buf);
 	dev->policy_buf = new_policy_buf;
-- 
2.34.1

Re: [PATCH v2] platform/x86/amd/pmf: Fix memory leak in amd_pmf_get_pb_data()

[Hans de Goede]

Hi,

On 1/24/24 02:29, Cong Liu wrote:
> amd_pmf_get_pb_data() will allocate memory for the policy buffer,
> but does not free it if copy_from_user() fails. This leads to a memory
> leak.
> 
> Fixes: 10817f28e533 ("platform/x86/amd/pmf: Add capability to sideload of policy binary")
> Reviewed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Cong Liu <liucong2@kylinos.cn>

Thank you for your patch/series, I've applied this patch
(series) to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in the pdx86 review-hans branch once I've
pushed my local branch there, which might take a while.

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans



> ---
>  drivers/platform/x86/amd/pmf/tee-if.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 502ce93d5cdd..f8c0177afb0d 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -298,8 +298,10 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>  	if (!new_policy_buf)
>  		return -ENOMEM;
>  
> -	if (copy_from_user(new_policy_buf, buf, length))
> +	if (copy_from_user(new_policy_buf, buf, length)) {
> +		kfree(new_policy_buf);
>  		return -EFAULT;
> +	}
>  
>  	kfree(dev->policy_buf);
>  	dev->policy_buf = new_policy_buf;

Re: [v2] platform/x86/amd/pmf: Fix memory leak in amd_pmf_get_pb_data()

[Markus Elfring]

> Thank you for your patch/series, I've applied this patch
> (series) to my review-hans branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>
> Note it will show up in the pdx86 review-hans branch once I've
> pushed my local branch there, which might take a while.

Will development interests grow for the application of known scripts
also according to the semantic patch language?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/dev-tools/coccinelle.rst?h=v6.8-rc1#n71

Markus_Elfring@Sonne:…/Projekte/Linux/next-analyses> make COCCI=scripts/coccinelle/api/memdup_user.cocci M=drivers/platform/x86/amd/pmf/ coccicheck
…
drivers/platform/x86/amd/pmf/tee-if.c:297:18-25: WARNING opportunity for memdup_user


Regards,
Markus

Re: [v2] platform/x86/amd/pmf: Fix memory leak in amd_pmf_get_pb_data()

[Hans de Goede]

Hi,

On 1/28/24 11:45, Markus Elfring wrote:
>> Thank you for your patch/series, I've applied this patch
>> (series) to my review-hans branch:
>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>>
>> Note it will show up in the pdx86 review-hans branch once I've
>> pushed my local branch there, which might take a while.
> 
> Will development interests grow for the application of known scripts
> also according to the semantic patch language?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/dev-tools/coccinelle.rst?h=v6.8-rc1#n71

Markus,

I'm not sure what your question here is?

Is it: "Will coccinelle scripts be run as part of the regular patch
test/merge workflow?" then the answer is that there are no plans
that I'm aware of to do that at this moment.

If such a thing were to be done, IMHO it would be best to have one
of the existing CI systems like e.h. Intel's LKP test bot run this
on linux-next, or on all the trees LKP already monitors.

And it does sound like something interesting to do, but someone
would need to actually setup and maintain such a CI system.

If the question is: "Are patches generated by coccinelle welcome?"
then the answer is "Yes patches generated by coccinelle are very
much welcome".

Regards,

Hans

Re: [v2] platform/x86/amd/pmf: Fix memory leak in amd_pmf_get_pb_data()

[Markus Elfring]

> If the question is: "Are patches generated by coccinelle welcome?"
> then the answer is "Yes patches generated by coccinelle are very
> much welcome".

How do you think about to fix a questionable memory leak
by using the function “memdup_user” instead?
https://elixir.bootlin.com/linux/v6.8-rc1/source/mm/util.c#L185

Would you like to try a corresponding command out once more on source files
of a software like “Linux next-20240125”?
https://elixir.bootlin.com/linux/v6.8-rc1/source/scripts/coccinelle/api/memdup_user.cocci#L2

make COCCI=scripts/coccinelle/api/memdup_user.cocci M=drivers/platform/x86/amd/pmf/ MODE=patch coccicheck


Regards,
Markus