From 7c620ece125cbab7b5dfcb574ee1e64ab8b562cd Mon Sep 17 00:00:00 2001 From: Kyeongdon Kim Date: Wed, 6 Sep 2017 18:50:19 +0900 Subject: selinux: Use kmem_cache for hashtab_node During random test as own device to check slub account, we found some slack memory from hashtab_node(kmalloc-64). By using kzalloc(), middle of test result like below: allocated size 240768 request size 45144 slack size 195624 allocation count 3762 So, we want to use kmem_cache_zalloc() and that can reduce memory size 52byte(slack size/alloc count) per each struct. Signed-off-by: Kyeongdon Kim Signed-off-by: Paul Moore --- security/selinux/ss/hashtab.c | 17 +++++++++++++++-- security/selinux/ss/hashtab.h | 4 ++++ security/selinux/ss/services.c | 4 ++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c index 686c3917064c..bef7577d1270 100644 --- a/security/selinux/ss/hashtab.c +++ b/security/selinux/ss/hashtab.c @@ -9,6 +9,8 @@ #include #include "hashtab.h" +static struct kmem_cache *hashtab_node_cachep; + struct hashtab *hashtab_create(u32 (*hash_value)(struct hashtab *h, const void *key), int (*keycmp)(struct hashtab *h, const void *key1, const void *key2), u32 size) @@ -57,7 +59,7 @@ int hashtab_insert(struct hashtab *h, void *key, void *datum) if (cur && (h->keycmp(h, key, cur->key) == 0)) return -EEXIST; - newnode = kzalloc(sizeof(*newnode), GFP_KERNEL); + newnode = kmem_cache_zalloc(hashtab_node_cachep, GFP_KERNEL); if (!newnode) return -ENOMEM; newnode->key = key; @@ -106,7 +108,7 @@ void hashtab_destroy(struct hashtab *h) while (cur) { temp = cur; cur = cur->next; - kfree(temp); + kmem_cache_free(hashtab_node_cachep, temp); } h->htable[i] = NULL; } @@ -166,3 +168,14 @@ void hashtab_stat(struct hashtab *h, struct hashtab_info *info) info->slots_used = slots_used; info->max_chain_len = max_chain_len; } +void hashtab_cache_init(void) +{ + hashtab_node_cachep = kmem_cache_create("hashtab_node", + sizeof(struct hashtab_node), + 0, SLAB_PANIC, NULL); +} + +void hashtab_cache_destroy(void) +{ + kmem_cache_destroy(hashtab_node_cachep); +} diff --git a/security/selinux/ss/hashtab.h b/security/selinux/ss/hashtab.h index 009fb5e06172..d6883d3e7c5b 100644 --- a/security/selinux/ss/hashtab.h +++ b/security/selinux/ss/hashtab.h @@ -84,4 +84,8 @@ int hashtab_map(struct hashtab *h, /* Fill info with some hash table statistics */ void hashtab_stat(struct hashtab *h, struct hashtab_info *info); +/* Use kmem_cache for hashtab_node */ +void hashtab_cache_init(void); +void hashtab_cache_destroy(void); + #endif /* _SS_HASHTAB_H */ diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index e4a1c0dc561a..33cfe5d3d6cb 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2060,10 +2060,12 @@ int security_load_policy(void *data, size_t len) if (!ss_initialized) { avtab_cache_init(); ebitmap_cache_init(); + hashtab_cache_init(); rc = policydb_read(&policydb, fp); if (rc) { avtab_cache_destroy(); ebitmap_cache_destroy(); + hashtab_cache_destroy(); goto out; } @@ -2075,6 +2077,7 @@ int security_load_policy(void *data, size_t len) policydb_destroy(&policydb); avtab_cache_destroy(); ebitmap_cache_destroy(); + hashtab_cache_destroy(); goto out; } @@ -2083,6 +2086,7 @@ int security_load_policy(void *data, size_t len) policydb_destroy(&policydb); avtab_cache_destroy(); ebitmap_cache_destroy(); + hashtab_cache_destroy(); goto out; } -- cgit v1.2.3 From 6b240306ee1631587a87845127824df54a0a5abe Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 2 Oct 2017 09:38:20 -0500 Subject: selinux: Perform both commoncap and selinux xattr checks When selinux is loaded the relax permission checks for writing security.capable are not honored. Which keeps file capabilities from being used in user namespaces. Stephen Smalley writes: > Originally SELinux called the cap functions directly since there was no > stacking support in the infrastructure and one had to manually stack a > secondary module internally. inode_setxattr and inode_removexattr > however were special cases because the cap functions would check > CAP_SYS_ADMIN for any non-capability attributes in the security.* > namespace, and we don't want to impose that requirement on setting > security.selinux. Thus, we inlined the capabilities logic into the > selinux hook functions and adapted it appropriately. Now that the permission checks in commoncap have evolved this inlining of their contents has become a problem. So restructure selinux_inode_removexattr, and selinux_inode_setxattr to call both the corresponding cap_inode_ function and dentry_has_perm when the attribute is not a selinux security xattr. This ensures the policies of both commoncap and selinux are enforced. This results in smack and selinux having the same basic structure for setxattr and removexattr. Performing their own special permission checks when it is their modules xattr being written to, and deferring to commoncap when that is not the case. Then finally performing their generic module policy on all xattr writes. This structure is fine when you only consider stacking with the commoncap lsm, but it becomes a problem if two lsms that don't want the commoncap security checks on their own attributes need to be stack. This means there will need to be updates in the future as lsm stacking is improved, but at least now the structure between smack and selinux is common making the code easier to refactor. This change also has the effect that selinux_linux_setotherxattr becomes unnecessary so it is removed. Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") Fixes: 7bbf0e052b76 ("[PATCH] selinux merge") Historical Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git Signed-off-by: "Eric W. Biederman" Reviewed-by: Serge Hallyn Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/hooks.c | 43 ++++++++++++++++++------------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f5d304736852..c78dbec627f6 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3124,27 +3124,6 @@ static int selinux_inode_getattr(const struct path *path) return path_has_perm(current_cred(), path, FILE__GETATTR); } -static int selinux_inode_setotherxattr(struct dentry *dentry, const char *name) -{ - const struct cred *cred = current_cred(); - - if (!strncmp(name, XATTR_SECURITY_PREFIX, - sizeof XATTR_SECURITY_PREFIX - 1)) { - if (!strcmp(name, XATTR_NAME_CAPS)) { - if (!capable(CAP_SETFCAP)) - return -EPERM; - } else if (!capable(CAP_SYS_ADMIN)) { - /* A different attribute in the security namespace. - Restrict to administrator. */ - return -EPERM; - } - } - - /* Not an attribute we recognize, so just check the - ordinary setattr permission. */ - return dentry_has_perm(cred, dentry, FILE__SETATTR); -} - static bool has_cap_mac_admin(bool audit) { const struct cred *cred = current_cred(); @@ -3167,8 +3146,15 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, u32 newsid, sid = current_sid(); int rc = 0; - if (strcmp(name, XATTR_NAME_SELINUX)) - return selinux_inode_setotherxattr(dentry, name); + if (strcmp(name, XATTR_NAME_SELINUX)) { + rc = cap_inode_setxattr(dentry, name, value, size, flags); + if (rc) + return rc; + + /* Not an attribute we recognize, so just check the + ordinary setattr permission. */ + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); + } sbsec = inode->i_sb->s_security; if (!(sbsec->flags & SBLABEL_MNT)) @@ -3282,8 +3268,15 @@ static int selinux_inode_listxattr(struct dentry *dentry) static int selinux_inode_removexattr(struct dentry *dentry, const char *name) { - if (strcmp(name, XATTR_NAME_SELINUX)) - return selinux_inode_setotherxattr(dentry, name); + if (strcmp(name, XATTR_NAME_SELINUX)) { + int rc = cap_inode_removexattr(dentry, name); + if (rc) + return rc; + + /* Not an attribute we recognize, so just check the + ordinary setattr permission. */ + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); + } /* No one is allowed to remove a SELinux security label. You can change the label, but all data must be labeled. */ -- cgit v1.2.3 From c0d4f464caeb075e3bb9063a64cd63c093ac03ad Mon Sep 17 00:00:00 2001 From: Corentin LABBE Date: Wed, 4 Oct 2017 20:32:17 +0200 Subject: selinux: fix build warning by removing the unused sid variable This patch remove the unused variable sid This fix the following build warning: security/selinux/hooks.c:2921:6: warning: variable 'sid' set but not used [-Wunused-but-set-variable] Signed-off-by: Corentin Labbe Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/hooks.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index c78dbec627f6..46fc649ca886 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2918,13 +2918,12 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir, { const struct task_security_struct *tsec = current_security(); struct superblock_security_struct *sbsec; - u32 sid, newsid, clen; + u32 newsid, clen; int rc; char *context; sbsec = dir->i_sb->s_security; - sid = tsec->sid; newsid = tsec->create_sid; rc = selinux_determine_inode_label(current_security(), -- cgit v1.2.3 From 4298555df5e5cb956549de5b01e4c77b1e4bc00a Mon Sep 17 00:00:00 2001 From: Corentin LABBE Date: Wed, 4 Oct 2017 20:32:18 +0200 Subject: selinux: fix build warning This patch make selinux_task_prlimit() static since it is not used anywhere else. This fix the following build warning: security/selinux/hooks.c:3981:5: warning: no previous prototype for 'selinux_task_prlimit' [-Wmissing-prototypes] Signed-off-by: Corentin Labbe Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/hooks.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 46fc649ca886..2dd4dd6bdbc1 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3970,8 +3970,8 @@ static int selinux_task_getioprio(struct task_struct *p) PROCESS__GETSCHED, NULL); } -int selinux_task_prlimit(const struct cred *cred, const struct cred *tcred, - unsigned int flags) +static int selinux_task_prlimit(const struct cred *cred, const struct cred *tcred, + unsigned int flags) { u32 av = 0; -- cgit v1.2.3 From add24372141855b057bf53982824c5fe50898957 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Sat, 14 Oct 2017 13:46:55 +0100 Subject: selinux: remove redundant assignment to str str is being assigned to an empty string but str is never being read after that, so the assignment is redundant and can be removed. Moving the declaration of str to a more localised block, cleans up clang warning: "Value stored to 'str' is never read" Signed-off-by: Colin Ian King Signed-off-by: Paul Moore --- security/selinux/hooks.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 2dd4dd6bdbc1..f21f1e0e6452 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3176,18 +3176,17 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, if (!has_cap_mac_admin(true)) { struct audit_buffer *ab; size_t audit_size; - const char *str; /* We strip a nul only if it is at the end, otherwise the * context contains a nul and we should audit that */ if (value) { - str = value; + const char *str = value; + if (str[size - 1] == '\0') audit_size = size - 1; else audit_size = size; } else { - str = ""; audit_size = 0; } ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR); -- cgit v1.2.3 From 73e4977873bfbd8698d5b116ba32d48f2a889276 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Sat, 14 Oct 2017 16:00:49 +0100 Subject: selinux: remove redundant assignment to len The variable len is being set to zero and this value is never being read since len is being set to a different value just a few lines later. Remove this redundant assignment. Cleans up clang warning: Value stored to 'len' is never read Signed-off-by: Colin Ian King Signed-off-by: Paul Moore --- security/selinux/ss/conditional.c | 1 - 1 file changed, 1 deletion(-) diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 771c96afe1d5..c91543a617ac 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -361,7 +361,6 @@ static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list * *ret_list = NULL; - len = 0; rc = next_entry(buf, fp, sizeof(u32)); if (rc) return rc; -- cgit v1.2.3 From 5794ed762ac2125299644494766704da94168ec0 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Sat, 14 Oct 2017 16:38:56 +0100 Subject: selinux: remove extraneous initialization of slots_used and max_chain_len Variables slots_used and max_chain_len are being initialized to zero twice. Remove the second set of initializations in the for loop. Cleans up the clang warnings: Value stored to 'slots_used' is never read Value stored to 'max_chain_len' is never read Signed-off-by: Colin Ian King Signed-off-by: Paul Moore --- security/selinux/ss/hashtab.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c index bef7577d1270..e0443f4afea5 100644 --- a/security/selinux/ss/hashtab.c +++ b/security/selinux/ss/hashtab.c @@ -150,7 +150,7 @@ void hashtab_stat(struct hashtab *h, struct hashtab_info *info) slots_used = 0; max_chain_len = 0; - for (slots_used = max_chain_len = i = 0; i < h->size; i++) { + for (i = 0; i < h->size; i++) { cur = h->htable[i]; if (cur) { slots_used++; -- cgit v1.2.3