Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Hou Tao <houtao1@huawei.com>, Chen Jun <chenjun102@huawei.com>,
        linux-kernel@vger.kernel.org, selinux@vger.kernel.org
Cc:     stephen.smalley.work@gmail.com, eparis@parisplace.org,
        rui.xiang@huawei.com, guohanjun@huawei.com,
        Casey Schaufler <casey@schaufler-ca.com>
References: <20201028020615.8789-1-chenjun102@huawei.com>
 <c2eba2fb-79f8-eb48-ddd1-77fbc205ebff@schaufler-ca.com>
 <c9e1646e-f242-4d76-f482-f6281585860e@huawei.com>
From:   Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [RFC PATCH v2] selinux: Fix kmemleak after disabling selinux
 runtime
Message-ID: <70f2c67a-eb8a-5c28-9f78-838b397370b3@schaufler-ca.com>
Date:   Fri, 30 Oct 2020 05:33:51 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.3
MIME-Version: 1.0
In-Reply-To: <c9e1646e-f242-4d76-f482-f6281585860e@huawei.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Content-Length: 8159
Precedence: bulk

On 10/30/2020 12:57 AM, Hou Tao wrote:
> Hi,
>
> On 2020/10/29 0:29, Casey Schaufler wrote:
>> On 10/27/2020 7:06 PM, Chen Jun wrote:
>>> From: Chen Jun <c00424029@huawei.com>
>>>
>>> Kmemleak will report a problem after using
>>> "echo 1 > /sys/fs/selinux/disable" to disable selinux on runtime.
>> Runtime disable of SELinux has been deprecated. It would be
>> wasteful to make these changes in support of a facility that
>> is going away.
>>
> But this sysfs file will still be present and workable on LTS kernel ve=
rsions, so
> is the proposed fixe OK for these LTS kernel versions ?

It's not my call to make. Paul Moore has the voice that matters here.
I think that the trivial memory leak here is inconsequential compared
to the overhead you're introducing by leaving the NO_DEL hooks enabled.

>
> Regards,
> Tao
>
>
>>> kmemleak report=EF=BC=9A
>>> unreferenced object 0xffff901281c208a0 (size 96):
>>>   comm "swapper/0", pid 1, jiffies 4294668265 (age 692.799s)
>>>   hex dump (first 32 bytes):
>>>     00 40 c8 81 12 90 ff ff 03 00 00 00 05 00 00 00  .@..............=

>>>     03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................=

>>>   backtrace:
>>>     [<0000000014622ef8>] selinux_sb_alloc_security+0x1b/0xa0
>>>     [<00000000044914e1>] security_sb_alloc+0x1d/0x30
>>>     [<000000009f9d5ffd>] alloc_super+0xa7/0x310
>>>     [<000000003c5f0b5b>] sget_fc+0xca/0x230
>>>     [<00000000367a9996>] vfs_get_super+0x37/0x110
>>>     [<000000001c47e818>] vfs_get_tree+0x20/0xc0
>>>     [<00000000d239b404>] fc_mount+0x9/0x30
>>>     [<00000000708a102f>] vfs_kern_mount.part.36+0x6a/0x80
>>>     [<000000005db542fe>] kern_mount+0x1b/0x30
>>>     [<0000000051919f9f>] init_sel_fs+0x8b/0x119
>>>     [<000000000f328fe0>] do_one_initcall+0x3f/0x1d0
>>>     [<000000008a6ceb81>] kernel_init_freeable+0x1b4/0x1f2
>>>     [<000000003a425dcd>] kernel_init+0x5/0x110
>>>     [<000000004e8d6c9d>] ret_from_fork+0x22/0x30
>>>
>>> "echo 1 > /sys/fs/selinux/disable" will delete the hooks.
>>> Any memory alloced by calling HOOKFUNCTION (like call_int_hook(sb_all=
oc_security, 0, sb))
>>> has no chance to be freed after deleting hooks.
>>>
>>> Add a flag to mark a hook not be delete when deleting hooks.
>>>
>>> Signed-off-by: Chen Jun <chenjun102@huawei.com>
>>> ---
>>>  include/linux/lsm_hooks.h |  6 +++++-
>>>  security/selinux/hooks.c  | 20 ++++++++++----------
>>>  2 files changed, 15 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index c503f7ab8afb..85de731b0c74 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -1554,6 +1554,7 @@ struct security_hook_list {
>>>  	struct hlist_head		*head;
>>>  	union security_list_options	hook;
>>>  	char				*lsm;
>>> +	bool				no_del;
>>>  } __randomize_layout;
>>> =20
>>>  /*
>>> @@ -1582,6 +1583,8 @@ struct lsm_blob_sizes {
>>>   */
>>>  #define LSM_HOOK_INIT(HEAD, HOOK) \
>>>  	{ .head =3D &security_hook_heads.HEAD, .hook =3D { .HEAD =3D HOOK }=
 }
>>> +#define LSM_HOOK_INIT_NO_DEL(HEAD, HOOK) \
>>> +	{ .head =3D &security_hook_heads.HEAD, .hook =3D { .HEAD =3D HOOK }=
, .no_del =3D 1 }
>>> =20
>>>  extern struct security_hook_heads security_hook_heads;
>>>  extern char *lsm_names;
>>> @@ -1638,7 +1641,8 @@ static inline void security_delete_hooks(struct=
 security_hook_list *hooks,
>>>  	int i;
>>> =20
>>>  	for (i =3D 0; i < count; i++)
>>> -		hlist_del_rcu(&hooks[i].list);
>>> +		if (!hooks[i].no_del)
>>> +			hlist_del_rcu(&hooks[i].list);
>>>  }
>>>  #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>>> =20
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 6b1826fc3658..daff084fd1c7 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -6974,8 +6974,8 @@ static struct security_hook_list selinux_hooks[=
] __lsm_ro_after_init =3D {
>>>  	LSM_HOOK_INIT(bprm_committing_creds, selinux_bprm_committing_creds)=
,
>>>  	LSM_HOOK_INIT(bprm_committed_creds, selinux_bprm_committed_creds),
>>> =20
>>> -	LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
>>> -	LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
>>> +	LSM_HOOK_INIT_NO_DEL(sb_free_security, selinux_sb_free_security),
>>> +	LSM_HOOK_INIT_NO_DEL(sb_free_mnt_opts, selinux_free_mnt_opts),
>>>  	LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
>>>  	LSM_HOOK_INIT(sb_kern_mount, selinux_sb_kern_mount),
>>>  	LSM_HOOK_INIT(sb_show_options, selinux_sb_show_options),
>>> @@ -7081,7 +7081,7 @@ static struct security_hook_list selinux_hooks[=
] __lsm_ro_after_init =3D {
>>> =20
>>>  	LSM_HOOK_INIT(ismaclabel, selinux_ismaclabel),
>>>  	LSM_HOOK_INIT(secctx_to_secid, selinux_secctx_to_secid),
>>> -	LSM_HOOK_INIT(release_secctx, selinux_release_secctx),
>>> +	LSM_HOOK_INIT_NO_DEL(release_secctx, selinux_release_secctx),
>>>  	LSM_HOOK_INIT(inode_invalidate_secctx, selinux_inode_invalidate_sec=
ctx),
>>>  	LSM_HOOK_INIT(inode_notifysecctx, selinux_inode_notifysecctx),
>>>  	LSM_HOOK_INIT(inode_setsecctx, selinux_inode_setsecctx),
>>> @@ -7107,7 +7107,7 @@ static struct security_hook_list selinux_hooks[=
] __lsm_ro_after_init =3D {
>>>  	LSM_HOOK_INIT(socket_getpeersec_stream,
>>>  			selinux_socket_getpeersec_stream),
>>>  	LSM_HOOK_INIT(socket_getpeersec_dgram, selinux_socket_getpeersec_dg=
ram),
>>> -	LSM_HOOK_INIT(sk_free_security, selinux_sk_free_security),
>>> +	LSM_HOOK_INIT_NO_DEL(sk_free_security, selinux_sk_free_security),
>>>  	LSM_HOOK_INIT(sk_clone_security, selinux_sk_clone_security),
>>>  	LSM_HOOK_INIT(sk_getsecid, selinux_sk_getsecid),
>>>  	LSM_HOOK_INIT(sock_graft, selinux_sock_graft),
>>> @@ -7121,7 +7121,7 @@ static struct security_hook_list selinux_hooks[=
] __lsm_ro_after_init =3D {
>>>  	LSM_HOOK_INIT(secmark_refcount_inc, selinux_secmark_refcount_inc),
>>>  	LSM_HOOK_INIT(secmark_refcount_dec, selinux_secmark_refcount_dec),
>>>  	LSM_HOOK_INIT(req_classify_flow, selinux_req_classify_flow),
>>> -	LSM_HOOK_INIT(tun_dev_free_security, selinux_tun_dev_free_security)=
,
>>> +	LSM_HOOK_INIT_NO_DEL(tun_dev_free_security, selinux_tun_dev_free_se=
curity),
>>>  	LSM_HOOK_INIT(tun_dev_create, selinux_tun_dev_create),
>>>  	LSM_HOOK_INIT(tun_dev_attach_queue, selinux_tun_dev_attach_queue),
>>>  	LSM_HOOK_INIT(tun_dev_attach, selinux_tun_dev_attach),
>>> @@ -7130,7 +7130,7 @@ static struct security_hook_list selinux_hooks[=
] __lsm_ro_after_init =3D {
>>>  	LSM_HOOK_INIT(ib_pkey_access, selinux_ib_pkey_access),
>>>  	LSM_HOOK_INIT(ib_endport_manage_subnet,
>>>  		      selinux_ib_endport_manage_subnet),
>>> -	LSM_HOOK_INIT(ib_free_security, selinux_ib_free_security),
>>> +	LSM_HOOK_INIT_NO_DEL(ib_free_security, selinux_ib_free_security),
>>>  #endif
>>>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
>>>  	LSM_HOOK_INIT(xfrm_policy_free_security, selinux_xfrm_policy_free),=

>>> @@ -7144,7 +7144,7 @@ static struct security_hook_list selinux_hooks[=
] __lsm_ro_after_init =3D {
>>>  #endif
>>> =20
>>>  #ifdef CONFIG_KEYS
>>> -	LSM_HOOK_INIT(key_free, selinux_key_free),
>>> +	LSM_HOOK_INIT_NO_DEL(key_free, selinux_key_free),
>>>  	LSM_HOOK_INIT(key_permission, selinux_key_permission),
>>>  	LSM_HOOK_INIT(key_getsecurity, selinux_key_getsecurity),
>>>  #ifdef CONFIG_KEY_NOTIFICATIONS
>>> @@ -7162,13 +7162,13 @@ static struct security_hook_list selinux_hook=
s[] __lsm_ro_after_init =3D {
>>>  	LSM_HOOK_INIT(bpf, selinux_bpf),
>>>  	LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
>>>  	LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
>>> -	LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
>>> -	LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
>>> +	LSM_HOOK_INIT_NO_DEL(bpf_map_free_security, selinux_bpf_map_free),
>>> +	LSM_HOOK_INIT_NO_DEL(bpf_prog_free_security, selinux_bpf_prog_free)=
,
>>>  #endif
>>> =20
>>>  #ifdef CONFIG_PERF_EVENTS
>>>  	LSM_HOOK_INIT(perf_event_open, selinux_perf_event_open),
>>> -	LSM_HOOK_INIT(perf_event_free, selinux_perf_event_free),
>>> +	LSM_HOOK_INIT_NO_DEL(perf_event_free, selinux_perf_event_free),
>>>  	LSM_HOOK_INIT(perf_event_read, selinux_perf_event_read),
>>>  	LSM_HOOK_INIT(perf_event_write, selinux_perf_event_write),
>>>  #endif
>> .
>>