| Message ID | 20220611090550.135674-1-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [-next] selinux: Fix potential memory leak in selinux_add_opt | expand |
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.
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.
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.
在 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. >
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)
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(-)