Messages in this thread | | | Subject | Re: [PATCH -next] selinux: Fix potential memory leak in selinux_add_opt | From | xiujianfeng <> | Date | Tue, 14 Jun 2022 09:18:34 +0800 |
| |
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.
| |