Re: [PATCH v4 1/3] security: Refactor LSM hooks into an array and enum
On Wed, Mar 7, 2018 at 12:23 PM, Casey Schaufler wrote: > On 3/7/2018 11:18 AM, Sargun Dhillon wrote: >> On Wed, Mar 7, 2018 at 9:45 AM, Casey Schaufler >> wrote: >>> On 3/6/2018 11:23 PM, Sargun Dhillon wrote: This commit should have no functional change. It changes the security hook list heads struct into an array. Additionally, it exposes all of the hooks via an enum. This loses memory layout randomization as the enum is not randomized. >>> Please explain why you want to do this. I still dislike it. >>> >> Do you dislike it because of the loss of randomization, or some other reason? > > I dislike a huge array of untyped function pointers. > I dislike the loss of type checking in security.c > Ok, I can go back to that. My previous approach was using two list_heads, but people suggested otherwise. Would you be okay with the >> The reason for not just having a second list_heads is that it's >> somewhat ugly having to replicate that structure twice -- once for >> dynamic hooks, and once for 'static' hooks. > > There was discussion about this some time ago. In the case > where you don't allow dynamic hooks, you mark the lists ro_after_init > whereas in the case with them you don't, but use the locking. > > Why expose the compiled-in hooks to being RW? To me, having two list_heads seems better, because you have the built-in ones be read only, and the ones at load time be read/write. In fact, we could protect the r/w with pmalloc? >> Instead, we have one enum that LSMs can use and two arrays of heads >> rather than an entire unrolled set of list_heads. > > But how is this better? What is the advantage? > >> >> If we had a way to randomize this, would it make you comfortable? >> >
Re: [PATCH v4 1/3] security: Refactor LSM hooks into an array and enum
On 3/7/2018 11:18 AM, Sargun Dhillon wrote: > On Wed, Mar 7, 2018 at 9:45 AM, Casey Schaufler > wrote: >> On 3/6/2018 11:23 PM, Sargun Dhillon wrote: >>> This commit should have no functional change. It changes the security hook >>> list heads struct into an array. Additionally, it exposes all of the hooks >>> via an enum. This loses memory layout randomization as the enum is not >>> randomized. >> Please explain why you want to do this. I still dislike it. >> > Do you dislike it because of the loss of randomization, or some other reason? I dislike a huge array of untyped function pointers. I dislike the loss of type checking in security.c > The reason for not just having a second list_heads is that it's > somewhat ugly having to replicate that structure twice -- once for > dynamic hooks, and once for 'static' hooks. There was discussion about this some time ago. In the case where you don't allow dynamic hooks, you mark the lists ro_after_init whereas in the case with them you don't, but use the locking. > Instead, we have one enum that LSMs can use and two arrays of heads > rather than an entire unrolled set of list_heads. But how is this better? What is the advantage? > > If we had a way to randomize this, would it make you comfortable? >
Re: [PATCH v4 1/3] security: Refactor LSM hooks into an array and enum
On Wed, Mar 7, 2018 at 9:45 AM, Casey Schaufler wrote: > On 3/6/2018 11:23 PM, Sargun Dhillon wrote: >> This commit should have no functional change. It changes the security hook >> list heads struct into an array. Additionally, it exposes all of the hooks >> via an enum. This loses memory layout randomization as the enum is not >> randomized. > > Please explain why you want to do this. I still dislike it. > Do you dislike it because of the loss of randomization, or some other reason? The reason for not just having a second list_heads is that it's somewhat ugly having to replicate that structure twice -- once for dynamic hooks, and once for 'static' hooks. Instead, we have one enum that LSMs can use and two arrays of heads rather than an entire unrolled set of list_heads. If we had a way to randomize this, would it make you comfortable?
Re: [PATCH v4 1/3] security: Refactor LSM hooks into an array and enum
On 3/6/2018 11:23 PM, Sargun Dhillon wrote: > This commit should have no functional change. It changes the security hook > list heads struct into an array. Additionally, it exposes all of the hooks > via an enum. This loses memory layout randomization as the enum is not > randomized. Please explain why you want to do this. I still dislike it.
[PATCH v4 1/3] security: Refactor LSM hooks into an array and enum
This commit should have no functional change. It changes the security hook
list heads struct into an array. Additionally, it exposes all of the hooks
via an enum. This loses memory layout randomization as the enum is not
randomized.
Signed-off-by: Sargun Dhillon
---
include/linux/lsm_hooks.h | 433 +++---
security/security.c | 30 ++--
2 files changed, 233 insertions(+), 230 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 7161d8e7ee79..d28c7f5b01c1 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1729,241 +1729,243 @@ union security_list_options {
#endif /* CONFIG_BPF_SYSCALL */
};
-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_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 lsm_hook {
+ LSM_HOOK_binder_set_context_mgr,
+ LSM_HOOK_binder_transaction,
+ LSM_HOOK_binder_transfer_binder,
+ LSM_HOOK_binder_transfer_file,
+ LSM_HOOK_ptrace_access_check,
+ LSM_HOOK_ptrace_traceme,
+ LSM_HOOK_capget,
+ LSM_HOOK_capset,
+ LSM_HOOK_capable,
+ LSM_HOOK_quotactl,
+ LSM_HOOK_quota_on,
+ LSM_HOOK_syslog,
+ LSM_HOOK_settime,
+ LSM_HOOK_vm_enough_memory,
+ LSM_HOOK_bprm_set_creds,
+ LSM_HOOK_bprm_check_security,
+ LSM_HOOK_bprm_committing_creds,
+ LSM_HOOK_bprm_committed_creds,
+ LSM_HOOK_sb_alloc_security,
+ LSM_HOOK_sb_free_security,
+ LSM_HOOK_sb_copy_data,
+ LSM_HOOK_sb_remount,
+ LSM_HOOK_sb_kern_mount,
+ LSM_HOOK_sb_show_options,
+ LSM_HOOK_sb_statfs,
+ LSM_HOOK_sb_mount,
+ LSM_HOOK_sb_umount,
+ LSM_HOOK_sb_pivotroot,
+ LSM_HOOK_sb_set_mnt_opts,
+ LSM_HOOK_sb_clone_mnt_opts,
+ LSM_HOOK_sb_parse_opts_str,
+ LSM_HOOK_dentry_init_security,
+ LSM_HOOK_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_HOOK_path_unlink,
+ LSM_HOOK_path_mkdir,
+ LSM_HOOK_path_rmdir,
+ LSM_HOOK_path_mknod,
+ LSM_HOOK_path_truncate,
+ LSM_HOOK_path_symlink,
+ LSM_HOOK_path_link,
+ LSM_HOOK_path_rename,
+ LSM_HOOK_path_chmod,
+ LSM_HOOK_path_chown,
+ LSM_HOOK_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
