| Message ID | 20190606085524.GA21119@zhanggen-UX430UQ (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_o pts() | expand |
On Thu, Jun 6, 2019 at 10:55 AM Gen Zhang <blackgod016574@gmail.com> wrote: > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' > should be freed when error. > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()") My comments about the subject and an empty line before label apply here as well, but Paul can fix both easily when applying, so: Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 3ec702c..13479cd 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2616,10 +2616,11 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) > char *from = options; > char *to = options; > bool first = true; > + int rc; > > while (1) { > int len = opt_len(from); > - int token, rc; > + int token; > char *arg = NULL; > > token = match_opt_prefix(from, len, &arg); > @@ -2635,15 +2636,15 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) > *q++ = c; > } > arg = kmemdup_nul(arg, q - arg, GFP_KERNEL); > + if (!arg) { > + rc = -ENOMEM; > + goto free_opt; > + } > } > rc = selinux_add_opt(token, arg, mnt_opts); > if (unlikely(rc)) { > kfree(arg); > - if (*mnt_opts) { > - selinux_free_mnt_opts(*mnt_opts); > - *mnt_opts = NULL; > - } > - return rc; > + goto free_opt; > } > } else { > if (!first) { // copy with preceding comma > @@ -2661,6 +2662,12 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) > } > *to = '\0'; > return 0; > +free_opt: > + if (*mnt_opts) { > + selinux_free_mnt_opts(*mnt_opts); > + *mnt_opts = NULL; > + } > + return rc; > } > > static int selinux_sb_remount(struct super_block *sb, void *mnt_opts) -- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc.
On Fri, Jun 7, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Jun 6, 2019 at 10:55 AM Gen Zhang <blackgod016574@gmail.com> wrote: > > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It > > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' > > should be freed when error. > > > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()") > > My comments about the subject and an empty line before label apply > here as well, but Paul can fix both easily when applying ... Since we've been discussing general best practices for submitting patches in this thread (and the other related thread), I wanted to (re)clarify my thoughts around maintainers fixing patches when merging them upstream. When in doubt, do not ever rely on the upstream maintainer fixing your patch while merging it, and if problems do arise during review, it is best to not ask the maintainer to fix them for you, but for you to fix them instead (you are the patch author after all!). Similarly, making comments along the lines of "X can fix both easily when applying", is also a bad thing to say when reviewing patches. It's the patch author's responsibility to fix the patch by address review comments, not the maintainer. I'll typically let you know if you don't need to rework a patch(set). That said, there are times when the maintainer will change the patch during merging, most of which are due to resolving merge conflicts/fuzz with changes already in the tree (that *is* the maintainer's responsibility). Speaking for myself, sometimes I will also make some minor changes if the patch author is away, or unreliable, or if there is a hard deadline near and I'm worried that the updated patch might not be ready in time. I'll also sometimes make the changes directly if the patch is holding up a larger, more important patch(set), but that is really rare. I'm sure I've made changes for other reasons in the past, and I'm sure I'll make changes for other reasons in the future, but hopefully this will give you a better idea of how the process works :)
On Mon, Jun 10, 2019 at 04:20:28PM -0400, Paul Moore wrote: > On Fri, Jun 7, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > On Thu, Jun 6, 2019 at 10:55 AM Gen Zhang <blackgod016574@gmail.com> wrote: > > > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It > > > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' > > > should be freed when error. > > > > > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > > > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()") > > > > My comments about the subject and an empty line before label apply > > here as well, but Paul can fix both easily when applying ... > > Since we've been discussing general best practices for submitting > patches in this thread (and the other related thread), I wanted to > (re)clarify my thoughts around maintainers fixing patches when merging > them upstream. > > When in doubt, do not ever rely on the upstream maintainer fixing your > patch while merging it, and if problems do arise during review, it is > best to not ask the maintainer to fix them for you, but for you to fix > them instead (you are the patch author after all!). Similarly, making > comments along the lines of "X can fix both easily when applying", is > also a bad thing to say when reviewing patches. It's the patch > author's responsibility to fix the patch by address review comments, > not the maintainer. I'll typically let you know if you don't need to > rework a patch(set). > > That said, there are times when the maintainer will change the patch > during merging, most of which are due to resolving merge > conflicts/fuzz with changes already in the tree (that *is* the > maintainer's responsibility). Speaking for myself, sometimes I will > also make some minor changes if the patch author is away, or > unreliable, or if there is a hard deadline near and I'm worried that > the updated patch might not be ready in time. I'll also sometimes > make the changes directly if the patch is holding up a larger, more > important patch(set), but that is really rare. I'm sure I've made > changes for other reasons in the past, and I'm sure I'll make changes > for other reasons in the future, but hopefully this will give you a > better idea of how the process works :) > > -- > paul moore > www.paul-moore.com Thanks for your comments. I will resend a patch after revising. Thanks Gen
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 3ec702c..13479cd 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2616,10 +2616,11 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) char *from = options; char *to = options; bool first = true; + int rc; while (1) { int len = opt_len(from); - int token, rc; + int token; char *arg = NULL; token = match_opt_prefix(from, len, &arg); @@ -2635,15 +2636,15 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) *q++ = c; } arg = kmemdup_nul(arg, q - arg, GFP_KERNEL); + if (!arg) { + rc = -ENOMEM; + goto free_opt; + } } rc = selinux_add_opt(token, arg, mnt_opts); if (unlikely(rc)) { kfree(arg); - if (*mnt_opts) { - selinux_free_mnt_opts(*mnt_opts); - *mnt_opts = NULL; - } - return rc; + goto free_opt; } } else { if (!first) { // copy with preceding comma @@ -2661,6 +2662,12 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) } *to = '\0'; return 0; +free_opt: + if (*mnt_opts) { + selinux_free_mnt_opts(*mnt_opts); + *mnt_opts = NULL; + } + return rc; } static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' should be freed when error. Signed-off-by: Gen Zhang <blackgod016574@gmail.com> Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()") ---