[PATCH -next] selinux: Fix potential memory leak in selinux_add_opt

[PATCH -next] selinux: Fix potential memory leak in selinux_add_opt

[Xiu Jianfeng]

In the entry of selinux_add_opt, *mnt_opts may be assigned to new
allocated memory, and also may be freed and reset at the end of the
function. however, if security_context_str_to_sid failed, it returns
directly and skips the procedure for free and reset, even if it may be
handled at the caller of this function, It is better to handle it
inside.

Fixes: 70f4169ab421 ("selinux: parse contexts for mount options early")
Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 security/selinux/hooks.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4af4986d3893..3d67c1dab2c6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -949,7 +949,7 @@ 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;
+	int rc = -EINVAL;
 
 	if (token == Opt_seclabel)
 		/* eaten and completely ignored */
@@ -993,13 +993,15 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts)
 		break;
 	default:
 		WARN_ON(1);
-		return -EINVAL;
+		goto err;
 	}
 	rc = security_context_str_to_sid(&selinux_state, s, dst_sid, GFP_KERNEL);
-	if (rc)
+	if (rc) {
 		pr_warn("SELinux: security_context_str_to_sid (%s) failed with errno=%d\n",
 			s, rc);
-	return rc;
+		goto err;
+	}
+	return 0;
 
 err:
 	if (is_alloc_opts) {
@@ -1007,7 +1009,7 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts)
 		*mnt_opts = NULL;
 	}
 	pr_warn(SEL_MOUNT_FAIL_MSG);
-	return -EINVAL;
+	return rc;
 }
 
 static int show_sid(struct seq_file *m, u32 sid)
-- 
2.17.1

Re: [PATCH -next] selinux: Fix potential memory leak in selinux_add_opt

[Paul Moore]

On Sat, Jun 11, 2022 at 5:07 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
>
> In the entry of selinux_add_opt, *mnt_opts may be assigned to new
> allocated memory, and also may be freed and reset at the end of the
> function. however, if security_context_str_to_sid failed, it returns
> directly and skips the procedure for free and reset, even if it may be
> handled at the caller of this function, It is better to handle it
> inside.
>
> Fixes: 70f4169ab421 ("selinux: parse contexts for mount options early")
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>  security/selinux/hooks.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Have you actually observed a memory leak from the selinux_mnt_opts
allocation in selinux_add_opt()?

The selinux_add_opt() function has two callers:
selinux_sb_eat_lsm_opts() and selinux_fs_context_parse_param().  The
former cleans up the selinux_mnt_opts allocation it its error handler
while the latter will end up calling
security_free_mnt_opts()/selinux_free_mnt_opts() to free the
fs_context:security when the fs_context is destroyed.

This patch shouldn't be necessary.

-- 
paul-moore.com

Re: [PATCH -next] selinux: Fix potential memory leak in selinux_add_opt

[xiujianfeng]

Hi,

在 2022/6/14 4:22, Paul Moore 写道:
> On Sat, Jun 11, 2022 at 5:07 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
>> In the entry of selinux_add_opt, *mnt_opts may be assigned to new
>> allocated memory, and also may be freed and reset at the end of the
>> function. however, if security_context_str_to_sid failed, it returns
>> directly and skips the procedure for free and reset, even if it may be
>> handled at the caller of this function, It is better to handle it
>> inside.
>>
>> Fixes: 70f4169ab421 ("selinux: parse contexts for mount options early")
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>> ---
>>   security/selinux/hooks.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
> Have you actually observed a memory leak from the selinux_mnt_opts
> allocation in selinux_add_opt()?
>
> The selinux_add_opt() function has two callers:
> selinux_sb_eat_lsm_opts() and selinux_fs_context_parse_param().  The
> former cleans up the selinux_mnt_opts allocation it its error handler
> while the latter will end up calling
> security_free_mnt_opts()/selinux_free_mnt_opts() to free the
> fs_context:security when the fs_context is destroyed.
>
> This patch shouldn't be necessary.

I may not have made it clear, I said potential means may have a third 
caller in the future. Anyway,

Yes, you are right,  currently no memleak here, because the two callers 
will do the cleanup, from this point of view,

I think the error handler as following is not necessary:

err:
         if (is_alloc_opts) {
                 kfree(opts);
                 *mnt_opts = NULL;
         }

otherwise, some error paths goto err label while others don't, It's 
confusing.

Re: [PATCH -next] selinux: Fix potential memory leak in selinux_add_opt

[Paul Moore]

On Mon, Jun 13, 2022 at 9:18 PM xiujianfeng <xiujianfeng@huawei.com> wrote:
> 在 2022/6/14 4:22, Paul Moore 写道:
> > On Sat, Jun 11, 2022 at 5:07 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
> >> In the entry of selinux_add_opt, *mnt_opts may be assigned to new
> >> allocated memory, and also may be freed and reset at the end of the
> >> function. however, if security_context_str_to_sid failed, it returns
> >> directly and skips the procedure for free and reset, even if it may be
> >> handled at the caller of this function, It is better to handle it
> >> inside.
> >>
> >> Fixes: 70f4169ab421 ("selinux: parse contexts for mount options early")
> >> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> >> ---
> >>   security/selinux/hooks.c | 12 +++++++-----
> >>   1 file changed, 7 insertions(+), 5 deletions(-)
> > Have you actually observed a memory leak from the selinux_mnt_opts
> > allocation in selinux_add_opt()?
> >
> > The selinux_add_opt() function has two callers:
> > selinux_sb_eat_lsm_opts() and selinux_fs_context_parse_param().  The
> > former cleans up the selinux_mnt_opts allocation it its error handler
> > while the latter will end up calling
> > security_free_mnt_opts()/selinux_free_mnt_opts() to free the
> > fs_context:security when the fs_context is destroyed.
> >
> > This patch shouldn't be necessary.
>
> I may not have made it clear, I said potential means may have a third
> caller in the future.

Let's not worry about it.  If you wanted to add a comment header to
the function (see selinux_skb_peerlbl_sid() for an example) to make it
clear that callers are responsible for cleaning up @mnt_opts on error
I think that would be okay ... although even that is going to be a
problem in the new mount API case where selinux_add_opt() is going to
be called multiple times.

> I think the error handler as following is not necessary:
>
> err:
>          if (is_alloc_opts) {
>                  kfree(opts);
>                  *mnt_opts = NULL;
>          }
>
> otherwise, some error paths goto err label while others don't, It's
> confusing.

That's a fair point.  Looking at the patch which added it, we should
probably also return EINVAL when @s is NULL instead of ENOMEM.  In
fact, in all the cases where we currently jump to @err, I think we are
guaranteed that @is_alloc_opts is false as it requires a previously
populated @opts.

If you want to submit another patch, I would suggest doing the
following in the patch:

1. Change the @s NULL check to return -EINVAL when @s is NULL.
2. Allocate @opts/@mnt_opts if NULL, but don't call kfree() on the
object in case of error.  The new mount API will cleanup when it is
done and selinux_sb_eat_lsm_opts() will cleanup on error.

If you don't have time to put together a patch for this, let me know and I will.

-- 
paul-moore.com

Re: [PATCH -next] selinux: Fix potential memory leak in selinux_add_opt

[xiujianfeng]

在 2022/6/15 9:17, Paul Moore 写道:
> On Mon, Jun 13, 2022 at 9:18 PM xiujianfeng <xiujianfeng@huawei.com> wrote:
>> 在 2022/6/14 4:22, Paul Moore 写道:
>>> On Sat, Jun 11, 2022 at 5:07 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
>>>> In the entry of selinux_add_opt, *mnt_opts may be assigned to new
>>>> allocated memory, and also may be freed and reset at the end of the
>>>> function. however, if security_context_str_to_sid failed, it returns
>>>> directly and skips the procedure for free and reset, even if it may be
>>>> handled at the caller of this function, It is better to handle it
>>>> inside.
>>>>
>>>> Fixes: 70f4169ab421 ("selinux: parse contexts for mount options early")
>>>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>>>> ---
>>>>    security/selinux/hooks.c | 12 +++++++-----
>>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>> Have you actually observed a memory leak from the selinux_mnt_opts
>>> allocation in selinux_add_opt()?
>>>
>>> The selinux_add_opt() function has two callers:
>>> selinux_sb_eat_lsm_opts() and selinux_fs_context_parse_param().  The
>>> former cleans up the selinux_mnt_opts allocation it its error handler
>>> while the latter will end up calling
>>> security_free_mnt_opts()/selinux_free_mnt_opts() to free the
>>> fs_context:security when the fs_context is destroyed.
>>>
>>> This patch shouldn't be necessary.
>> I may not have made it clear, I said potential means may have a third
>> caller in the future.
> Let's not worry about it.  If you wanted to add a comment header to
> the function (see selinux_skb_peerlbl_sid() for an example) to make it
> clear that callers are responsible for cleaning up @mnt_opts on error
> I think that would be okay ... although even that is going to be a
> problem in the new mount API case where selinux_add_opt() is going to
> be called multiple times.
>
>> I think the error handler as following is not necessary:
>>
>> err:
>>           if (is_alloc_opts) {
>>                   kfree(opts);
>>                   *mnt_opts = NULL;
>>           }
>>
>> otherwise, some error paths goto err label while others don't, It's
>> confusing.
> That's a fair point.  Looking at the patch which added it, we should
> probably also return EINVAL when @s is NULL instead of ENOMEM.  In
> fact, in all the cases where we currently jump to @err, I think we are
> guaranteed that @is_alloc_opts is false as it requires a previously
> populated @opts.
>
> If you want to submit another patch, I would suggest doing the
> following in the patch:
>
> 1. Change the @s NULL check to return -EINVAL when @s is NULL.
> 2. Allocate @opts/@mnt_opts if NULL, but don't call kfree() on the
> object in case of error.  The new mount API will cleanup when it is
> done and selinux_sb_eat_lsm_opts() will cleanup on error.
>
> If you don't have time to put together a patch for this, let me know and I will.

no problem, I will do it, thanks for your suggestion.

>