diff mbox

LSM: Convert security_hook_heads into explicit array of struct list_head

Message ID 1495883858-3336-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive)
State New, archived
Headers show

Commit Message

Tetsuo Handa May 27, 2017, 11:17 a.m. UTC
Commit 3dfc9b02864b19f4 ("LSM: Initialize security_hook_heads upon
registration.") treats "struct security_hook_heads" as an implicit array
of "struct list_head" so that we can eliminate code for static
initialization. Although we haven't encountered compilers which do not
treat sizeof(security_hook_heads) != sizeof(struct list_head) *
(sizeof(security_hook_heads) / sizeof(struct list_head)), Casey does not
like the assumption that a structure of N elements can be assumed to be
the same as an array of N elements.

Now that Kees found that randstruct complains such casting

  security/security.c: In function 'security_init':
  security/security.c:59:20: note: found mismatched op0 struct pointer types: 'struct list_head' and 'struct security_hook_heads'

    struct list_head *list = (struct list_head *) &security_hook_heads;

and Christoph thinks that we should fix it rather than make randstruct
whitelist it, this patch fixes it.

It would be possible to revert commit 3dfc9b02864b19f4, but this patch
converts security_hook_heads into an explicit array of struct list_head
by introducing an enum, due to reasons explained below.

In MM subsystem, a sealable memory allocator patch was proposed, and
the LSM hooks ("struct security_hook_heads security_hook_heads" and
"struct security_hook_list ...[]") will benefit from this allocator via
protection using set_memory_ro()/set_memory_rw(), and that allocator
will remove CONFIG_SECURITY_WRITABLE_HOOKS config option. Thus, we will
likely be moving to that direction.

This means that these structures will be allocated at run time using
this allocator, and therefore the address of these structures will be
determined at run time rather than compile time.

But currently, LSM_HOOK_INIT() macro depends on the address of
security_hook_heads being known at compile time. If we use an enum
so that LSM_HOOK_INIT() macro does not need to know absolute address of
security_hook_heads, it will help us to use that allocator for LSM hooks.

As a result of introducing an enum, security_hook_heads becomes a local
variable, making it easier to allocate security_hook_heads at run time.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Christoph Hellwig <hch@infradead.org>
---
 include/linux/lsm_hooks.h | 412 +++++++++++++++++++++++-----------------------
 security/security.c       |  38 +++--
 2 files changed, 229 insertions(+), 221 deletions(-)

Comments

Casey Schaufler May 27, 2017, 10:30 p.m. UTC | #1
On 5/27/2017 4:17 AM, Tetsuo Handa wrote:
> Commit 3dfc9b02864b19f4 ("LSM: Initialize security_hook_heads upon
> registration.") treats "struct security_hook_heads" as an implicit array
> of "struct list_head" so that we can eliminate code for static
> initialization. Although we haven't encountered compilers which do not
> treat sizeof(security_hook_heads) != sizeof(struct list_head) *
> (sizeof(security_hook_heads) / sizeof(struct list_head)), Casey does not
> like the assumption that a structure of N elements can be assumed to be
> the same as an array of N elements.
>
> Now that Kees found that randstruct complains such casting
>
>   security/security.c: In function 'security_init':
>   security/security.c:59:20: note: found mismatched op0 struct pointer types: 'struct list_head' and 'struct security_hook_heads'
>
>     struct list_head *list = (struct list_head *) &security_hook_heads;
>
> and Christoph thinks that we should fix it rather than make randstruct
> whitelist it, this patch fixes it.
>
> It would be possible to revert commit 3dfc9b02864b19f4, but this patch
> converts security_hook_heads into an explicit array of struct list_head
> by introducing an enum, due to reasons explained below.
>
> In MM subsystem, a sealable memory allocator patch was proposed, and
> the LSM hooks ("struct security_hook_heads security_hook_heads" and
> "struct security_hook_list ...[]") will benefit from this allocator via
> protection using set_memory_ro()/set_memory_rw(), and that allocator
> will remove CONFIG_SECURITY_WRITABLE_HOOKS config option. Thus, we will
> likely be moving to that direction.
>
> This means that these structures will be allocated at run time using
> this allocator, and therefore the address of these structures will be
> determined at run time rather than compile time.
>
> But currently, LSM_HOOK_INIT() macro depends on the address of
> security_hook_heads being known at compile time. If we use an enum
> so that LSM_HOOK_INIT() macro does not need to know absolute address of
> security_hook_heads, it will help us to use that allocator for LSM hooks.
>
> As a result of introducing an enum, security_hook_heads becomes a local
> variable, making it easier to allocate security_hook_heads at run time.

You loose the type checking in security.c. This is the same
objection I had before to this approach. It's why I objected
to 3dfc9b02864b19f4 and why I didn't adopt the array approach
in the first place. If it's so important that randstruct not
complain about the unnatural cast, revert the patch that
introduced it. I see no net benefit in hiding the symbol over
loosing the typing. You trade a list of typed function
pointers for an enumerated list of values. It doesn't even
make the code look smaller!
 

>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Igor Stoppa <igor.stoppa@huawei.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> ---
>  include/linux/lsm_hooks.h | 412 +++++++++++++++++++++++-----------------------
>  security/security.c       |  38 +++--
>  2 files changed, 229 insertions(+), 221 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 080f34e..1f98d1b 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1663,219 +1663,220 @@
>  #endif /* CONFIG_AUDIT */
>  };
>  
> -struct security_hook_heads {
> -	struct list_head binder_set_context_mgr;
> -	struct list_head binder_transaction;
> -	struct list_head binder_transfer_binder;
> -	struct list_head binder_transfer_file;
> -	struct list_head ptrace_access_check;
> -	struct list_head ptrace_traceme;
> -	struct list_head capget;
> -	struct list_head capset;
> -	struct list_head capable;
> -	struct list_head quotactl;
> -	struct list_head quota_on;
> -	struct list_head syslog;
> -	struct list_head settime;
> -	struct list_head vm_enough_memory;
> -	struct list_head bprm_set_creds;
> -	struct list_head bprm_check_security;
> -	struct list_head bprm_secureexec;
> -	struct list_head bprm_committing_creds;
> -	struct list_head bprm_committed_creds;
> -	struct list_head sb_alloc_security;
> -	struct list_head sb_free_security;
> -	struct list_head sb_copy_data;
> -	struct list_head sb_remount;
> -	struct list_head sb_kern_mount;
> -	struct list_head sb_show_options;
> -	struct list_head sb_statfs;
> -	struct list_head sb_mount;
> -	struct list_head sb_umount;
> -	struct list_head sb_pivotroot;
> -	struct list_head sb_set_mnt_opts;
> -	struct list_head sb_clone_mnt_opts;
> -	struct list_head sb_parse_opts_str;
> -	struct list_head dentry_init_security;
> -	struct list_head dentry_create_files_as;
> +enum security_hook_index {
> +	LSM_binder_set_context_mgr,
> +	LSM_binder_transaction,
> +	LSM_binder_transfer_binder,
> +	LSM_binder_transfer_file,
> +	LSM_ptrace_access_check,
> +	LSM_ptrace_traceme,
> +	LSM_capget,
> +	LSM_capset,
> +	LSM_capable,
> +	LSM_quotactl,
> +	LSM_quota_on,
> +	LSM_syslog,
> +	LSM_settime,
> +	LSM_vm_enough_memory,
> +	LSM_bprm_set_creds,
> +	LSM_bprm_check_security,
> +	LSM_bprm_secureexec,
> +	LSM_bprm_committing_creds,
> +	LSM_bprm_committed_creds,
> +	LSM_sb_alloc_security,
> +	LSM_sb_free_security,
> +	LSM_sb_copy_data,
> +	LSM_sb_remount,
> +	LSM_sb_kern_mount,
> +	LSM_sb_show_options,
> +	LSM_sb_statfs,
> +	LSM_sb_mount,
> +	LSM_sb_umount,
> +	LSM_sb_pivotroot,
> +	LSM_sb_set_mnt_opts,
> +	LSM_sb_clone_mnt_opts,
> +	LSM_sb_parse_opts_str,
> +	LSM_dentry_init_security,
> +	LSM_dentry_create_files_as,
>  #ifdef CONFIG_SECURITY_PATH
> -	struct list_head path_unlink;
> -	struct list_head path_mkdir;
> -	struct list_head path_rmdir;
> -	struct list_head path_mknod;
> -	struct list_head path_truncate;
> -	struct list_head path_symlink;
> -	struct list_head path_link;
> -	struct list_head path_rename;
> -	struct list_head path_chmod;
> -	struct list_head path_chown;
> -	struct list_head path_chroot;
> +	LSM_path_unlink,
> +	LSM_path_mkdir,
> +	LSM_path_rmdir,
> +	LSM_path_mknod,
> +	LSM_path_truncate,
> +	LSM_path_symlink,
> +	LSM_path_link,
> +	LSM_path_rename,
> +	LSM_path_chmod,
> +	LSM_path_chown,
> +	LSM_path_chroot,
>  #endif
> -	struct list_head inode_alloc_security;
> -	struct list_head inode_free_security;
> -	struct list_head inode_init_security;
> -	struct list_head inode_create;
> -	struct list_head inode_link;
> -	struct list_head inode_unlink;
> -	struct list_head inode_symlink;
> -	struct list_head inode_mkdir;
> -	struct list_head inode_rmdir;
> -	struct list_head inode_mknod;
> -	struct list_head inode_rename;
> -	struct list_head inode_readlink;
> -	struct list_head inode_follow_link;
> -	struct list_head inode_permission;
> -	struct list_head inode_setattr;
> -	struct list_head inode_getattr;
> -	struct list_head inode_setxattr;
> -	struct list_head inode_post_setxattr;
> -	struct list_head inode_getxattr;
> -	struct list_head inode_listxattr;
> -	struct list_head inode_removexattr;
> -	struct list_head inode_need_killpriv;
> -	struct list_head inode_killpriv;
> -	struct list_head inode_getsecurity;
> -	struct list_head inode_setsecurity;
> -	struct list_head inode_listsecurity;
> -	struct list_head inode_getsecid;
> -	struct list_head inode_copy_up;
> -	struct list_head inode_copy_up_xattr;
> -	struct list_head file_permission;
> -	struct list_head file_alloc_security;
> -	struct list_head file_free_security;
> -	struct list_head file_ioctl;
> -	struct list_head mmap_addr;
> -	struct list_head mmap_file;
> -	struct list_head file_mprotect;
> -	struct list_head file_lock;
> -	struct list_head file_fcntl;
> -	struct list_head file_set_fowner;
> -	struct list_head file_send_sigiotask;
> -	struct list_head file_receive;
> -	struct list_head file_open;
> -	struct list_head task_create;
> -	struct list_head task_alloc;
> -	struct list_head task_free;
> -	struct list_head cred_alloc_blank;
> -	struct list_head cred_free;
> -	struct list_head cred_prepare;
> -	struct list_head cred_transfer;
> -	struct list_head kernel_act_as;
> -	struct list_head kernel_create_files_as;
> -	struct list_head kernel_read_file;
> -	struct list_head kernel_post_read_file;
> -	struct list_head kernel_module_request;
> -	struct list_head task_fix_setuid;
> -	struct list_head task_setpgid;
> -	struct list_head task_getpgid;
> -	struct list_head task_getsid;
> -	struct list_head task_getsecid;
> -	struct list_head task_setnice;
> -	struct list_head task_setioprio;
> -	struct list_head task_getioprio;
> -	struct list_head task_prlimit;
> -	struct list_head task_setrlimit;
> -	struct list_head task_setscheduler;
> -	struct list_head task_getscheduler;
> -	struct list_head task_movememory;
> -	struct list_head task_kill;
> -	struct list_head task_prctl;
> -	struct list_head task_to_inode;
> -	struct list_head ipc_permission;
> -	struct list_head ipc_getsecid;
> -	struct list_head msg_msg_alloc_security;
> -	struct list_head msg_msg_free_security;
> -	struct list_head msg_queue_alloc_security;
> -	struct list_head msg_queue_free_security;
> -	struct list_head msg_queue_associate;
> -	struct list_head msg_queue_msgctl;
> -	struct list_head msg_queue_msgsnd;
> -	struct list_head msg_queue_msgrcv;
> -	struct list_head shm_alloc_security;
> -	struct list_head shm_free_security;
> -	struct list_head shm_associate;
> -	struct list_head shm_shmctl;
> -	struct list_head shm_shmat;
> -	struct list_head sem_alloc_security;
> -	struct list_head sem_free_security;
> -	struct list_head sem_associate;
> -	struct list_head sem_semctl;
> -	struct list_head sem_semop;
> -	struct list_head netlink_send;
> -	struct list_head d_instantiate;
> -	struct list_head getprocattr;
> -	struct list_head setprocattr;
> -	struct list_head ismaclabel;
> -	struct list_head secid_to_secctx;
> -	struct list_head secctx_to_secid;
> -	struct list_head release_secctx;
> -	struct list_head inode_invalidate_secctx;
> -	struct list_head inode_notifysecctx;
> -	struct list_head inode_setsecctx;
> -	struct list_head inode_getsecctx;
> +	LSM_inode_alloc_security,
> +	LSM_inode_free_security,
> +	LSM_inode_init_security,
> +	LSM_inode_create,
> +	LSM_inode_link,
> +	LSM_inode_unlink,
> +	LSM_inode_symlink,
> +	LSM_inode_mkdir,
> +	LSM_inode_rmdir,
> +	LSM_inode_mknod,
> +	LSM_inode_rename,
> +	LSM_inode_readlink,
> +	LSM_inode_follow_link,
> +	LSM_inode_permission,
> +	LSM_inode_setattr,
> +	LSM_inode_getattr,
> +	LSM_inode_setxattr,
> +	LSM_inode_post_setxattr,
> +	LSM_inode_getxattr,
> +	LSM_inode_listxattr,
> +	LSM_inode_removexattr,
> +	LSM_inode_need_killpriv,
> +	LSM_inode_killpriv,
> +	LSM_inode_getsecurity,
> +	LSM_inode_setsecurity,
> +	LSM_inode_listsecurity,
> +	LSM_inode_getsecid,
> +	LSM_inode_copy_up,
> +	LSM_inode_copy_up_xattr,
> +	LSM_file_permission,
> +	LSM_file_alloc_security,
> +	LSM_file_free_security,
> +	LSM_file_ioctl,
> +	LSM_mmap_addr,
> +	LSM_mmap_file,
> +	LSM_file_mprotect,
> +	LSM_file_lock,
> +	LSM_file_fcntl,
> +	LSM_file_set_fowner,
> +	LSM_file_send_sigiotask,
> +	LSM_file_receive,
> +	LSM_file_open,
> +	LSM_task_create,
> +	LSM_task_alloc,
> +	LSM_task_free,
> +	LSM_cred_alloc_blank,
> +	LSM_cred_free,
> +	LSM_cred_prepare,
> +	LSM_cred_transfer,
> +	LSM_kernel_act_as,
> +	LSM_kernel_create_files_as,
> +	LSM_kernel_read_file,
> +	LSM_kernel_post_read_file,
> +	LSM_kernel_module_request,
> +	LSM_task_fix_setuid,
> +	LSM_task_setpgid,
> +	LSM_task_getpgid,
> +	LSM_task_getsid,
> +	LSM_task_getsecid,
> +	LSM_task_setnice,
> +	LSM_task_setioprio,
> +	LSM_task_getioprio,
> +	LSM_task_prlimit,
> +	LSM_task_setrlimit,
> +	LSM_task_setscheduler,
> +	LSM_task_getscheduler,
> +	LSM_task_movememory,
> +	LSM_task_kill,
> +	LSM_task_prctl,
> +	LSM_task_to_inode,
> +	LSM_ipc_permission,
> +	LSM_ipc_getsecid,
> +	LSM_msg_msg_alloc_security,
> +	LSM_msg_msg_free_security,
> +	LSM_msg_queue_alloc_security,
> +	LSM_msg_queue_free_security,
> +	LSM_msg_queue_associate,
> +	LSM_msg_queue_msgctl,
> +	LSM_msg_queue_msgsnd,
> +	LSM_msg_queue_msgrcv,
> +	LSM_shm_alloc_security,
> +	LSM_shm_free_security,
> +	LSM_shm_associate,
> +	LSM_shm_shmctl,
> +	LSM_shm_shmat,
> +	LSM_sem_alloc_security,
> +	LSM_sem_free_security,
> +	LSM_sem_associate,
> +	LSM_sem_semctl,
> +	LSM_sem_semop,
> +	LSM_netlink_send,
> +	LSM_d_instantiate,
> +	LSM_getprocattr,
> +	LSM_setprocattr,
> +	LSM_ismaclabel,
> +	LSM_secid_to_secctx,
> +	LSM_secctx_to_secid,
> +	LSM_release_secctx,
> +	LSM_inode_invalidate_secctx,
> +	LSM_inode_notifysecctx,
> +	LSM_inode_setsecctx,
> +	LSM_inode_getsecctx,
>  #ifdef CONFIG_SECURITY_NETWORK
> -	struct list_head unix_stream_connect;
> -	struct list_head unix_may_send;
> -	struct list_head socket_create;
> -	struct list_head socket_post_create;
> -	struct list_head socket_bind;
> -	struct list_head socket_connect;
> -	struct list_head socket_listen;
> -	struct list_head socket_accept;
> -	struct list_head socket_sendmsg;
> -	struct list_head socket_recvmsg;
> -	struct list_head socket_getsockname;
> -	struct list_head socket_getpeername;
> -	struct list_head socket_getsockopt;
> -	struct list_head socket_setsockopt;
> -	struct list_head socket_shutdown;
> -	struct list_head socket_sock_rcv_skb;
> -	struct list_head socket_getpeersec_stream;
> -	struct list_head socket_getpeersec_dgram;
> -	struct list_head sk_alloc_security;
> -	struct list_head sk_free_security;
> -	struct list_head sk_clone_security;
> -	struct list_head sk_getsecid;
> -	struct list_head sock_graft;
> -	struct list_head inet_conn_request;
> -	struct list_head inet_csk_clone;
> -	struct list_head inet_conn_established;
> -	struct list_head secmark_relabel_packet;
> -	struct list_head secmark_refcount_inc;
> -	struct list_head secmark_refcount_dec;
> -	struct list_head req_classify_flow;
> -	struct list_head tun_dev_alloc_security;
> -	struct list_head tun_dev_free_security;
> -	struct list_head tun_dev_create;
> -	struct list_head tun_dev_attach_queue;
> -	struct list_head tun_dev_attach;
> -	struct list_head tun_dev_open;
> +	LSM_unix_stream_connect,
> +	LSM_unix_may_send,
> +	LSM_socket_create,
> +	LSM_socket_post_create,
> +	LSM_socket_bind,
> +	LSM_socket_connect,
> +	LSM_socket_listen,
> +	LSM_socket_accept,
> +	LSM_socket_sendmsg,
> +	LSM_socket_recvmsg,
> +	LSM_socket_getsockname,
> +	LSM_socket_getpeername,
> +	LSM_socket_getsockopt,
> +	LSM_socket_setsockopt,
> +	LSM_socket_shutdown,
> +	LSM_socket_sock_rcv_skb,
> +	LSM_socket_getpeersec_stream,
> +	LSM_socket_getpeersec_dgram,
> +	LSM_sk_alloc_security,
> +	LSM_sk_free_security,
> +	LSM_sk_clone_security,
> +	LSM_sk_getsecid,
> +	LSM_sock_graft,
> +	LSM_inet_conn_request,
> +	LSM_inet_csk_clone,
> +	LSM_inet_conn_established,
> +	LSM_secmark_relabel_packet,
> +	LSM_secmark_refcount_inc,
> +	LSM_secmark_refcount_dec,
> +	LSM_req_classify_flow,
> +	LSM_tun_dev_alloc_security,
> +	LSM_tun_dev_free_security,
> +	LSM_tun_dev_create,
> +	LSM_tun_dev_attach_queue,
> +	LSM_tun_dev_attach,
> +	LSM_tun_dev_open,
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> -	struct list_head xfrm_policy_alloc_security;
> -	struct list_head xfrm_policy_clone_security;
> -	struct list_head xfrm_policy_free_security;
> -	struct list_head xfrm_policy_delete_security;
> -	struct list_head xfrm_state_alloc;
> -	struct list_head xfrm_state_alloc_acquire;
> -	struct list_head xfrm_state_free_security;
> -	struct list_head xfrm_state_delete_security;
> -	struct list_head xfrm_policy_lookup;
> -	struct list_head xfrm_state_pol_flow_match;
> -	struct list_head xfrm_decode_session;
> +	LSM_xfrm_policy_alloc_security,
> +	LSM_xfrm_policy_clone_security,
> +	LSM_xfrm_policy_free_security,
> +	LSM_xfrm_policy_delete_security,
> +	LSM_xfrm_state_alloc,
> +	LSM_xfrm_state_alloc_acquire,
> +	LSM_xfrm_state_free_security,
> +	LSM_xfrm_state_delete_security,
> +	LSM_xfrm_policy_lookup,
> +	LSM_xfrm_state_pol_flow_match,
> +	LSM_xfrm_decode_session,
>  #endif	/* CONFIG_SECURITY_NETWORK_XFRM */
>  #ifdef CONFIG_KEYS
> -	struct list_head key_alloc;
> -	struct list_head key_free;
> -	struct list_head key_permission;
> -	struct list_head key_getsecurity;
> +	LSM_key_alloc,
> +	LSM_key_free,
> +	LSM_key_permission,
> +	LSM_key_getsecurity,
>  #endif	/* CONFIG_KEYS */
>  #ifdef CONFIG_AUDIT
> -	struct list_head audit_rule_init;
> -	struct list_head audit_rule_known;
> -	struct list_head audit_rule_match;
> -	struct list_head audit_rule_free;
> +	LSM_audit_rule_init,
> +	LSM_audit_rule_known,
> +	LSM_audit_rule_match,
> +	LSM_audit_rule_free,
>  #endif /* CONFIG_AUDIT */
> +	LSM_max_security_hook_index
>  };
>  
>  /*
> @@ -1884,8 +1885,8 @@ struct security_hook_heads {
>   */
>  struct security_hook_list {
>  	struct list_head		list;
> -	struct list_head		*head;
>  	union security_list_options	hook;
> +	enum security_hook_index	idx;
>  	char				*lsm;
>  };
>  
> @@ -1896,9 +1897,8 @@ struct security_hook_list {
>   * text involved.
>   */
>  #define LSM_HOOK_INIT(HEAD, HOOK) \
> -	{ .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
> +	{ .idx = LSM_##HEAD, .hook = { .HEAD = HOOK } }
>  
> -extern struct security_hook_heads security_hook_heads;
>  extern char *lsm_names;
>  
>  extern void security_add_hooks(struct security_hook_list *hooks, int count,
> diff --git a/security/security.c b/security/security.c
> index 38316bb..bd3c07e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -33,7 +33,8 @@
>  /* Maximum number of letters for an LSM name string */
>  #define SECURITY_NAME_MAX	10
>  
> -struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> +static struct list_head security_hook_heads[LSM_max_security_hook_index]
> +	__lsm_ro_after_init;
>  char *lsm_names;
>  /* Boot-time LSM user choice */
>  static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
> @@ -56,12 +57,10 @@ static void __init do_security_initcalls(void)
>   */
>  int __init security_init(void)
>  {
> -	int i;
> -	struct list_head *list = (struct list_head *) &security_hook_heads;
> +	enum security_hook_index i;
>  
> -	for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct list_head);
> -	     i++)
> -		INIT_LIST_HEAD(&list[i]);
> +	for (i = 0; i < LSM_max_security_hook_index; i++)
> +		INIT_LIST_HEAD(&security_hook_heads[i]);
>  	pr_info("Security Framework initialized\n");
>  
>  	/*
> @@ -158,8 +157,12 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>  	int i;
>  
>  	for (i = 0; i < count; i++) {
> +		enum security_hook_index idx = hooks[i].idx;
> +
>  		hooks[i].lsm = lsm;
> -		list_add_tail_rcu(&hooks[i].list, hooks[i].head);
> +		/* Can't hit this BUG_ON() unless LSM_HOOK_INIT() is broken. */
> +		BUG_ON(idx < 0 || idx >= LSM_max_security_hook_index);
> +		list_add_tail_rcu(&hooks[i].list, &security_hook_heads[idx]);
>  	}
>  	if (lsm_append(lsm, &lsm_names) < 0)
>  		panic("%s - Cannot get early memory.\n", __func__);
> @@ -179,7 +182,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>  	do {							\
>  		struct security_hook_list *P;			\
>  								\
> -		list_for_each_entry(P, &security_hook_heads.FUNC, list)	\
> +		list_for_each_entry(P, &security_hook_heads	\
> +				    [LSM_##FUNC], list)		\
>  			P->hook.FUNC(__VA_ARGS__);		\
>  	} while (0)
>  
> @@ -188,7 +192,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>  	do {							\
>  		struct security_hook_list *P;			\
>  								\
> -		list_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> +		list_for_each_entry(P, &security_hook_heads     \
> +				    [LSM_##FUNC], list) {	\
>  			RC = P->hook.FUNC(__VA_ARGS__);		\
>  			if (RC != 0)				\
>  				break;				\
> @@ -295,7 +300,8 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
>  	 * agree that it should be set it will. If any module
>  	 * thinks it should not be set it won't.
>  	 */
> -	list_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
> +	list_for_each_entry(hp, &security_hook_heads[LSM_vm_enough_memory],
> +			    list) {
>  		rc = hp->hook.vm_enough_memory(mm, pages);
>  		if (rc <= 0) {
>  			cap_sys_admin = 0;
> @@ -785,7 +791,8 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
>  	/*
>  	 * Only one module will provide an attribute with a given name.
>  	 */
> -	list_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
> +	list_for_each_entry(hp, &security_hook_heads[LSM_inode_getsecurity],
> +			    list) {
>  		rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
>  		if (rc != -EOPNOTSUPP)
>  			return rc;
> @@ -803,7 +810,8 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
>  	/*
>  	 * Only one module will provide an attribute with a given name.
>  	 */
> -	list_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
> +	list_for_each_entry(hp, &security_hook_heads[LSM_inode_setsecurity],
> +			    list) {
>  		rc = hp->hook.inode_setsecurity(inode, name, value, size,
>  								flags);
>  		if (rc != -EOPNOTSUPP)
> @@ -1111,7 +1119,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
>  	int rc = -ENOSYS;
>  	struct security_hook_list *hp;
>  
> -	list_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
> +	list_for_each_entry(hp, &security_hook_heads[LSM_task_prctl], list) {
>  		thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
>  		if (thisrc != -ENOSYS) {
>  			rc = thisrc;
> @@ -1587,8 +1595,8 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
>  	 * For speed optimization, we explicitly break the loop rather than
>  	 * using the macro
>  	 */
> -	list_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
> -				list) {
> +	list_for_each_entry(hp, &security_hook_heads
> +			    [LSM_xfrm_state_pol_flow_match], list) {
>  		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
>  		break;
>  	}

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tetsuo Handa May 28, 2017, 12:38 a.m. UTC | #2
Casey Schaufler wrote:
> > But currently, LSM_HOOK_INIT() macro depends on the address of
> > security_hook_heads being known at compile time. If we use an enum
> > so that LSM_HOOK_INIT() macro does not need to know absolute address of
> > security_hook_heads, it will help us to use that allocator for LSM hooks.
> >
> > As a result of introducing an enum, security_hook_heads becomes a local
> > variable, making it easier to allocate security_hook_heads at run time.
> 
> You loose the type checking in security.c. This is the same
> objection I had before to this approach. It's why I objected
> to 3dfc9b02864b19f4 and why I didn't adopt the array approach
> in the first place. If it's so important that randstruct not
> complain about the unnatural cast, revert the patch that
> introduced it. I see no net benefit in hiding the symbol over
> loosing the typing. You trade a list of typed function
> pointers for an enumerated list of values. It doesn't even
> make the code look smaller!

I still cannot understand what you are referring by "type checking".
Please explain me what the type checking in security/security.c is.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook May 28, 2017, 1:04 a.m. UTC | #3
On Sat, May 27, 2017 at 4:17 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Commit 3dfc9b02864b19f4 ("LSM: Initialize security_hook_heads upon
> registration.") treats "struct security_hook_heads" as an implicit array
> of "struct list_head" so that we can eliminate code for static
> initialization. Although we haven't encountered compilers which do not
> treat sizeof(security_hook_heads) != sizeof(struct list_head) *
> (sizeof(security_hook_heads) / sizeof(struct list_head)), Casey does not
> like the assumption that a structure of N elements can be assumed to be
> the same as an array of N elements.
>
> Now that Kees found that randstruct complains such casting
>
>   security/security.c: In function 'security_init':
>   security/security.c:59:20: note: found mismatched op0 struct pointer types: 'struct list_head' and 'struct security_hook_heads'
>
>     struct list_head *list = (struct list_head *) &security_hook_heads;
>
> and Christoph thinks that we should fix it rather than make randstruct
> whitelist it, this patch fixes it.
>
> It would be possible to revert commit 3dfc9b02864b19f4, but this patch
> converts security_hook_heads into an explicit array of struct list_head
> by introducing an enum, due to reasons explained below.

Like Casey, I had confused this patch with the other(?) that resulted
in dropped type checking. This just switches from named list_heads to
indexed list_heads, which is fine now that the BUG_ON exists to
sanity-check the index being used.

> In MM subsystem, a sealable memory allocator patch was proposed, and
> the LSM hooks ("struct security_hook_heads security_hook_heads" and
> "struct security_hook_list ...[]") will benefit from this allocator via
> protection using set_memory_ro()/set_memory_rw(), and that allocator
> will remove CONFIG_SECURITY_WRITABLE_HOOKS config option. Thus, we will
> likely be moving to that direction.

It's unlikely that smalloc will allow unsealing after initialization,
so the SELinux disabling case will remain, IIUC.

> This means that these structures will be allocated at run time using
> this allocator, and therefore the address of these structures will be
> determined at run time rather than compile time.
>
> But currently, LSM_HOOK_INIT() macro depends on the address of
> security_hook_heads being known at compile time. If we use an enum
> so that LSM_HOOK_INIT() macro does not need to know absolute address of
> security_hook_heads, it will help us to use that allocator for LSM hooks.
>
> As a result of introducing an enum, security_hook_heads becomes a local
> variable, making it easier to allocate security_hook_heads at run time.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Igor Stoppa <igor.stoppa@huawei.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> ---
>  include/linux/lsm_hooks.h | 412 +++++++++++++++++++++++-----------------------
>  security/security.c       |  38 +++--
>  2 files changed, 229 insertions(+), 221 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 38316bb..bd3c07e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -179,7 +182,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>         do {                                                    \
>                 struct security_hook_list *P;                   \
>                                                                 \
> -               list_for_each_entry(P, &security_hook_heads.FUNC, list) \
> +               list_for_each_entry(P, &security_hook_heads     \
> +                                   [LSM_##FUNC], list)         \

Can this be unsplit so the [...] remains next to security_hook_heads?

>                         P->hook.FUNC(__VA_ARGS__);              \
>         } while (0)
>
> @@ -188,7 +192,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>         do {                                                    \
>                 struct security_hook_list *P;                   \
>                                                                 \
> -               list_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> +               list_for_each_entry(P, &security_hook_heads     \
> +                                   [LSM_##FUNC], list) {       \

Same

>                         RC = P->hook.FUNC(__VA_ARGS__);         \
>                         if (RC != 0)                            \
>                                 break;                          \
> @@ -1587,8 +1595,8 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
>          * For speed optimization, we explicitly break the loop rather than
>          * using the macro
>          */
> -       list_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
> -                               list) {
> +       list_for_each_entry(hp, &security_hook_heads
> +                           [LSM_xfrm_state_pol_flow_match], list) {

Same

>                 rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
>                 break;
>         }
> --
> 1.8.3.1
>

Otherwise, yeah, I can be convinced to take this. :) Thanks for
persisting with this, I think it makes sense now.

-Kees
Tetsuo Handa May 28, 2017, 1:26 a.m. UTC | #4
Kees Cook wrote:
> On Sat, May 27, 2017 at 4:17 AM, Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > Commit 3dfc9b02864b19f4 ("LSM: Initialize security_hook_heads upon
> > registration.") treats "struct security_hook_heads" as an implicit array
> > of "struct list_head" so that we can eliminate code for static
> > initialization. Although we haven't encountered compilers which do not
> > treat sizeof(security_hook_heads) != sizeof(struct list_head) *
> > (sizeof(security_hook_heads) / sizeof(struct list_head)), Casey does not
> > like the assumption that a structure of N elements can be assumed to be
> > the same as an array of N elements.
> >
> > Now that Kees found that randstruct complains such casting
> >
> >   security/security.c: In function 'security_init':
> >   security/security.c:59:20: note: found mismatched op0 struct pointer types: 'struct list_head' and 'struct security_hook_heads'
> >
> >     struct list_head *list = (struct list_head *) &security_hook_heads;
> >
> > and Christoph thinks that we should fix it rather than make randstruct
> > whitelist it, this patch fixes it.
> >
> > It would be possible to revert commit 3dfc9b02864b19f4, but this patch
> > converts security_hook_heads into an explicit array of struct list_head
> > by introducing an enum, due to reasons explained below.
> 
> Like Casey, I had confused this patch with the other(?) that resulted
> in dropped type checking. This just switches from named list_heads to
> indexed list_heads, which is fine now that the BUG_ON exists to
> sanity-check the index being used.

Casey, are you just confused as well?

> 
> > In MM subsystem, a sealable memory allocator patch was proposed, and
> > the LSM hooks ("struct security_hook_heads security_hook_heads" and
> > "struct security_hook_list ...[]") will benefit from this allocator via
> > protection using set_memory_ro()/set_memory_rw(), and that allocator
> > will remove CONFIG_SECURITY_WRITABLE_HOOKS config option. Thus, we will
> > likely be moving to that direction.
> 
> It's unlikely that smalloc will allow unsealing after initialization,
> so the SELinux disabling case will remain, IIUC.

LKM-based LSM modules will need it. Look at the result of a recent poll at
https://distrowatch.com/weekly.php?pollnumber=102&myaction=SeeVote&issue=20170522#poll .
We are still failing to provide users "a security module that individual user
can afford enabling". And we know that we cannot merge all security modules
into mainline. Thus, allowing LKM-based LSM modules is inevitable.

> > @@ -179,7 +182,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> >         do {                                                    \
> >                 struct security_hook_list *P;                   \
> >                                                                 \
> > -               list_for_each_entry(P, &security_hook_heads.FUNC, list) \
> > +               list_for_each_entry(P, &security_hook_heads     \
> > +                                   [LSM_##FUNC], list)         \
> 
> Can this be unsplit so the [...] remains next to security_hook_heads?

These are needed for passing 80 columns check by scripts/checkpatch.pl .
Should we ignore that warning or rename security_hook_heads to e.g. SHH ?

> Otherwise, yeah, I can be convinced to take this. :) Thanks for
> persisting with this, I think it makes sense now.

Thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Casey Schaufler May 28, 2017, 5:57 p.m. UTC | #5
On 5/27/2017 6:26 PM, Tetsuo Handa wrote:
> Kees Cook wrote:
>> On Sat, May 27, 2017 at 4:17 AM, Tetsuo Handa
>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>> Commit 3dfc9b02864b19f4 ("LSM: Initialize security_hook_heads upon
>>> registration.") treats "struct security_hook_heads" as an implicit array
>>> of "struct list_head" so that we can eliminate code for static
>>> initialization. Although we haven't encountered compilers which do not
>>> treat sizeof(security_hook_heads) != sizeof(struct list_head) *
>>> (sizeof(security_hook_heads) / sizeof(struct list_head)), Casey does not
>>> like the assumption that a structure of N elements can be assumed to be
>>> the same as an array of N elements.
>>>
>>> Now that Kees found that randstruct complains such casting
>>>
>>>   security/security.c: In function 'security_init':
>>>   security/security.c:59:20: note: found mismatched op0 struct pointer types: 'struct list_head' and 'struct security_hook_heads'
>>>
>>>     struct list_head *list = (struct list_head *) &security_hook_heads;
>>>
>>> and Christoph thinks that we should fix it rather than make randstruct
>>> whitelist it, this patch fixes it.
>>>
>>> It would be possible to revert commit 3dfc9b02864b19f4, but this patch
>>> converts security_hook_heads into an explicit array of struct list_head
>>> by introducing an enum, due to reasons explained below.
>> Like Casey, I had confused this patch with the other(?) that resulted
>> in dropped type checking. This just switches from named list_heads to
>> indexed list_heads, which is fine now that the BUG_ON exists to
>> sanity-check the index being used.
> Casey, are you just confused as well?

I am indeed "just confused". I still don't like it, I liked
it the way I had it, but I don't see it worth fighting over.


>>> In MM subsystem, a sealable memory allocator patch was proposed, and
>>> the LSM hooks ("struct security_hook_heads security_hook_heads" and
>>> "struct security_hook_list ...[]") will benefit from this allocator via
>>> protection using set_memory_ro()/set_memory_rw(), and that allocator
>>> will remove CONFIG_SECURITY_WRITABLE_HOOKS config option. Thus, we will
>>> likely be moving to that direction.
>> It's unlikely that smalloc will allow unsealing after initialization,
>> so the SELinux disabling case will remain, IIUC.
> LKM-based LSM modules will need it. Look at the result of a recent poll at
> https://distrowatch.com/weekly.php?pollnumber=102&myaction=SeeVote&issue=20170522#poll .
> We are still failing to provide users "a security module that individual user
> can afford enabling". And we know that we cannot merge all security modules
> into mainline. Thus, allowing LKM-based LSM modules is inevitable.
>
>>> @@ -179,7 +182,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>>         do {                                                    \
>>>                 struct security_hook_list *P;                   \
>>>                                                                 \
>>> -               list_for_each_entry(P, &security_hook_heads.FUNC, list) \
>>> +               list_for_each_entry(P, &security_hook_heads     \
>>> +                                   [LSM_##FUNC], list)         \
>> Can this be unsplit so the [...] remains next to security_hook_heads?
> These are needed for passing 80 columns check by scripts/checkpatch.pl .
> Should we ignore that warning or rename security_hook_heads to e.g. SHH ?

No! I spend way too much of my life battling with checkpatch.pl.
OK, you could rename it since it's static. hook_heads gets my vote.

>
>> Otherwise, yeah, I can be convinced to take this. :) Thanks for
>> persisting with this, I think it makes sense now.
> Thank you.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Morris May 30, 2017, 10:22 a.m. UTC | #6
On Sun, 28 May 2017, Tetsuo Handa wrote:

> can afford enabling". And we know that we cannot merge all security modules
> into mainline. Thus, allowing LKM-based LSM modules is inevitable.

Nope, it's not inevitable.  The LSM API only caters to in-tree users.

I'm not sure why you persist against this.
Tetsuo Handa May 30, 2017, 2:29 p.m. UTC | #7
James Morris wrote:
> On Sun, 28 May 2017, Tetsuo Handa wrote:
> 
> > can afford enabling". And we know that we cannot merge all security modules
> > into mainline. Thus, allowing LKM-based LSM modules is inevitable.
> 
> Nope, it's not inevitable.  The LSM API only caters to in-tree users.
> 
> I'm not sure why you persist against this.

Then, we are willing to accept LSM modules with users less than 10, aren't we?
Forcing users to patch and recompile is as heartless as forcing CONFIG_MODULES=n.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox May 30, 2017, 3:25 p.m. UTC | #8
On Tue, 30 May 2017 23:29:10 +0900
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> James Morris wrote:
> > On Sun, 28 May 2017, Tetsuo Handa wrote:
> >   
> > > can afford enabling". And we know that we cannot merge all security modules
> > > into mainline. Thus, allowing LKM-based LSM modules is inevitable.  
> > 
> > Nope, it's not inevitable.  The LSM API only caters to in-tree users.
> > 
> > I'm not sure why you persist against this.  
> 
> Then, we are willing to accept LSM modules with users less than 10, aren't we?

Why not if they are properly written and maintained. Historically we've
supported an entire architecture that had one machine ever built. We
supported a strange subclass of x86 machines for many years because James
Bottomley cared enough to do the work. We still support M68K, PA-RISC and
other stuff as well as plenty of hardware which probably has few users -
providing it doesn't cause maintenance problems.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Morris May 30, 2017, 11:06 p.m. UTC | #9
On Tue, 30 May 2017, Alan Cox wrote:

> On Tue, 30 May 2017 23:29:10 +0900
> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
> > James Morris wrote:
> > > On Sun, 28 May 2017, Tetsuo Handa wrote:
> > >   
> > > > can afford enabling". And we know that we cannot merge all security modules
> > > > into mainline. Thus, allowing LKM-based LSM modules is inevitable.  
> > > 
> > > Nope, it's not inevitable.  The LSM API only caters to in-tree users.
> > > 
> > > I'm not sure why you persist against this.  
> > 
> > Then, we are willing to accept LSM modules with users less than 10, aren't we?
> 
> Why not if they are properly written and maintained. Historically we've
> supported an entire architecture that had one machine ever built. We
> supported a strange subclass of x86 machines for many years because James
> Bottomley cared enough to do the work. We still support M68K, PA-RISC and
> other stuff as well as plenty of hardware which probably has few users -
> providing it doesn't cause maintenance problems.

This is what we as a community came up with in 2008:

  "Essentially, any new security project—not limited to LSMs—should be 
   accompanied by a clear and concise document outlining its requirements 
   and expected uses. This is to allow both security and regular folk to 
   perform review of the code in terms of how it meets the specified 
   requirements, and to avoid getting bogged down in unresolvable 
   discussions about the project’s security model."

From https://blog.namei.org/2008/12/11/the-arjan-protocol/

(Not sure if the original document at Kernel Trap still exists, alas).

So what we need is clear design documentation that the code can be 
reviewed against.  There is nothing about the number of users.  If the 
code is simply using the existing API and meets its design goals, it's 
pretty straightforward.  If changes to the API or core kernel are also 
required, then more discussion and review will be needed.




- James
José Bollo May 31, 2017, 9:44 a.m. UTC | #10
On Tue, 30 May 2017 23:29:10 +0900
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> James Morris wrote:
> > On Sun, 28 May 2017, Tetsuo Handa wrote:
> >   
> > > can afford enabling". And we know that we cannot merge all
> > > security modules into mainline. Thus, allowing LKM-based LSM
> > > modules is inevitable.  
> > 
> > Nope, it's not inevitable.  The LSM API only caters to in-tree
> > users.
> > 
> > I'm not sure why you persist against this.  
> 
> Then, we are willing to accept LSM modules with users less than 10,
> aren't we? Forcing users to patch and recompile is as heartless as
> forcing CONFIG_MODULES=n.

These are good reasons. I'm in favor of Tetsuo.

Regards
José
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tetsuo Handa May 31, 2017, 10:41 a.m. UTC | #11
James Morris wrote:
> On Tue, 30 May 2017, Alan Cox wrote:
> 
> > On Tue, 30 May 2017 23:29:10 +0900
> > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > 
> > > James Morris wrote:
> > > > On Sun, 28 May 2017, Tetsuo Handa wrote:
> > > >   
> > > > > can afford enabling". And we know that we cannot merge all security modules
> > > > > into mainline. Thus, allowing LKM-based LSM modules is inevitable.  
> > > > 
> > > > Nope, it's not inevitable.  The LSM API only caters to in-tree users.
> > > > 
> > > > I'm not sure why you persist against this.  
> > > 
> > > Then, we are willing to accept LSM modules with users less than 10, aren't we?
> > 
> > Why not if they are properly written and maintained. Historically we've
> > supported an entire architecture that had one machine ever built. We
> > supported a strange subclass of x86 machines for many years because James
> > Bottomley cared enough to do the work. We still support M68K, PA-RISC and
> > other stuff as well as plenty of hardware which probably has few users -
> > providing it doesn't cause maintenance problems.
> 
> This is what we as a community came up with in 2008:
> 
>   "Essentially, any new security project - not limited to LSMs - should be 
>    accompanied by a clear and concise document outlining its requirements 
>    and expected uses. This is to allow both security and regular folk to 
>    perform review of the code in terms of how it meets the specified 
>    requirements, and to avoid getting bogged down in unresolvable 
>    discussions about the project’s security model."
> 
> From https://blog.namei.org/2008/12/11/the-arjan-protocol/
> 
> (Not sure if the original document at Kernel Trap still exists, alas).
> 
> So what we need is clear design documentation that the code can be 
> reviewed against.  There is nothing about the number of users.  If the 
> code is simply using the existing API and meets its design goals, it's 
> pretty straightforward.  If changes to the API or core kernel are also 
> required, then more discussion and review will be needed.

I saw several companies who ship their embedded devices with
single-function LSM modules (e.g. restrict only mount operation and
ptrace operation). What is unfortunate is that their LSM modules had
never been proposed for upstream, and thus bugs remained unnoticed.

They cannot afford enabling SELinux or SMACK or TOMOYO or AppArmor
in their products, whereas LSM community tends to think as whether
SELinux or SMACK or TOMOYO or AppArmor can do what they want.

It will become possible to get single-function LSM modules merged if we
stop saying "you can do it with SELinux or SMACK or TOMOYO or AppArmor".
But there is the other barrier if we think about distributions.

If distributors enable all LSM modules which are wanted by their users
(e.g. Ubuntu, or LSM modules dedicated for their embedded devices), there
is no need to support LKM-based LSM modules.

But otherwise (e.g. https://bugzilla.redhat.com/show_bug.cgi?id=542986 ),
we are forcing users to rebuild distribution kernels even if the LSM code is
available in vanilla kernels. It is a sort of enforcing CONFIG_MODULES=n.
Offering users the ability to use LSM modules as LKM and allowing users to
use LSM modules which are not enabled by distributors as LKM at user's own
risk is a realistic choice, and therefore I developed AKARI.

The location we are directing to via read-only LSM hooks seems to be: let's
accept source code of LSM modules which they want (so that LKM-based LSM
modules won't be needed), but let's prevent users from running binary code
of LSM modules which they want (by forcing users to rebuild their kernels
via lack of ability to use LKM-based LSM modules). My customers cannot afford
enabling SELinux, but my customers cannot rebuild their kernels because
rebuilding makes it even more difficult to get help from support centers.
Therefore, my customers remain unable to use LSM modules which they want.
This is really unfortunate for me.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Morris May 31, 2017, 11:04 a.m. UTC | #12
On Wed, 31 May 2017, Tetsuo Handa wrote:

> via lack of ability to use LKM-based LSM modules). My customers cannot afford
> enabling SELinux, but my customers cannot rebuild their kernels because
> rebuilding makes it even more difficult to get help from support centers.
> Therefore, my customers remain unable to use LSM modules which they want.
> This is really unfortunate for me.

And they'll be able to get vendor support when they have their own custom 
LSMs installed?
Tetsuo Handa May 31, 2017, 11:31 a.m. UTC | #13
James Morris wrote:
> On Wed, 31 May 2017, Tetsuo Handa wrote:
> 
> > via lack of ability to use LKM-based LSM modules). My customers cannot afford
> > enabling SELinux, but my customers cannot rebuild their kernels because
> > rebuilding makes it even more difficult to get help from support centers.
> > Therefore, my customers remain unable to use LSM modules which they want.
> > This is really unfortunate for me.
> 
> And they'll be able to get vendor support when they have their own custom 
> LSMs installed?  

As long as customers are using the vmlinux provided by that distributor, they
can get distributor's support regarding problems which are not caused by use of
their own custom LKM-based LSMs. For example, distributors do not unconditionally
reject due to use of storage driver kernel module provided by hardware venders
(or, their servers won't boot) and/or on-access scanner kernel module provided by
antivirus venders. Customers won't be able to get distributor's support regarding
problems caused by use of storage driver / on-access scanner kernel modules not
provided by distributors. But rebuilding the vmlinux in order to use LSM modules
not enabled by distributors makes customer's situation very worse.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox May 31, 2017, 2:43 p.m. UTC | #14
> I saw several companies who ship their embedded devices with
> single-function LSM modules (e.g. restrict only mount operation and
> ptrace operation). What is unfortunate is that their LSM modules had
> never been proposed for upstream, and thus bugs remained unnoticed.

So which of them cannot be done with seccomp ? We have a small tight
interface for simple things like restricting a few calls.
 
> via lack of ability to use LKM-based LSM modules). My customers cannot afford
> enabling SELinux, but my customers cannot rebuild their kernels because
> rebuilding makes it even more difficult to get help from support centers.

And "I've loaded this third party module" doesn't ?

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tetsuo Handa May 31, 2017, 3:10 p.m. UTC | #15
Alan Cox wrote:
> > I saw several companies who ship their embedded devices with
> > single-function LSM modules (e.g. restrict only mount operation and
> > ptrace operation). What is unfortunate is that their LSM modules had
> > never been proposed for upstream, and thus bugs remained unnoticed.
>
> So which of them cannot be done with seccomp ? We have a small tight
> interface for simple things like restricting a few calls.

They restricted based on hard-coded rules. seccomp is too much for their cases.

> 
> > via lack of ability to use LKM-based LSM modules). My customers cannot afford
> > enabling SELinux, but my customers cannot rebuild their kernels because
> > rebuilding makes it even more difficult to get help from support centers.
>
> And "I've loaded this third party module" doesn't ?

Situation is far much better than "I've recompiled this vmlinux". ;-)
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox May 31, 2017, 3:14 p.m. UTC | #16
On Thu, 1 Jun 2017 00:10:07 +0900
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> Alan Cox wrote:
> > > I saw several companies who ship their embedded devices with
> > > single-function LSM modules (e.g. restrict only mount operation and
> > > ptrace operation). What is unfortunate is that their LSM modules had
> > > never been proposed for upstream, and thus bugs remained unnoticed.  
> >
> > So which of them cannot be done with seccomp ? We have a small tight
> > interface for simple things like restricting a few calls.  
> 
> They restricted based on hard-coded rules. seccomp is too much for their cases.

Seccomp is tiny. They may not know how to use it but the job of the
kernel is to provide generic interfaces. Seccomp seems to do that just
fine for simple stuff.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 080f34e..1f98d1b 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1663,219 +1663,220 @@ 
 #endif /* CONFIG_AUDIT */
 };
 
-struct security_hook_heads {
-	struct list_head binder_set_context_mgr;
-	struct list_head binder_transaction;
-	struct list_head binder_transfer_binder;
-	struct list_head binder_transfer_file;
-	struct list_head ptrace_access_check;
-	struct list_head ptrace_traceme;
-	struct list_head capget;
-	struct list_head capset;
-	struct list_head capable;
-	struct list_head quotactl;
-	struct list_head quota_on;
-	struct list_head syslog;
-	struct list_head settime;
-	struct list_head vm_enough_memory;
-	struct list_head bprm_set_creds;
-	struct list_head bprm_check_security;
-	struct list_head bprm_secureexec;
-	struct list_head bprm_committing_creds;
-	struct list_head bprm_committed_creds;
-	struct list_head sb_alloc_security;
-	struct list_head sb_free_security;
-	struct list_head sb_copy_data;
-	struct list_head sb_remount;
-	struct list_head sb_kern_mount;
-	struct list_head sb_show_options;
-	struct list_head sb_statfs;
-	struct list_head sb_mount;
-	struct list_head sb_umount;
-	struct list_head sb_pivotroot;
-	struct list_head sb_set_mnt_opts;
-	struct list_head sb_clone_mnt_opts;
-	struct list_head sb_parse_opts_str;
-	struct list_head dentry_init_security;
-	struct list_head dentry_create_files_as;
+enum security_hook_index {
+	LSM_binder_set_context_mgr,
+	LSM_binder_transaction,
+	LSM_binder_transfer_binder,
+	LSM_binder_transfer_file,
+	LSM_ptrace_access_check,
+	LSM_ptrace_traceme,
+	LSM_capget,
+	LSM_capset,
+	LSM_capable,
+	LSM_quotactl,
+	LSM_quota_on,
+	LSM_syslog,
+	LSM_settime,
+	LSM_vm_enough_memory,
+	LSM_bprm_set_creds,
+	LSM_bprm_check_security,
+	LSM_bprm_secureexec,
+	LSM_bprm_committing_creds,
+	LSM_bprm_committed_creds,
+	LSM_sb_alloc_security,
+	LSM_sb_free_security,
+	LSM_sb_copy_data,
+	LSM_sb_remount,
+	LSM_sb_kern_mount,
+	LSM_sb_show_options,
+	LSM_sb_statfs,
+	LSM_sb_mount,
+	LSM_sb_umount,
+	LSM_sb_pivotroot,
+	LSM_sb_set_mnt_opts,
+	LSM_sb_clone_mnt_opts,
+	LSM_sb_parse_opts_str,
+	LSM_dentry_init_security,
+	LSM_dentry_create_files_as,
 #ifdef CONFIG_SECURITY_PATH
-	struct list_head path_unlink;
-	struct list_head path_mkdir;
-	struct list_head path_rmdir;
-	struct list_head path_mknod;
-	struct list_head path_truncate;
-	struct list_head path_symlink;
-	struct list_head path_link;
-	struct list_head path_rename;
-	struct list_head path_chmod;
-	struct list_head path_chown;
-	struct list_head path_chroot;
+	LSM_path_unlink,
+	LSM_path_mkdir,
+	LSM_path_rmdir,
+	LSM_path_mknod,
+	LSM_path_truncate,
+	LSM_path_symlink,
+	LSM_path_link,
+	LSM_path_rename,
+	LSM_path_chmod,
+	LSM_path_chown,
+	LSM_path_chroot,
 #endif
-	struct list_head inode_alloc_security;
-	struct list_head inode_free_security;
-	struct list_head inode_init_security;
-	struct list_head inode_create;
-	struct list_head inode_link;
-	struct list_head inode_unlink;
-	struct list_head inode_symlink;
-	struct list_head inode_mkdir;
-	struct list_head inode_rmdir;
-	struct list_head inode_mknod;
-	struct list_head inode_rename;
-	struct list_head inode_readlink;
-	struct list_head inode_follow_link;
-	struct list_head inode_permission;
-	struct list_head inode_setattr;
-	struct list_head inode_getattr;
-	struct list_head inode_setxattr;
-	struct list_head inode_post_setxattr;
-	struct list_head inode_getxattr;
-	struct list_head inode_listxattr;
-	struct list_head inode_removexattr;
-	struct list_head inode_need_killpriv;
-	struct list_head inode_killpriv;
-	struct list_head inode_getsecurity;
-	struct list_head inode_setsecurity;
-	struct list_head inode_listsecurity;
-	struct list_head inode_getsecid;
-	struct list_head inode_copy_up;
-	struct list_head inode_copy_up_xattr;
-	struct list_head file_permission;
-	struct list_head file_alloc_security;
-	struct list_head file_free_security;
-	struct list_head file_ioctl;
-	struct list_head mmap_addr;
-	struct list_head mmap_file;
-	struct list_head file_mprotect;
-	struct list_head file_lock;
-	struct list_head file_fcntl;
-	struct list_head file_set_fowner;
-	struct list_head file_send_sigiotask;
-	struct list_head file_receive;
-	struct list_head file_open;
-	struct list_head task_create;
-	struct list_head task_alloc;
-	struct list_head task_free;
-	struct list_head cred_alloc_blank;
-	struct list_head cred_free;
-	struct list_head cred_prepare;
-	struct list_head cred_transfer;
-	struct list_head kernel_act_as;
-	struct list_head kernel_create_files_as;
-	struct list_head kernel_read_file;
-	struct list_head kernel_post_read_file;
-	struct list_head kernel_module_request;
-	struct list_head task_fix_setuid;
-	struct list_head task_setpgid;
-	struct list_head task_getpgid;
-	struct list_head task_getsid;
-	struct list_head task_getsecid;
-	struct list_head task_setnice;
-	struct list_head task_setioprio;
-	struct list_head task_getioprio;
-	struct list_head task_prlimit;
-	struct list_head task_setrlimit;
-	struct list_head task_setscheduler;
-	struct list_head task_getscheduler;
-	struct list_head task_movememory;
-	struct list_head task_kill;
-	struct list_head task_prctl;
-	struct list_head task_to_inode;
-	struct list_head ipc_permission;
-	struct list_head ipc_getsecid;
-	struct list_head msg_msg_alloc_security;
-	struct list_head msg_msg_free_security;
-	struct list_head msg_queue_alloc_security;
-	struct list_head msg_queue_free_security;
-	struct list_head msg_queue_associate;
-	struct list_head msg_queue_msgctl;
-	struct list_head msg_queue_msgsnd;
-	struct list_head msg_queue_msgrcv;
-	struct list_head shm_alloc_security;
-	struct list_head shm_free_security;
-	struct list_head shm_associate;
-	struct list_head shm_shmctl;
-	struct list_head shm_shmat;
-	struct list_head sem_alloc_security;
-	struct list_head sem_free_security;
-	struct list_head sem_associate;
-	struct list_head sem_semctl;
-	struct list_head sem_semop;
-	struct list_head netlink_send;
-	struct list_head d_instantiate;
-	struct list_head getprocattr;
-	struct list_head setprocattr;
-	struct list_head ismaclabel;
-	struct list_head secid_to_secctx;
-	struct list_head secctx_to_secid;
-	struct list_head release_secctx;
-	struct list_head inode_invalidate_secctx;
-	struct list_head inode_notifysecctx;
-	struct list_head inode_setsecctx;
-	struct list_head inode_getsecctx;
+	LSM_inode_alloc_security,
+	LSM_inode_free_security,
+	LSM_inode_init_security,
+	LSM_inode_create,
+	LSM_inode_link,
+	LSM_inode_unlink,
+	LSM_inode_symlink,
+	LSM_inode_mkdir,
+	LSM_inode_rmdir,
+	LSM_inode_mknod,
+	LSM_inode_rename,
+	LSM_inode_readlink,
+	LSM_inode_follow_link,
+	LSM_inode_permission,
+	LSM_inode_setattr,
+	LSM_inode_getattr,
+	LSM_inode_setxattr,
+	LSM_inode_post_setxattr,
+	LSM_inode_getxattr,
+	LSM_inode_listxattr,
+	LSM_inode_removexattr,
+	LSM_inode_need_killpriv,
+	LSM_inode_killpriv,
+	LSM_inode_getsecurity,
+	LSM_inode_setsecurity,
+	LSM_inode_listsecurity,
+	LSM_inode_getsecid,
+	LSM_inode_copy_up,
+	LSM_inode_copy_up_xattr,
+	LSM_file_permission,
+	LSM_file_alloc_security,
+	LSM_file_free_security,
+	LSM_file_ioctl,
+	LSM_mmap_addr,
+	LSM_mmap_file,
+	LSM_file_mprotect,
+	LSM_file_lock,
+	LSM_file_fcntl,
+	LSM_file_set_fowner,
+	LSM_file_send_sigiotask,
+	LSM_file_receive,
+	LSM_file_open,
+	LSM_task_create,
+	LSM_task_alloc,
+	LSM_task_free,
+	LSM_cred_alloc_blank,
+	LSM_cred_free,
+	LSM_cred_prepare,
+	LSM_cred_transfer,
+	LSM_kernel_act_as,
+	LSM_kernel_create_files_as,
+	LSM_kernel_read_file,
+	LSM_kernel_post_read_file,
+	LSM_kernel_module_request,
+	LSM_task_fix_setuid,
+	LSM_task_setpgid,
+	LSM_task_getpgid,
+	LSM_task_getsid,
+	LSM_task_getsecid,
+	LSM_task_setnice,
+	LSM_task_setioprio,
+	LSM_task_getioprio,
+	LSM_task_prlimit,
+	LSM_task_setrlimit,
+	LSM_task_setscheduler,
+	LSM_task_getscheduler,
+	LSM_task_movememory,
+	LSM_task_kill,
+	LSM_task_prctl,
+	LSM_task_to_inode,
+	LSM_ipc_permission,
+	LSM_ipc_getsecid,
+	LSM_msg_msg_alloc_security,
+	LSM_msg_msg_free_security,
+	LSM_msg_queue_alloc_security,
+	LSM_msg_queue_free_security,
+	LSM_msg_queue_associate,
+	LSM_msg_queue_msgctl,
+	LSM_msg_queue_msgsnd,
+	LSM_msg_queue_msgrcv,
+	LSM_shm_alloc_security,
+	LSM_shm_free_security,
+	LSM_shm_associate,
+	LSM_shm_shmctl,
+	LSM_shm_shmat,
+	LSM_sem_alloc_security,
+	LSM_sem_free_security,
+	LSM_sem_associate,
+	LSM_sem_semctl,
+	LSM_sem_semop,
+	LSM_netlink_send,
+	LSM_d_instantiate,
+	LSM_getprocattr,
+	LSM_setprocattr,
+	LSM_ismaclabel,
+	LSM_secid_to_secctx,
+	LSM_secctx_to_secid,
+	LSM_release_secctx,
+	LSM_inode_invalidate_secctx,
+	LSM_inode_notifysecctx,
+	LSM_inode_setsecctx,
+	LSM_inode_getsecctx,
 #ifdef CONFIG_SECURITY_NETWORK
-	struct list_head unix_stream_connect;
-	struct list_head unix_may_send;
-	struct list_head socket_create;
-	struct list_head socket_post_create;
-	struct list_head socket_bind;
-	struct list_head socket_connect;
-	struct list_head socket_listen;
-	struct list_head socket_accept;
-	struct list_head socket_sendmsg;
-	struct list_head socket_recvmsg;
-	struct list_head socket_getsockname;
-	struct list_head socket_getpeername;
-	struct list_head socket_getsockopt;
-	struct list_head socket_setsockopt;
-	struct list_head socket_shutdown;
-	struct list_head socket_sock_rcv_skb;
-	struct list_head socket_getpeersec_stream;
-	struct list_head socket_getpeersec_dgram;
-	struct list_head sk_alloc_security;
-	struct list_head sk_free_security;
-	struct list_head sk_clone_security;
-	struct list_head sk_getsecid;
-	struct list_head sock_graft;
-	struct list_head inet_conn_request;
-	struct list_head inet_csk_clone;
-	struct list_head inet_conn_established;
-	struct list_head secmark_relabel_packet;
-	struct list_head secmark_refcount_inc;
-	struct list_head secmark_refcount_dec;
-	struct list_head req_classify_flow;
-	struct list_head tun_dev_alloc_security;
-	struct list_head tun_dev_free_security;
-	struct list_head tun_dev_create;
-	struct list_head tun_dev_attach_queue;
-	struct list_head tun_dev_attach;
-	struct list_head tun_dev_open;
+	LSM_unix_stream_connect,
+	LSM_unix_may_send,
+	LSM_socket_create,
+	LSM_socket_post_create,
+	LSM_socket_bind,
+	LSM_socket_connect,
+	LSM_socket_listen,
+	LSM_socket_accept,
+	LSM_socket_sendmsg,
+	LSM_socket_recvmsg,
+	LSM_socket_getsockname,
+	LSM_socket_getpeername,
+	LSM_socket_getsockopt,
+	LSM_socket_setsockopt,
+	LSM_socket_shutdown,
+	LSM_socket_sock_rcv_skb,
+	LSM_socket_getpeersec_stream,
+	LSM_socket_getpeersec_dgram,
+	LSM_sk_alloc_security,
+	LSM_sk_free_security,
+	LSM_sk_clone_security,
+	LSM_sk_getsecid,
+	LSM_sock_graft,
+	LSM_inet_conn_request,
+	LSM_inet_csk_clone,
+	LSM_inet_conn_established,
+	LSM_secmark_relabel_packet,
+	LSM_secmark_refcount_inc,
+	LSM_secmark_refcount_dec,
+	LSM_req_classify_flow,
+	LSM_tun_dev_alloc_security,
+	LSM_tun_dev_free_security,
+	LSM_tun_dev_create,
+	LSM_tun_dev_attach_queue,
+	LSM_tun_dev_attach,
+	LSM_tun_dev_open,
 #endif	/* CONFIG_SECURITY_NETWORK */
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
-	struct list_head xfrm_policy_alloc_security;
-	struct list_head xfrm_policy_clone_security;
-	struct list_head xfrm_policy_free_security;
-	struct list_head xfrm_policy_delete_security;
-	struct list_head xfrm_state_alloc;
-	struct list_head xfrm_state_alloc_acquire;
-	struct list_head xfrm_state_free_security;
-	struct list_head xfrm_state_delete_security;
-	struct list_head xfrm_policy_lookup;
-	struct list_head xfrm_state_pol_flow_match;
-	struct list_head xfrm_decode_session;
+	LSM_xfrm_policy_alloc_security,
+	LSM_xfrm_policy_clone_security,
+	LSM_xfrm_policy_free_security,
+	LSM_xfrm_policy_delete_security,
+	LSM_xfrm_state_alloc,
+	LSM_xfrm_state_alloc_acquire,
+	LSM_xfrm_state_free_security,
+	LSM_xfrm_state_delete_security,
+	LSM_xfrm_policy_lookup,
+	LSM_xfrm_state_pol_flow_match,
+	LSM_xfrm_decode_session,
 #endif	/* CONFIG_SECURITY_NETWORK_XFRM */
 #ifdef CONFIG_KEYS
-	struct list_head key_alloc;
-	struct list_head key_free;
-	struct list_head key_permission;
-	struct list_head key_getsecurity;
+	LSM_key_alloc,
+	LSM_key_free,
+	LSM_key_permission,
+	LSM_key_getsecurity,
 #endif	/* CONFIG_KEYS */
 #ifdef CONFIG_AUDIT
-	struct list_head audit_rule_init;
-	struct list_head audit_rule_known;
-	struct list_head audit_rule_match;
-	struct list_head audit_rule_free;
+	LSM_audit_rule_init,
+	LSM_audit_rule_known,
+	LSM_audit_rule_match,
+	LSM_audit_rule_free,
 #endif /* CONFIG_AUDIT */
+	LSM_max_security_hook_index
 };
 
 /*
@@ -1884,8 +1885,8 @@  struct security_hook_heads {
  */
 struct security_hook_list {
 	struct list_head		list;
-	struct list_head		*head;
 	union security_list_options	hook;
+	enum security_hook_index	idx;
 	char				*lsm;
 };
 
@@ -1896,9 +1897,8 @@  struct security_hook_list {
  * text involved.
  */
 #define LSM_HOOK_INIT(HEAD, HOOK) \
-	{ .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
+	{ .idx = LSM_##HEAD, .hook = { .HEAD = HOOK } }
 
-extern struct security_hook_heads security_hook_heads;
 extern char *lsm_names;
 
 extern void security_add_hooks(struct security_hook_list *hooks, int count,
diff --git a/security/security.c b/security/security.c
index 38316bb..bd3c07e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -33,7 +33,8 @@ 
 /* Maximum number of letters for an LSM name string */
 #define SECURITY_NAME_MAX	10
 
-struct security_hook_heads security_hook_heads __lsm_ro_after_init;
+static struct list_head security_hook_heads[LSM_max_security_hook_index]
+	__lsm_ro_after_init;
 char *lsm_names;
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -56,12 +57,10 @@  static void __init do_security_initcalls(void)
  */
 int __init security_init(void)
 {
-	int i;
-	struct list_head *list = (struct list_head *) &security_hook_heads;
+	enum security_hook_index i;
 
-	for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct list_head);
-	     i++)
-		INIT_LIST_HEAD(&list[i]);
+	for (i = 0; i < LSM_max_security_hook_index; i++)
+		INIT_LIST_HEAD(&security_hook_heads[i]);
 	pr_info("Security Framework initialized\n");
 
 	/*
@@ -158,8 +157,12 @@  void __init security_add_hooks(struct security_hook_list *hooks, int count,
 	int i;
 
 	for (i = 0; i < count; i++) {
+		enum security_hook_index idx = hooks[i].idx;
+
 		hooks[i].lsm = lsm;
-		list_add_tail_rcu(&hooks[i].list, hooks[i].head);
+		/* Can't hit this BUG_ON() unless LSM_HOOK_INIT() is broken. */
+		BUG_ON(idx < 0 || idx >= LSM_max_security_hook_index);
+		list_add_tail_rcu(&hooks[i].list, &security_hook_heads[idx]);
 	}
 	if (lsm_append(lsm, &lsm_names) < 0)
 		panic("%s - Cannot get early memory.\n", __func__);
@@ -179,7 +182,8 @@  void __init security_add_hooks(struct security_hook_list *hooks, int count,
 	do {							\
 		struct security_hook_list *P;			\
 								\
-		list_for_each_entry(P, &security_hook_heads.FUNC, list)	\
+		list_for_each_entry(P, &security_hook_heads	\
+				    [LSM_##FUNC], list)		\
 			P->hook.FUNC(__VA_ARGS__);		\
 	} while (0)
 
@@ -188,7 +192,8 @@  void __init security_add_hooks(struct security_hook_list *hooks, int count,
 	do {							\
 		struct security_hook_list *P;			\
 								\
-		list_for_each_entry(P, &security_hook_heads.FUNC, list) { \
+		list_for_each_entry(P, &security_hook_heads     \
+				    [LSM_##FUNC], list) {	\
 			RC = P->hook.FUNC(__VA_ARGS__);		\
 			if (RC != 0)				\
 				break;				\
@@ -295,7 +300,8 @@  int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 	 * agree that it should be set it will. If any module
 	 * thinks it should not be set it won't.
 	 */
-	list_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
+	list_for_each_entry(hp, &security_hook_heads[LSM_vm_enough_memory],
+			    list) {
 		rc = hp->hook.vm_enough_memory(mm, pages);
 		if (rc <= 0) {
 			cap_sys_admin = 0;
@@ -785,7 +791,8 @@  int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
 	/*
 	 * Only one module will provide an attribute with a given name.
 	 */
-	list_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
+	list_for_each_entry(hp, &security_hook_heads[LSM_inode_getsecurity],
+			    list) {
 		rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
 		if (rc != -EOPNOTSUPP)
 			return rc;
@@ -803,7 +810,8 @@  int security_inode_setsecurity(struct inode *inode, const char *name, const void
 	/*
 	 * Only one module will provide an attribute with a given name.
 	 */
-	list_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
+	list_for_each_entry(hp, &security_hook_heads[LSM_inode_setsecurity],
+			    list) {
 		rc = hp->hook.inode_setsecurity(inode, name, value, size,
 								flags);
 		if (rc != -EOPNOTSUPP)
@@ -1111,7 +1119,7 @@  int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 	int rc = -ENOSYS;
 	struct security_hook_list *hp;
 
-	list_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
+	list_for_each_entry(hp, &security_hook_heads[LSM_task_prctl], list) {
 		thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
 		if (thisrc != -ENOSYS) {
 			rc = thisrc;
@@ -1587,8 +1595,8 @@  int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
 	 * For speed optimization, we explicitly break the loop rather than
 	 * using the macro
 	 */
-	list_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
-				list) {
+	list_for_each_entry(hp, &security_hook_heads
+			    [LSM_xfrm_state_pol_flow_match], list) {
 		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
 		break;
 	}