| Message ID | 20211202033447.253596-1-bernard@vivo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | security/selinux: fix potential memleak | expand |
On Wed, Dec 1, 2021 at 10:35 PM Bernard Zhao <bernard@vivo.com> wrote: > > This patch try to fix potential memleak in function > selinux_fs_context_dup`s error branch. > > Signed-off-by: Bernard Zhao <bernard@vivo.com> > --- > security/selinux/hooks.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 62d30c0a30c2..36d7fc373839 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2856,24 +2856,38 @@ static int selinux_fs_context_dup(struct fs_context *fc, > if (src->fscontext) { > opts->fscontext = kstrdup(src->fscontext, GFP_KERNEL); > if (!opts->fscontext) > - return -ENOMEM; > + goto err_fscontext; > } > if (src->context) { > opts->context = kstrdup(src->context, GFP_KERNEL); > if (!opts->context) > - return -ENOMEM; > + goto err_context; > } > if (src->rootcontext) { > opts->rootcontext = kstrdup(src->rootcontext, GFP_KERNEL); > if (!opts->rootcontext) > - return -ENOMEM; > + goto err_rootcontext; > } > if (src->defcontext) { > opts->defcontext = kstrdup(src->defcontext, GFP_KERNEL); > if (!opts->defcontext) > - return -ENOMEM; > + goto err_defcontext; > } > return 0; > + > +err_defcontext: > + if (src->rootcontext) > + kfree(opts->rootcontext); > +err_rootcontext: > + if (src->context) > + kfree(opts->context); > +err_context: > + if (src->fscontext) > + kfree(opts->fscontext); > +err_fscontext: > + kfree(fc->security); > + > + return -ENOMEM; > } Thanks for catching this a providing a patch, however I think the memory cleanup can be made simpler, see the pseudo code below: static int selinux_fs_context_dup(...) { fc->security = kzalloc(...); if (!fc->security) return -ENOMEM; opts = fc->security; if (src->fscontext) { opts->fscontext = kstrdup(...); if (!opts->fscontext) goto err; } /* ... */ return 0; err: kfree(opts->fscontext); kfree(opts->context); kfree(opts->rootcontext); kfree(opts->defcontext); kfree(opts); fc->security = NULL; return -ENOMEM; } -- paul moore www.paul-moore.com
On Mon, Dec 6, 2021 at 4:12 AM Paul Moore <paul@paul-moore.com> wrote: > On Wed, Dec 1, 2021 at 10:35 PM Bernard Zhao <bernard@vivo.com> wrote: > > > > This patch try to fix potential memleak in function > > selinux_fs_context_dup`s error branch. > > > > Signed-off-by: Bernard Zhao <bernard@vivo.com> > > --- > > security/selinux/hooks.c | 22 ++++++++++++++++++---- > > 1 file changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 62d30c0a30c2..36d7fc373839 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2856,24 +2856,38 @@ static int selinux_fs_context_dup(struct fs_context *fc, > > if (src->fscontext) { > > opts->fscontext = kstrdup(src->fscontext, GFP_KERNEL); > > if (!opts->fscontext) > > - return -ENOMEM; > > + goto err_fscontext; > > } > > if (src->context) { > > opts->context = kstrdup(src->context, GFP_KERNEL); > > if (!opts->context) > > - return -ENOMEM; > > + goto err_context; > > } > > if (src->rootcontext) { > > opts->rootcontext = kstrdup(src->rootcontext, GFP_KERNEL); > > if (!opts->rootcontext) > > - return -ENOMEM; > > + goto err_rootcontext; > > } > > if (src->defcontext) { > > opts->defcontext = kstrdup(src->defcontext, GFP_KERNEL); > > if (!opts->defcontext) > > - return -ENOMEM; > > + goto err_defcontext; > > } > > return 0; > > + > > +err_defcontext: > > + if (src->rootcontext) > > + kfree(opts->rootcontext); > > +err_rootcontext: > > + if (src->context) > > + kfree(opts->context); > > +err_context: > > + if (src->fscontext) > > + kfree(opts->fscontext); > > +err_fscontext: > > + kfree(fc->security); Also here you need to be careful to not double-free fc->security. (Paul's pseudocode below already correctly resets it to NULL after freeing.) > > + > > + return -ENOMEM; > > } > > Thanks for catching this a providing a patch, however I think the > memory cleanup can be made simpler, see the pseudo code below: > > static int selinux_fs_context_dup(...) > { > > fc->security = kzalloc(...); > if (!fc->security) > return -ENOMEM; > > opts = fc->security; > > if (src->fscontext) { > opts->fscontext = kstrdup(...); > if (!opts->fscontext) > goto err; > } > > /* ... */ > > return 0; > > err: > kfree(opts->fscontext); > kfree(opts->context); > kfree(opts->rootcontext); > kfree(opts->defcontext); > kfree(opts); > fc->security = NULL; > return -ENOMEM; > } > > -- > paul moore > www.paul-moore.com > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
-----邮件原件----- 发件人: bernard@vivo.com <bernard@vivo.com> 代表 Ondrej Mosnacek 发送时间: 2021年12月6日 17:16 收件人: Paul Moore <paul@paul-moore.com> 抄送: 赵军奎 <bernard@vivo.com>; Stephen Smalley <stephen.smalley.work@gmail.com>; Eric Paris <eparis@parisplace.org>; SElinux list <selinux@vger.kernel.org>; Linux kernel mailing list <linux-kernel@vger.kernel.org> 主题: Re: [PATCH] security/selinux: fix potential memleak On Mon, Dec 6, 2021 at 4:12 AM Paul Moore <paul@paul-moore.com> wrote: > On Wed, Dec 1, 2021 at 10:35 PM Bernard Zhao <bernard@vivo.com> wrote: > > > > This patch try to fix potential memleak in function > > selinux_fs_context_dup`s error branch. > > > > Signed-off-by: Bernard Zhao <bernard@vivo.com> > > --- > > security/selinux/hooks.c | 22 ++++++++++++++++++---- > > 1 file changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 62d30c0a30c2..36d7fc373839 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2856,24 +2856,38 @@ static int selinux_fs_context_dup(struct fs_context *fc, > > if (src->fscontext) { > > opts->fscontext = kstrdup(src->fscontext, GFP_KERNEL); > > if (!opts->fscontext) > > - return -ENOMEM; > > + goto err_fscontext; > > } > > if (src->context) { > > opts->context = kstrdup(src->context, GFP_KERNEL); > > if (!opts->context) > > - return -ENOMEM; > > + goto err_context; > > } > > if (src->rootcontext) { > > opts->rootcontext = kstrdup(src->rootcontext, GFP_KERNEL); > > if (!opts->rootcontext) > > - return -ENOMEM; > > + goto err_rootcontext; > > } > > if (src->defcontext) { > > opts->defcontext = kstrdup(src->defcontext, GFP_KERNEL); > > if (!opts->defcontext) > > - return -ENOMEM; > > + goto err_defcontext; > > } > > return 0; > > + > > +err_defcontext: > > + if (src->rootcontext) > > + kfree(opts->rootcontext); > > +err_rootcontext: > > + if (src->context) > > + kfree(opts->context); > > +err_context: > > + if (src->fscontext) > > + kfree(opts->fscontext); > > +err_fscontext: > > + kfree(fc->security); >Also here you need to be careful to not double-free fc->security. >(Paul's pseudocode below already correctly resets it to NULL after >freeing.) Hi Ondrej Mosnacek: Thanks for your comments! I will modify it and resubmit one patch, thanks! BR//Bernard > > + > > + return -ENOMEM; > > } > > Thanks for catching this a providing a patch, however I think the > memory cleanup can be made simpler, see the pseudo code below: > > static int selinux_fs_context_dup(...) { > > fc->security = kzalloc(...); > if (!fc->security) > return -ENOMEM; > > opts = fc->security; > > if (src->fscontext) { > opts->fscontext = kstrdup(...); > if (!opts->fscontext) > goto err; > } > > /* ... */ > > return 0; > > err: > kfree(opts->fscontext); > kfree(opts->context); > kfree(opts->rootcontext); > kfree(opts->defcontext); > kfree(opts); > fc->security = NULL; > return -ENOMEM; > } > > -- > paul moore > www.paul-moore.com > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 62d30c0a30c2..36d7fc373839 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2856,24 +2856,38 @@ static int selinux_fs_context_dup(struct fs_context *fc, if (src->fscontext) { opts->fscontext = kstrdup(src->fscontext, GFP_KERNEL); if (!opts->fscontext) - return -ENOMEM; + goto err_fscontext; } if (src->context) { opts->context = kstrdup(src->context, GFP_KERNEL); if (!opts->context) - return -ENOMEM; + goto err_context; } if (src->rootcontext) { opts->rootcontext = kstrdup(src->rootcontext, GFP_KERNEL); if (!opts->rootcontext) - return -ENOMEM; + goto err_rootcontext; } if (src->defcontext) { opts->defcontext = kstrdup(src->defcontext, GFP_KERNEL); if (!opts->defcontext) - return -ENOMEM; + goto err_defcontext; } return 0; + +err_defcontext: + if (src->rootcontext) + kfree(opts->rootcontext); +err_rootcontext: + if (src->context) + kfree(opts->context); +err_context: + if (src->fscontext) + kfree(opts->fscontext); +err_fscontext: + kfree(fc->security); + + return -ENOMEM; } static const struct fs_parameter_spec selinux_fs_parameters[] = {
This patch try to fix potential memleak in function selinux_fs_context_dup`s error branch. Signed-off-by: Bernard Zhao <bernard@vivo.com> --- security/selinux/hooks.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)