[PATCH -next] selinux: cleanup obsolete comments for selinux_hooks[]

[PATCH -next] selinux: cleanup obsolete comments for selinux_hooks[]

[Xiu Jianfeng]

After commit f22f9aaf6c3d ("selinux: remove the runtime disable
functionality"), the order in selinux_hooks[] does not affect
any selinux function, so remove the comments.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 security/selinux/hooks.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2906fdaf7371..ef813970cb9c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6951,21 +6951,6 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
 }
 #endif /* CONFIG_IO_URING */
 
-/*
- * IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
- * 1. any hooks that don't belong to (2.) or (3.) below,
- * 2. hooks that both access structures allocated by other hooks, and allocate
- *    structures that can be later accessed by other hooks (mostly "cloning"
- *    hooks),
- * 3. hooks that only allocate structures that can be later accessed by other
- *    hooks ("allocating" hooks).
- *
- * Please follow block comment delimiters in the list to keep this order.
- *
- * This ordering is needed for SELinux runtime disable to work at least somewhat
- * safely. Breaking the ordering rules above might lead to NULL pointer derefs
- * when disabling SELinux at runtime.
- */
 static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
 	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
@@ -7201,9 +7186,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(uring_cmd, selinux_uring_cmd),
 #endif
 
-	/*
-	 * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE
-	 */
 	LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup),
 	LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param),
 	LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
@@ -7211,9 +7193,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(xfrm_policy_clone_security, selinux_xfrm_policy_clone),
 #endif
 
-	/*
-	 * PUT "ALLOCATING" HOOKS HERE
-	 */
 	LSM_HOOK_INIT(msg_msg_alloc_security, selinux_msg_msg_alloc_security),
 	LSM_HOOK_INIT(msg_queue_alloc_security,
 		      selinux_msg_queue_alloc_security),
-- 
2.34.1

Re: [PATCH -next] selinux: cleanup obsolete comments for selinux_hooks[]

[Paul Moore]

On Mon, Jul 31, 2023 at 9:39 PM Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
>
> After commit f22f9aaf6c3d ("selinux: remove the runtime disable
> functionality"), the order in selinux_hooks[] does not affect
> any selinux function, so remove the comments.
>
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>  security/selinux/hooks.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2906fdaf7371..ef813970cb9c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6951,21 +6951,6 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
>  }
>  #endif /* CONFIG_IO_URING */
>
> -/*
> - * IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
> - * 1. any hooks that don't belong to (2.) or (3.) below,
> - * 2. hooks that both access structures allocated by other hooks, and allocate
> - *    structures that can be later accessed by other hooks (mostly "cloning"
> - *    hooks),
> - * 3. hooks that only allocate structures that can be later accessed by other
> - *    hooks ("allocating" hooks).
> - *
> - * Please follow block comment delimiters in the list to keep this order.
> - *
> - * This ordering is needed for SELinux runtime disable to work at least somewhat
> - * safely. Breaking the ordering rules above might lead to NULL pointer derefs
> - * when disabling SELinux at runtime.
> - */

I don't mind the hook ordering message, even if it is not strictly
necessary anymore, so let's keep it for now.  However, if you wanted
to remove that last paragraph about it being needed to support the
runtime disable functionality that would be okay.

>  static struct security_hook_list selinux_hooks[] __ro_after_init = {
>         LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
>         LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
> @@ -7201,9 +7186,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>         LSM_HOOK_INIT(uring_cmd, selinux_uring_cmd),
>  #endif
>
> -       /*
> -        * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE
> -        */
>         LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup),
>         LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param),
>         LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
> @@ -7211,9 +7193,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>         LSM_HOOK_INIT(xfrm_policy_clone_security, selinux_xfrm_policy_clone),
>  #endif
>
> -       /*
> -        * PUT "ALLOCATING" HOOKS HERE
> -        */
>         LSM_HOOK_INIT(msg_msg_alloc_security, selinux_msg_msg_alloc_security),
>         LSM_HOOK_INIT(msg_queue_alloc_security,
>                       selinux_msg_queue_alloc_security),
> --
> 2.34.1

-- 
paul-moore.com

Re: [PATCH -next] selinux: cleanup obsolete comments for selinux_hooks[]

[xiujianfeng]

Hi,

On 2023/8/4 10:25, Paul Moore wrote:
> On Mon, Jul 31, 2023 at 9:39 PM Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
>>
>> After commit f22f9aaf6c3d ("selinux: remove the runtime disable
>> functionality"), the order in selinux_hooks[] does not affect
>> any selinux function, so remove the comments.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>> ---
>>  security/selinux/hooks.c | 21 ---------------------
>>  1 file changed, 21 deletions(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 2906fdaf7371..ef813970cb9c 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6951,21 +6951,6 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
>>  }
>>  #endif /* CONFIG_IO_URING */
>>
>> -/*
>> - * IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
>> - * 1. any hooks that don't belong to (2.) or (3.) below,
>> - * 2. hooks that both access structures allocated by other hooks, and allocate
>> - *    structures that can be later accessed by other hooks (mostly "cloning"
>> - *    hooks),
>> - * 3. hooks that only allocate structures that can be later accessed by other
>> - *    hooks ("allocating" hooks).
>> - *
>> - * Please follow block comment delimiters in the list to keep this order.
>> - *
>> - * This ordering is needed for SELinux runtime disable to work at least somewhat
>> - * safely. Breaking the ordering rules above might lead to NULL pointer derefs
>> - * when disabling SELinux at runtime.
>> - */
> 
> I don't mind the hook ordering message, even if it is not strictly
> necessary anymore, so let's keep it for now.  However, if you wanted
> to remove that last paragraph about it being needed to support the
> runtime disable functionality that would be okay.

Thanks, I will send another one.

> 
>>  static struct security_hook_list selinux_hooks[] __ro_after_init = {
>>         LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
>>         LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>> @@ -7201,9 +7186,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>>         LSM_HOOK_INIT(uring_cmd, selinux_uring_cmd),
>>  #endif
>>
>> -       /*
>> -        * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE
>> -        */
>>         LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup),
>>         LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param),
>>         LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
>> @@ -7211,9 +7193,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>>         LSM_HOOK_INIT(xfrm_policy_clone_security, selinux_xfrm_policy_clone),
>>  #endif
>>
>> -       /*
>> -        * PUT "ALLOCATING" HOOKS HERE
>> -        */
>>         LSM_HOOK_INIT(msg_msg_alloc_security, selinux_msg_msg_alloc_security),
>>         LSM_HOOK_INIT(msg_queue_alloc_security,
>>                       selinux_msg_queue_alloc_security),
>> --
>> 2.34.1
>