| Message ID | 20240124012939.6550-1-liucong2@kylinos.cn (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Hans de Goede |
| Headers | show |
| Series | [v2] platform/x86/amd/pmf: Fix memory leak in amd_pmf_get_pb_data() | expand |
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;
> 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
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 <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(-)