| Message ID | 20220616115052.221803-1-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [-next] selinux: Let the caller free the momory in *mnt_opts on error | expand |
Hi Xiu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20220616]
url: https://github.com/intel-lab-lkp/linux/commits/Xiu-Jianfeng/selinux-Let-the-caller-free-the-momory-in-mnt_opts-on-error/20220616-195514
base: c6d7e3b385f19869ab96e9404c92ff1abc34f2c8
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220616/202206162324.W061Fv9o-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/ea1d224d611591b835ce446dea3e769eb2d5492f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Xiu-Jianfeng/selinux-Let-the-caller-free-the-momory-in-mnt_opts-on-error/20220616-195514
git checkout ea1d224d611591b835ce446dea3e769eb2d5492f
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
security/selinux/hooks.c:951: warning: Function parameter or member 'token' not described in 'selinux_add_opt'
security/selinux/hooks.c:951: warning: Function parameter or member 's' not described in 'selinux_add_opt'
security/selinux/hooks.c:951: warning: Function parameter or member 'mnt_opts' not described in 'selinux_add_opt'
>> security/selinux/hooks.c:951: warning: expecting prototype for NOTE(). Prototype was for selinux_add_opt() instead
vim +951 security/selinux/hooks.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 946
ea1d224d611591 Xiu Jianfeng 2022-06-16 947 /**
ea1d224d611591 Xiu Jianfeng 2022-06-16 948 * NOTE: the caller is resposible for freeing the memory even if on error.
ea1d224d611591 Xiu Jianfeng 2022-06-16 949 */
ba6418623385ab Al Viro 2018-12-14 950 static int selinux_add_opt(int token, const char *s, void **mnt_opts)
^1da177e4c3f41 Linus Torvalds 2005-04-16 @951 {
bd3236557bb256 Al Viro 2018-12-13 952 struct selinux_mnt_opts *opts = *mnt_opts;
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 953 u32 *dst_sid;
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 954 int rc;
c9180a57a9ab2d Eric Paris 2007-11-30 955
6cd9d4b9789156 Paul Moore 2021-12-21 956 if (token == Opt_seclabel)
6cd9d4b9789156 Paul Moore 2021-12-21 957 /* eaten and completely ignored */
169d68efb03b72 Al Viro 2018-12-14 958 return 0;
2e08df3c7c4e4e Bernard Zhao 2021-12-10 959 if (!s)
ea1d224d611591 Xiu Jianfeng 2022-06-16 960 return -EINVAL;
c9180a57a9ab2d Eric Paris 2007-11-30 961
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 962 if (!selinux_initialized(&selinux_state)) {
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 963 pr_warn("SELinux: Unable to set superblock options before the security server is initialized\n");
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 964 return -EINVAL;
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 965 }
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 966
bd3236557bb256 Al Viro 2018-12-13 967 if (!opts) {
6cd9d4b9789156 Paul Moore 2021-12-21 968 opts = kzalloc(sizeof(*opts), GFP_KERNEL);
bd3236557bb256 Al Viro 2018-12-13 969 if (!opts)
bd3236557bb256 Al Viro 2018-12-13 970 return -ENOMEM;
ba6418623385ab Al Viro 2018-12-14 971 *mnt_opts = opts;
bd3236557bb256 Al Viro 2018-12-13 972 }
2e08df3c7c4e4e Bernard Zhao 2021-12-10 973
c9180a57a9ab2d Eric Paris 2007-11-30 974 switch (token) {
c9180a57a9ab2d Eric Paris 2007-11-30 975 case Opt_context:
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 976 if (opts->context_sid || opts->defcontext_sid)
6cd9d4b9789156 Paul Moore 2021-12-21 977 goto err;
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 978 dst_sid = &opts->context_sid;
c9180a57a9ab2d Eric Paris 2007-11-30 979 break;
c9180a57a9ab2d Eric Paris 2007-11-30 980 case Opt_fscontext:
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 981 if (opts->fscontext_sid)
6cd9d4b9789156 Paul Moore 2021-12-21 982 goto err;
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 983 dst_sid = &opts->fscontext_sid;
c9180a57a9ab2d Eric Paris 2007-11-30 984 break;
c9180a57a9ab2d Eric Paris 2007-11-30 985 case Opt_rootcontext:
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 986 if (opts->rootcontext_sid)
6cd9d4b9789156 Paul Moore 2021-12-21 987 goto err;
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 988 dst_sid = &opts->rootcontext_sid;
c9180a57a9ab2d Eric Paris 2007-11-30 989 break;
c9180a57a9ab2d Eric Paris 2007-11-30 990 case Opt_defcontext:
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 991 if (opts->context_sid || opts->defcontext_sid)
6cd9d4b9789156 Paul Moore 2021-12-21 992 goto err;
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 993 dst_sid = &opts->defcontext_sid;
11689d47f09571 David P. Quigley 2009-01-16 994 break;
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 995 default:
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 996 WARN_ON(1);
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 997 return -EINVAL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 998 }
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 999 rc = security_context_str_to_sid(&selinux_state, s, dst_sid, GFP_KERNEL);
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 1000 if (rc)
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 1001 pr_warn("SELinux: security_context_str_to_sid (%s) failed with errno=%d\n",
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 1002 s, rc);
70f4169ab421b2 Ondrej Mosnacek 2022-02-02 1003 return rc;
e2e0e09758a6f7 Gen Zhang 2019-06-12 1004
6cd9d4b9789156 Paul Moore 2021-12-21 1005 err:
ba6418623385ab Al Viro 2018-12-14 1006 pr_warn(SEL_MOUNT_FAIL_MSG);
ba6418623385ab Al Viro 2018-12-14 1007 return -EINVAL;
e0007529893c1c Eric Paris 2008-03-05 1008 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 1009
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 4d20a139a86d..0ec03126ac8e 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -944,10 +944,12 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb, return rc; } +/** + * NOTE: the caller is resposible for freeing the memory even if on error. + */ static int selinux_add_opt(int token, const char *s, void **mnt_opts) { struct selinux_mnt_opts *opts = *mnt_opts; - bool is_alloc_opts = false; u32 *dst_sid; int rc; @@ -955,7 +957,7 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) /* eaten and completely ignored */ return 0; if (!s) - return -ENOMEM; + return -EINVAL; if (!selinux_initialized(&selinux_state)) { pr_warn("SELinux: Unable to set superblock options before the security server is initialized\n"); @@ -967,7 +969,6 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) if (!opts) return -ENOMEM; *mnt_opts = opts; - is_alloc_opts = true; } switch (token) { @@ -1002,10 +1003,6 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) return rc; err: - if (is_alloc_opts) { - kfree(opts); - *mnt_opts = NULL; - } pr_warn(SEL_MOUNT_FAIL_MSG); return -EINVAL; }
It may allocate memory for @mnt_opts if NULL in selinux_add_opt(), and now some error paths goto @err label while others don't, as suggested by Paul, don't free memory in case of error and let the caller to cleanup on error. And also this patch change the @s NULL check to return -EINVAL instead. Link: https://lore.kernel.org/lkml/20220611090550.135674-1-xiujianfeng@huawei.com/T/ Suggested-by: Paul Moore <paul@paul-moore.com> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> --- security/selinux/hooks.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)