[PATCH -next] selinux: Let the caller free the momory in *mnt_opts on error

[PATCH -next] selinux: Let the caller free the momory in *mnt_opts on error

[Xiu Jianfeng]

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(-)

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;
 }
-- 
2.17.1

Re: [PATCH -next] selinux: Let the caller free the momory in *mnt_opts on error

[kernel test robot]

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  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp