在 2022/6/15 9:17, Paul Moore 写道:
On Mon, Jun 13, 2022 at 9:18 PM xiujianfeng <xiujianfeng@xxxxxxxxxx> wrote:
在 2022/6/14 4:22, Paul Moore 写道:
On Sat, Jun 11, 2022 at 5:07 AM Xiu Jianfeng <xiujianfeng@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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.
