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 <[email protected]>
---
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
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 <[email protected]>
Thanks,
Shyam
>
> Signed-off-by: Cong Liu <[email protected]>
> ---
> 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;
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 <[email protected]>
Signed-off-by: Cong Liu <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Cong Liu <[email protected]>
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;
> 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
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
> 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