Message ID | 20190601021526.GA8264@zhanggen-UX430UQ (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v3] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_opts() | expand |
On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang 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. What's the latter one for? On failure we'll get to put_fs_context() pretty soon, so security_free_mnt_opts(&fc->security); will be called just fine. Leaving it allocated on failure is fine...
On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote: > On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang 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. > > What's the latter one for? On failure we'll get to put_fs_context() > pretty soon, so > security_free_mnt_opts(&fc->security); > will be called just fine. Leaving it allocated on failure is fine... Actually, right now in mainline it is not (btrfs_mount_root() has an odd call of security_sb_eat_lsm_opts()); eventually we will be down to just the callers in ->parse_monolithic() instances, at which point the above will become correct. At the moment it is not, so consider the objection withdrawn (and I really need to get some sleep, seeing how long did it take me to recall the context... ;-/)
On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote: > On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang 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. > > What's the latter one for? On failure we'll get to put_fs_context() > pretty soon, so > security_free_mnt_opts(&fc->security); > will be called just fine. Leaving it allocated on failure is fine... Paul Moore <paul@paul-moore.com> wrote: >It seems like we should also check for, and potentially free *mnt_opts >as the selinux_add_opt() error handling does just below this change, >yes? If that is the case we might want to move that error handling >code to the bottom of the function and jump there on error. I am not familiar with this part. So could you please show the function call sequence? Thanks Gen
On Sat, Jun 01, 2019 at 03:34:49AM +0100, Al Viro wrote: > On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote: > > On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang 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. > > > > What's the latter one for? On failure we'll get to put_fs_context() > > pretty soon, so > > security_free_mnt_opts(&fc->security); > > will be called just fine. Leaving it allocated on failure is fine... > > Actually, right now in mainline it is not (btrfs_mount_root() has > an odd call of security_sb_eat_lsm_opts()); eventually we will be > down to just the callers in ->parse_monolithic() instances, at which > point the above will become correct. At the moment it is not, so > consider the objection withdrawn (and I really need to get some sleep, > seeing how long did it take me to recall the context... ;-/) Thanks for your comments. And have a good dream. Thanks Gen
On Sat, Jun 1, 2019 at 4:15 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> > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> It looks like you're new to the kernel development community, so let me give you a bit of friendly advice for the future :) You don't need to repost the patch when people give you Acked-by/Reviewed-by/Tested-by (unless there is a different reason to respin/repost the patches). The maintainer goes over the replies when applying the final patch and adds Acked-by/Reviewed-by/... on his/her own. If you *do* need to respin a path for which you have received A/R/T, then you need to distinguish between two cases: 1. Only trivial changes to the patch (only fixed typos, edited commit message, removed empty line, etc. - for example, v1 -> v2 of this patch falls into this category) - in this case you can collect the A/R/T yourself and add them to the new version. This saves the maintainer and the reviewers from redundant work, since the patch is still semantically the same and the A/R/T from the last version still apply. 2. Non-trivial changes to the patch (as is the case for this patch) - in this case your patch needs to be reviewed again and you should disregard all A/R/T from the previous version. You can easily piss someone off if you add their Reviewed-by to a patch they haven't actually reviewed, so be careful ;-) (Someone please correct me if I got it wrong - this is what I gathered so far from my experience.) Good luck in your future work! > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()") > --- > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 3ec702c..f329fc0 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2616,6 +2616,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) > char *from = options; > char *to = options; > bool first = true; > + int ret; > > while (1) { > int len = opt_len(from); > @@ -2635,15 +2636,16 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) > *q++ = c; > } > arg = kmemdup_nul(arg, q - arg, GFP_KERNEL); > + if (!arg) { > + ret = -ENOMEM; > + goto free_opt; > + } > } > rc = selinux_add_opt(token, arg, mnt_opts); > if (unlikely(rc)) { > + ret = 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 +2663,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 ret; > } > > static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
On Sat, Jun 1, 2019 at 4:15 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> > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()") > --- > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 3ec702c..f329fc0 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2616,6 +2616,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) > char *from = options; > char *to = options; > bool first = true; > + int ret; I'd suggest just moving the declaration of 'rc' here and simply reuse that variable. Otherwise the patch looks good to me. > > while (1) { > int len = opt_len(from); > @@ -2635,15 +2636,16 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) > *q++ = c; > } > arg = kmemdup_nul(arg, q - arg, GFP_KERNEL); > + if (!arg) { > + ret = -ENOMEM; > + goto free_opt; > + } > } > rc = selinux_add_opt(token, arg, mnt_opts); > if (unlikely(rc)) { > + ret = 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 +2663,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 ret; > } > > static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
On Mon, Jun 3, 2019 at 3:27 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Sat, Jun 1, 2019 at 4:15 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> > > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()") > > --- > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 3ec702c..f329fc0 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2616,6 +2616,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) > > char *from = options; > > char *to = options; > > bool first = true; > > + int ret; > > I'd suggest just moving the declaration of 'rc' here and simply reuse > that variable. Otherwise the patch looks good to me. Agreed. Creating "ret" only makes the patch larger and doesn't add any value. I try to avoid making broad statements, but if you are unsure about which approach to take when fixing a problem, start with the smallest patch you can write. Even if it turns out not to be the "best" solution upstream, it will be easier to review, discuss, and potentially port to other/older kernels. > > > > while (1) { > > int len = opt_len(from); > > @@ -2635,15 +2636,16 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) > > *q++ = c; > > } > > arg = kmemdup_nul(arg, q - arg, GFP_KERNEL); > > + if (!arg) { > > + ret = -ENOMEM; > > + goto free_opt; > > + } > > } > > rc = selinux_add_opt(token, arg, mnt_opts); > > if (unlikely(rc)) { > > + ret = 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 +2663,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 ret; > > } > > > > 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, May 31, 2019 at 10:45 PM Gen Zhang <blackgod016574@gmail.com> wrote: > On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote: > > On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang 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. > > > > What's the latter one for? On failure we'll get to put_fs_context() > > pretty soon, so > > security_free_mnt_opts(&fc->security); > > will be called just fine. Leaving it allocated on failure is fine... > Paul Moore <paul@paul-moore.com> wrote: > >It seems like we should also check for, and potentially free *mnt_opts > >as the selinux_add_opt() error handling does just below this change, > >yes? If that is the case we might want to move that error handling > >code to the bottom of the function and jump there on error. > I am not familiar with this part. So could you please show the function > call sequence? I'm not sure I understand your question above, but I did review your latest patch and agree with Ondrej's comment regarding the ret/rc variable. If you make that change I think we can merge this into selinux/stable-5.2.
On Mon, Jun 3, 2019 at 3:23 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Sat, Jun 1, 2019 at 4:15 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> > > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > > It looks like you're new to the kernel development community, so let > me give you a bit of friendly advice for the future :) > > You don't need to repost the patch when people give you > Acked-by/Reviewed-by/Tested-by (unless there is a different reason to > respin/repost the patches). The maintainer goes over the replies when > applying the final patch and adds Acked-by/Reviewed-by/... on his/her > own. > > If you *do* need to respin a path for which you have received A/R/T, > then you need to distinguish between two cases: > 1. Only trivial changes to the patch (only fixed typos, edited commit > message, removed empty line, etc. - for example, v1 -> v2 of this > patch falls into this category) - in this case you can collect the > A/R/T yourself and add them to the new version. This saves the > maintainer and the reviewers from redundant work, since the patch is > still semantically the same and the A/R/T from the last version still > apply. > 2. Non-trivial changes to the patch (as is the case for this patch) - > in this case your patch needs to be reviewed again and you should > disregard all A/R/T from the previous version. You can easily piss > someone off if you add their Reviewed-by to a patch they haven't > actually reviewed, so be careful ;-) I want to stress Ondrej's last point. Carrying over an Acked-by/Reviewed-by/Tested-by tag if you make anything more than the most trivial change in a patch is *very* bad, and will likely result in a loss of trust between you and the maintainer. If you are unsure, drop the A/R/T tag, there is *much* less harm in asking someone to re-review a patch than falsely tagging a patch as reviewed by someone when you have made substantial changes. I suspect you may have already read the Documentation/process/submitting-patches.rst file, but if you haven't it is worth reading. It covers many of the things that are discussed elsewhere. If you aren't already, you should get in the habit of doing the following for each patch you post to the mailing list: 1. Make sure it compiles cleanly, or at least doesn't introduce any new compiler warnings/errors. 2. Run ./scripts/checkpatch.pl and fix as many problems as you can; a patch can still be accepted with checkpatch warnings/errors (and some maintainers might dislike some of checkpatch's decisions), but it helps a lot if you can fix all those. 3. At the very least make sure your kernel changes boot, if you can, run the associated subsystem's test (if they have one) to verify that there are no regressions (the SELinux kernel test suite is here: https://github.com/SELinuxProject/selinux-testsuite) Lastly, when in doubt, you can always ask the mailing list; the SELinux list is a pretty friendly place :)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 3ec702c..f329fc0 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2616,6 +2616,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) char *from = options; char *to = options; bool first = true; + int ret; while (1) { int len = opt_len(from); @@ -2635,15 +2636,16 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) *q++ = c; } arg = kmemdup_nul(arg, q - arg, GFP_KERNEL); + if (!arg) { + ret = -ENOMEM; + goto free_opt; + } } rc = selinux_add_opt(token, arg, mnt_opts); if (unlikely(rc)) { + ret = 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 +2663,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 ret; } static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)