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

[PATCH RESEND -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 to free memory 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 changes 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..9d08b91e05a2 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 RESEND -next] selinux: Let the caller free the momory in *mnt_opts on error

[Paul Moore]

On Fri, Jun 17, 2022 at 5:46 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
>
> It may allocate memory for @mnt_opts if NULL in selinux_add_opt(), and
> now some error paths goto @err label to free memory 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 changes 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(-)

Thanks, merged into selinux/next with some rewording of the subject
line and commit description.

-- 
paul-moore.com