From 4b36cb773a8153417a080f8025d522322f915aea Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Fri, 17 Jan 2020 14:15:14 +0100 Subject: [PATCH 01/20] selinux: move status variables out of selinux_ss It fits more naturally in selinux_state, since it reflects also global state (the enforcing and policyload fields). Signed-off-by: Ondrej Mosnacek Reviewed-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/Makefile | 4 ++-- security/selinux/hooks.c | 1 + security/selinux/include/security.h | 4 ++++ security/selinux/ss/services.c | 2 -- security/selinux/ss/services.h | 2 -- security/selinux/{ss => }/status.c | 32 ++++++++++++++--------------- 6 files changed, 23 insertions(+), 22 deletions(-) rename security/selinux/{ss => }/status.c (81%) diff --git a/security/selinux/Makefile b/security/selinux/Makefile index 2000f95fb197..0c77ede1cc11 100644 --- a/security/selinux/Makefile +++ b/security/selinux/Makefile @@ -6,9 +6,9 @@ obj-$(CONFIG_SECURITY_SELINUX) := selinux.o selinux-y := avc.o hooks.o selinuxfs.o netlink.o nlmsgtab.o netif.o \ - netnode.o netport.o \ + netnode.o netport.o status.o \ ss/ebitmap.o ss/hashtab.o ss/symtab.o ss/sidtab.o ss/avtab.o \ - ss/policydb.o ss/services.o ss/conditional.o ss/mls.o ss/status.o + ss/policydb.o ss/services.o ss/conditional.o ss/mls.o selinux-$(CONFIG_SECURITY_NETWORK_XFRM) += xfrm.o diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 4b6991e178d3..b33cf155cc48 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -7161,6 +7161,7 @@ static __init int selinux_init(void) selinux_state.checkreqprot = selinux_checkreqprot_boot; selinux_ss_init(&selinux_state.ss); selinux_avc_init(&selinux_state.avc); + mutex_init(&selinux_state.status_lock); /* Set the security state for the initial task. */ cred_init_security(); diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index a39f9565d80b..f3a621058aba 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -108,6 +108,10 @@ struct selinux_state { bool checkreqprot; bool initialized; bool policycap[__POLICYDB_CAPABILITY_MAX]; + + struct page *status_page; + struct mutex status_lock; + struct selinux_avc *avc; struct selinux_ss *ss; } __randomize_layout; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 216ce602a2b5..5cf491768142 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -46,7 +46,6 @@ #include #include #include -#include #include #include @@ -81,7 +80,6 @@ static struct selinux_ss selinux_ss; void selinux_ss_init(struct selinux_ss **ss) { rwlock_init(&selinux_ss.policy_rwlock); - mutex_init(&selinux_ss.status_lock); *ss = &selinux_ss; } diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h index c5896f39e8f6..e9bddf33e53d 100644 --- a/security/selinux/ss/services.h +++ b/security/selinux/ss/services.h @@ -29,8 +29,6 @@ struct selinux_ss { rwlock_t policy_rwlock; u32 latest_granting; struct selinux_map map; - struct page *status_page; - struct mutex status_lock; } __randomize_layout; void services_compute_xperms_drivers(struct extended_perms *xperms, diff --git a/security/selinux/ss/status.c b/security/selinux/status.c similarity index 81% rename from security/selinux/ss/status.c rename to security/selinux/status.c index 3c554a442467..4bc8f809934c 100644 --- a/security/selinux/ss/status.c +++ b/security/selinux/status.c @@ -11,7 +11,7 @@ #include #include #include "avc.h" -#include "services.h" +#include "security.h" /* * The selinux_status_page shall be exposed to userspace applications @@ -44,12 +44,12 @@ struct page *selinux_kernel_status_page(struct selinux_state *state) struct selinux_kernel_status *status; struct page *result = NULL; - mutex_lock(&state->ss->status_lock); - if (!state->ss->status_page) { - state->ss->status_page = alloc_page(GFP_KERNEL|__GFP_ZERO); + mutex_lock(&state->status_lock); + if (!state->status_page) { + state->status_page = alloc_page(GFP_KERNEL|__GFP_ZERO); - if (state->ss->status_page) { - status = page_address(state->ss->status_page); + if (state->status_page) { + status = page_address(state->status_page); status->version = SELINUX_KERNEL_STATUS_VERSION; status->sequence = 0; @@ -65,8 +65,8 @@ struct page *selinux_kernel_status_page(struct selinux_state *state) !security_get_allow_unknown(state); } } - result = state->ss->status_page; - mutex_unlock(&state->ss->status_lock); + result = state->status_page; + mutex_unlock(&state->status_lock); return result; } @@ -81,9 +81,9 @@ void selinux_status_update_setenforce(struct selinux_state *state, { struct selinux_kernel_status *status; - mutex_lock(&state->ss->status_lock); - if (state->ss->status_page) { - status = page_address(state->ss->status_page); + mutex_lock(&state->status_lock); + if (state->status_page) { + status = page_address(state->status_page); status->sequence++; smp_wmb(); @@ -93,7 +93,7 @@ void selinux_status_update_setenforce(struct selinux_state *state, smp_wmb(); status->sequence++; } - mutex_unlock(&state->ss->status_lock); + mutex_unlock(&state->status_lock); } /* @@ -107,9 +107,9 @@ void selinux_status_update_policyload(struct selinux_state *state, { struct selinux_kernel_status *status; - mutex_lock(&state->ss->status_lock); - if (state->ss->status_page) { - status = page_address(state->ss->status_page); + mutex_lock(&state->status_lock); + if (state->status_page) { + status = page_address(state->status_page); status->sequence++; smp_wmb(); @@ -120,5 +120,5 @@ void selinux_status_update_policyload(struct selinux_state *state, smp_wmb(); status->sequence++; } - mutex_unlock(&state->ss->status_lock); + mutex_unlock(&state->status_lock); } From e9c38f9fc2ccd31befe1bb1605b69213483a15b7 Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Wed, 8 Jan 2020 11:24:47 -0500 Subject: [PATCH 02/20] Documentation,selinux: deprecate setting checkreqprot to 1 Deprecate setting the SELinux checkreqprot tunable to 1 via kernel parameter or /sys/fs/selinux/checkreqprot. Setting it to 0 is left intact for compatibility since Android and some Linux distributions do so for security and treat an inability to set it as a fatal error. Eventually setting it to 0 will become a no-op and the kernel will stop using checkreqprot's value internally altogether. checkreqprot was originally introduced as a compatibility mechanism for legacy userspace and the READ_IMPLIES_EXEC personality flag. However, if set to 1, it weakens security by allowing mappings to be made executable without authorization by policy. The default value for the SECURITY_SELINUX_CHECKREQPROT_VALUE config option was changed from 1 to 0 in commit 2a35d196c160e3 ("selinux: change CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE default") and both Android and Linux distributions began explicitly setting /sys/fs/selinux/checkreqprot to 0 some time ago. Signed-off-by: Stephen Smalley Signed-off-by: Paul Moore --- .../ABI/obsolete/sysfs-selinux-checkreqprot | 23 +++++++++++++++++++ .../admin-guide/kernel-parameters.txt | 1 + MAINTAINERS | 1 + security/selinux/Kconfig | 3 +++ security/selinux/hooks.c | 5 +++- security/selinux/selinuxfs.c | 8 +++++++ 6 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/obsolete/sysfs-selinux-checkreqprot diff --git a/Documentation/ABI/obsolete/sysfs-selinux-checkreqprot b/Documentation/ABI/obsolete/sysfs-selinux-checkreqprot new file mode 100644 index 000000000000..49ed9c8fd1e5 --- /dev/null +++ b/Documentation/ABI/obsolete/sysfs-selinux-checkreqprot @@ -0,0 +1,23 @@ +What: /sys/fs/selinux/checkreqprot +Date: April 2005 (predates git) +KernelVersion: 2.6.12-rc2 (predates git) +Contact: selinux@vger.kernel.org +Description: + + The selinuxfs "checkreqprot" node allows SELinux to be configured + to check the protection requested by userspace for mmap/mprotect + calls instead of the actual protection applied by the kernel. + This was a compatibility mechanism for legacy userspace and + for the READ_IMPLIES_EXEC personality flag. However, if set to + 1, it weakens security by allowing mappings to be made executable + without authorization by policy. The default value of checkreqprot + at boot was changed starting in Linux v4.4 to 0 (i.e. check the + actual protection), and Android and Linux distributions have been + explicitly writing a "0" to /sys/fs/selinux/checkreqprot during + initialization for some time. Support for setting checkreqprot to 1 + will be removed in a future kernel release, at which point the kernel + will always cease using checkreqprot internally and will always + check the actual protections being applied upon mmap/mprotect calls. + The checkreqprot selinuxfs node will remain for backward compatibility + but will discard writes of the "0" value and will reject writes of the + "1" value when this mechanism is removed. diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index dbc22d684627..ff1428d69b2d 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -518,6 +518,7 @@ Default value is set via a kernel config option. Value can be changed at runtime via /sys/fs/selinux/checkreqprot. + Setting checkreqprot to 1 is deprecated. cio_ignore= [S390] See Documentation/s390/common_io.rst for details. diff --git a/MAINTAINERS b/MAINTAINERS index 38fe2f3f7b6f..0b370797e8a6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14986,6 +14986,7 @@ F: security/selinux/ F: scripts/selinux/ F: Documentation/admin-guide/LSM/SELinux.rst F: Documentation/ABI/obsolete/sysfs-selinux-disable +F: Documentation/ABI/obsolete/sysfs-selinux-checkreqprot SENSABLE PHANTOM M: Jiri Slaby diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig index 1014cb0ee956..9e921fc72538 100644 --- a/security/selinux/Kconfig +++ b/security/selinux/Kconfig @@ -88,6 +88,9 @@ config SECURITY_SELINUX_CHECKREQPROT_VALUE 'checkreqprot=' boot parameter. It may also be changed at runtime via /sys/fs/selinux/checkreqprot if authorized by policy. + WARNING: this option is deprecated and will be removed in a future + kernel release. + If you are unsure how to answer this question, answer 0. config SECURITY_SELINUX_SIDTAB_HASH_BITS diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index b33cf155cc48..d7762264f262 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -142,8 +142,11 @@ static int __init checkreqprot_setup(char *str) { unsigned long checkreqprot; - if (!kstrtoul(str, 0, &checkreqprot)) + if (!kstrtoul(str, 0, &checkreqprot)) { selinux_checkreqprot_boot = checkreqprot ? 1 : 0; + if (checkreqprot) + pr_warn("SELinux: checkreqprot set to 1 via kernel parameter. This is deprecated and will be rejected in a future kernel release.\n"); + } return 1; } __setup("checkreqprot=", checkreqprot_setup); diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 79c710911a3c..d6566adc73a6 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -668,6 +668,14 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf, if (sscanf(page, "%u", &new_value) != 1) goto out; + if (new_value) { + char comm[sizeof(current->comm)]; + + memcpy(comm, current->comm, sizeof(comm)); + pr_warn_once("SELinux: %s (%d) set checkreqprot to 1. This is deprecated and will be rejected in a future kernel release.\n", + comm, current->pid); + } + fsi->state->checkreqprot = new_value ? 1 : 0; length = count; out: From 06c2efe2cf3aa70abbdf97e88641abca2e707a15 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Fri, 17 Jan 2020 09:58:32 +0100 Subject: [PATCH 03/20] selinux: simplify evaluate_cond_node() It never fails, so it can just return void. Signed-off-by: Ondrej Mosnacek Reviewed-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/ss/conditional.c | 3 +-- security/selinux/ss/conditional.h | 2 +- security/selinux/ss/services.c | 14 ++++---------- 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 70c378ee1a2f..04593062008d 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -85,7 +85,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr) * list appropriately. If the result of the expression is undefined * all of the rules are disabled for safety. */ -int evaluate_cond_node(struct policydb *p, struct cond_node *node) +void evaluate_cond_node(struct policydb *p, struct cond_node *node) { int new_state; struct cond_av_list *cur; @@ -111,7 +111,6 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node) cur->node->key.specified |= AVTAB_ENABLED; } } - return 0; } int cond_policydb_init(struct policydb *p) diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h index ec846e45904c..d86ef286ca84 100644 --- a/security/selinux/ss/conditional.h +++ b/security/selinux/ss/conditional.h @@ -75,6 +75,6 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key, struct av_decision *avd, struct extended_perms *xperms); void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key, struct extended_perms_decision *xpermd); -int evaluate_cond_node(struct policydb *p, struct cond_node *node); +void evaluate_cond_node(struct policydb *p, struct cond_node *node); #endif /* _CONDITIONAL_H_ */ diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 5cf491768142..922b5e4a03e8 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2956,11 +2956,8 @@ int security_set_bools(struct selinux_state *state, int len, int *values) policydb->bool_val_to_struct[i]->state = 0; } - for (cur = policydb->cond_list; cur; cur = cur->next) { - rc = evaluate_cond_node(policydb, cur); - if (rc) - goto out; - } + for (cur = policydb->cond_list; cur; cur = cur->next) + evaluate_cond_node(policydb, cur); seqno = ++state->ss->latest_granting; rc = 0; @@ -3013,11 +3010,8 @@ static int security_preserve_bools(struct selinux_state *state, if (booldatum) booldatum->state = bvalues[i]; } - for (cur = policydb->cond_list; cur; cur = cur->next) { - rc = evaluate_cond_node(policydb, cur); - if (rc) - goto out; - } + for (cur = policydb->cond_list; cur; cur = cur->next) + evaluate_cond_node(policydb, cur); out: if (bnames) { From 7470d0d13fb680bb82b40f18831f7d4ee7a4bb62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 28 Jan 2020 20:16:48 +0100 Subject: [PATCH 04/20] selinux: allow kernfs symlinks to inherit parent directory context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently symlinks on kernel filesystems, like sysfs, are labeled on creation with the parent filesystem root sid. Allow symlinks to inherit the parent directory context, so fine-grained kernfs labeling can be applied to symlinks too and checking contexts doesn't complain about them. For backward-compatibility this behavior is contained in a new policy capability: genfs_seclabel_symlinks Signed-off-by: Christian Göttsche Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/hooks.c | 4 +++- security/selinux/include/security.h | 8 ++++++++ security/selinux/ss/services.c | 3 ++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d7762264f262..7c37cdb3aba0 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1478,7 +1478,9 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent /* Default to the fs superblock SID. */ sid = sbsec->sid; - if ((sbsec->flags & SE_SBGENFS) && !S_ISLNK(inode->i_mode)) { + if ((sbsec->flags & SE_SBGENFS) && + (!S_ISLNK(inode->i_mode) || + selinux_policycap_genfs_seclabel_symlinks())) { /* We must have a dentry to determine the label on * procfs inodes */ if (opt_dentry) { diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index f3a621058aba..d6036c018cf2 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -79,6 +79,7 @@ enum { POLICYDB_CAPABILITY_ALWAYSNETWORK, POLICYDB_CAPABILITY_CGROUPSECLABEL, POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION, + POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS, __POLICYDB_CAPABILITY_MAX }; #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1) @@ -213,6 +214,13 @@ static inline bool selinux_policycap_nnp_nosuid_transition(void) return state->policycap[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION]; } +static inline bool selinux_policycap_genfs_seclabel_symlinks(void) +{ + struct selinux_state *state = &selinux_state; + + return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]; +} + int security_mls_enabled(struct selinux_state *state); int security_load_policy(struct selinux_state *state, void *data, size_t len); diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 922b5e4a03e8..e310f8ee21a1 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -72,7 +72,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = { "extended_socket_class", "always_check_network", "cgroup_seclabel", - "nnp_nosuid_transition" + "nnp_nosuid_transition", + "genfs_seclabel_symlinks" }; static struct selinux_ss selinux_ss; From 8d269a8e2a8f0bca89022f4ec98de460acb90365 Mon Sep 17 00:00:00 2001 From: Vasily Averin Date: Sat, 1 Feb 2020 10:47:47 +0300 Subject: [PATCH 05/20] selinux: sel_avc_get_stat_idx should increase position index If seq_file .next function does not change position index, read after some lseek can generate unexpected output. $ dd if=/sys/fs/selinux/avc/cache_stats # usual output lookups hits misses allocations reclaims frees 817223 810034 7189 7189 6992 7037 1934894 1926896 7998 7998 7632 7683 1322812 1317176 5636 5636 5456 5507 1560571 1551548 9023 9023 9056 9115 0+1 records in 0+1 records out 189 bytes copied, 5,1564e-05 s, 3,7 MB/s $# read after lseek to midle of last line $ dd if=/sys/fs/selinux/avc/cache_stats bs=180 skip=1 dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset 056 9115 <<<< end of last line 1560571 1551548 9023 9023 9056 9115 <<< whole last line once again 0+1 records in 0+1 records out 45 bytes copied, 8,7221e-05 s, 516 kB/s $# read after lseek beyond end of of file $ dd if=/sys/fs/selinux/avc/cache_stats bs=1000 skip=1 dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset 1560571 1551548 9023 9023 9056 9115 <<<< generates whole last line 0+1 records in 0+1 records out 36 bytes copied, 9,0934e-05 s, 396 kB/s https://bugzilla.kernel.org/show_bug.cgi?id=206283 Signed-off-by: Vasily Averin Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/selinuxfs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index d6566adc73a6..66e6ed7fac56 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -1544,6 +1544,7 @@ static struct avc_cache_stats *sel_avc_get_stat_idx(loff_t *idx) *idx = cpu + 1; return &per_cpu(avc_cache_stats, cpu); } + (*idx)++; return NULL; } From 60abd3181db29ea81742106cc0ac2e27fd05b418 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Mon, 3 Feb 2020 12:27:20 +0100 Subject: [PATCH 06/20] selinux: convert cond_list to array Since it is fixed-size after allocation and we know the size beforehand, using a plain old array is simpler and more efficient. While there, also fix signedness of some related variables/parameters. Signed-off-by: Ondrej Mosnacek Signed-off-by: Paul Moore --- security/selinux/include/conditional.h | 8 ++-- security/selinux/selinuxfs.c | 4 +- security/selinux/ss/conditional.c | 54 ++++++++++---------------- security/selinux/ss/conditional.h | 3 +- security/selinux/ss/policydb.c | 2 +- security/selinux/ss/policydb.h | 3 +- security/selinux/ss/services.c | 28 ++++++------- 7 files changed, 43 insertions(+), 59 deletions(-) diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h index 0ab316f61da0..539ab357707d 100644 --- a/security/selinux/include/conditional.h +++ b/security/selinux/include/conditional.h @@ -14,12 +14,10 @@ #include "security.h" int security_get_bools(struct selinux_state *state, - int *len, char ***names, int **values); + u32 *len, char ***names, int **values); -int security_set_bools(struct selinux_state *state, - int len, int *values); +int security_set_bools(struct selinux_state *state, u32 len, int *values); -int security_get_bool_value(struct selinux_state *state, - int index); +int security_get_bool_value(struct selinux_state *state, u32 index); #endif diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 66e6ed7fac56..533ab170ad52 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -1335,14 +1335,14 @@ static void sel_remove_entries(struct dentry *de) static int sel_make_bools(struct selinux_fs_info *fsi) { - int i, ret; + int ret; ssize_t len; struct dentry *dentry = NULL; struct dentry *dir = fsi->bool_dir; struct inode *inode = NULL; struct inode_security_struct *isec; char **names = NULL, *page; - int num; + u32 i, num; int *values = NULL; u32 sid; diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 04593062008d..e6d203b76545 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -119,6 +119,7 @@ int cond_policydb_init(struct policydb *p) p->bool_val_to_struct = NULL; p->cond_list = NULL; + p->cond_list_len = 0; rc = avtab_init(&p->te_cond_avtab); if (rc) @@ -147,27 +148,22 @@ static void cond_node_destroy(struct cond_node *node) } cond_av_list_destroy(node->true_list); cond_av_list_destroy(node->false_list); - kfree(node); } -static void cond_list_destroy(struct cond_node *list) +static void cond_list_destroy(struct policydb *p) { - struct cond_node *next, *cur; + u32 i; - if (list == NULL) - return; - - for (cur = list; cur; cur = next) { - next = cur->next; - cond_node_destroy(cur); - } + for (i = 0; i < p->cond_list_len; i++) + cond_node_destroy(&p->cond_list[i]); + kfree(p->cond_list); } void cond_policydb_destroy(struct policydb *p) { kfree(p->bool_val_to_struct); avtab_destroy(&p->te_cond_avtab); - cond_list_destroy(p->cond_list); + cond_list_destroy(p); } int cond_init_bool_indexes(struct policydb *p) @@ -447,7 +443,6 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp) int cond_read_list(struct policydb *p, void *fp) { - struct cond_node *node, *last = NULL; __le32 buf[1]; u32 i, len; int rc; @@ -458,29 +453,24 @@ int cond_read_list(struct policydb *p, void *fp) len = le32_to_cpu(buf[0]); + p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL); + if (!p->cond_list) + return rc; + rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel); if (rc) goto err; - for (i = 0; i < len; i++) { - rc = -ENOMEM; - node = kzalloc(sizeof(*node), GFP_KERNEL); - if (!node) - goto err; + p->cond_list_len = len; - rc = cond_read_node(p, node, fp); + for (i = 0; i < len; i++) { + rc = cond_read_node(p, &p->cond_list[i], fp); if (rc) goto err; - - if (i == 0) - p->cond_list = node; - else - last->next = node; - last = node; } return 0; err: - cond_list_destroy(p->cond_list); + cond_list_destroy(p); p->cond_list = NULL; return rc; } @@ -585,23 +575,19 @@ static int cond_write_node(struct policydb *p, struct cond_node *node, return 0; } -int cond_write_list(struct policydb *p, struct cond_node *list, void *fp) +int cond_write_list(struct policydb *p, void *fp) { - struct cond_node *cur; - u32 len; + u32 i; __le32 buf[1]; int rc; - len = 0; - for (cur = list; cur != NULL; cur = cur->next) - len++; - buf[0] = cpu_to_le32(len); + buf[0] = cpu_to_le32(p->cond_list_len); rc = put_entry(buf, sizeof(u32), 1, fp); if (rc) return rc; - for (cur = list; cur != NULL; cur = cur->next) { - rc = cond_write_node(p, cur, fp); + for (i = 0; i < p->cond_list_len; i++) { + rc = cond_write_node(p, &p->cond_list[i], fp); if (rc) return rc; } diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h index d86ef286ca84..e474bdd3a0ed 100644 --- a/security/selinux/ss/conditional.h +++ b/security/selinux/ss/conditional.h @@ -55,7 +55,6 @@ struct cond_node { struct cond_expr *expr; struct cond_av_list *true_list; struct cond_av_list *false_list; - struct cond_node *next; }; int cond_policydb_init(struct policydb *p); @@ -69,7 +68,7 @@ int cond_index_bool(void *key, void *datum, void *datap); int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp); int cond_read_list(struct policydb *p, void *fp); int cond_write_bool(void *key, void *datum, void *ptr); -int cond_write_list(struct policydb *p, struct cond_node *list, void *fp); +int cond_write_list(struct policydb *p, void *fp); void cond_compute_av(struct avtab *ctab, struct avtab_key *key, struct av_decision *avd, struct extended_perms *xperms); diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 2aa7f2e1a8e7..8ac9b9ffc83c 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -3483,7 +3483,7 @@ int policydb_write(struct policydb *p, void *fp) if (rc) return rc; - rc = cond_write_list(p, p->cond_list, fp); + rc = cond_write_list(p, fp); if (rc) return rc; diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 69b24191fa38..6459616f8487 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -272,8 +272,9 @@ struct policydb { struct cond_bool_datum **bool_val_to_struct; /* type enforcement conditional access vectors and transitions */ struct avtab te_cond_avtab; - /* linked list indexing te_cond_avtab by conditional */ + /* array indexing te_cond_avtab by conditional */ struct cond_node *cond_list; + u32 cond_list_len; /* role allows */ struct role_allow *role_allow; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index e310f8ee21a1..1e652d6ed8cd 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2867,10 +2867,11 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) } int security_get_bools(struct selinux_state *state, - int *len, char ***names, int **values) + u32 *len, char ***names, int **values) { struct policydb *policydb; - int i, rc; + u32 i; + int rc; if (!selinux_initialized(state)) { *len = 0; @@ -2924,12 +2925,11 @@ int security_get_bools(struct selinux_state *state, } -int security_set_bools(struct selinux_state *state, int len, int *values) +int security_set_bools(struct selinux_state *state, u32 len, int *values) { struct policydb *policydb; - int i, rc; - int lenp, seqno = 0; - struct cond_node *cur; + int rc; + u32 i, lenp, seqno = 0; write_lock_irq(&state->ss->policy_rwlock); @@ -2957,8 +2957,8 @@ int security_set_bools(struct selinux_state *state, int len, int *values) policydb->bool_val_to_struct[i]->state = 0; } - for (cur = policydb->cond_list; cur; cur = cur->next) - evaluate_cond_node(policydb, cur); + for (i = 0; i < policydb->cond_list_len; i++) + evaluate_cond_node(policydb, &policydb->cond_list[i]); seqno = ++state->ss->latest_granting; rc = 0; @@ -2974,11 +2974,11 @@ int security_set_bools(struct selinux_state *state, int len, int *values) } int security_get_bool_value(struct selinux_state *state, - int index) + u32 index) { struct policydb *policydb; int rc; - int len; + u32 len; read_lock(&state->ss->policy_rwlock); @@ -2998,10 +2998,10 @@ int security_get_bool_value(struct selinux_state *state, static int security_preserve_bools(struct selinux_state *state, struct policydb *policydb) { - int rc, nbools = 0, *bvalues = NULL, i; + int rc, *bvalues = NULL; char **bnames = NULL; struct cond_bool_datum *booldatum; - struct cond_node *cur; + u32 i, nbools = 0; rc = security_get_bools(state, &nbools, &bnames, &bvalues); if (rc) @@ -3011,8 +3011,8 @@ static int security_preserve_bools(struct selinux_state *state, if (booldatum) booldatum->state = bvalues[i]; } - for (cur = policydb->cond_list; cur; cur = cur->next) - evaluate_cond_node(policydb, cur); + for (i = 0; i < policydb->cond_list_len; i++) + evaluate_cond_node(policydb, &policydb->cond_list[i]); out: if (bnames) { From 2b3a003e1543ab47b2f150abe31df4e7a3f8dde8 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Mon, 3 Feb 2020 12:27:21 +0100 Subject: [PATCH 07/20] selinux: convert cond_av_list to array Since it is fixed-size after allocation and we know the size beforehand, using a plain old array is simpler and more efficient. Signed-off-by: Ondrej Mosnacek Reviewed-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/ss/conditional.c | 124 ++++++++++++------------------ security/selinux/ss/conditional.h | 8 +- 2 files changed, 53 insertions(+), 79 deletions(-) diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index e6d203b76545..82002b90809c 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -87,8 +87,9 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr) */ void evaluate_cond_node(struct policydb *p, struct cond_node *node) { + struct avtab_node *avnode; int new_state; - struct cond_av_list *cur; + u32 i; new_state = cond_evaluate_expr(p, node->expr); if (new_state != node->cur_state) { @@ -96,19 +97,21 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node) if (new_state == -1) pr_err("SELinux: expression result was undefined - disabling all rules.\n"); /* turn the rules on or off */ - for (cur = node->true_list; cur; cur = cur->next) { + for (i = 0; i < node->true_list.len; i++) { + avnode = node->true_list.nodes[i]; if (new_state <= 0) - cur->node->key.specified &= ~AVTAB_ENABLED; + avnode->key.specified &= ~AVTAB_ENABLED; else - cur->node->key.specified |= AVTAB_ENABLED; + avnode->key.specified |= AVTAB_ENABLED; } - for (cur = node->false_list; cur; cur = cur->next) { + for (i = 0; i < node->false_list.len; i++) { + avnode = node->false_list.nodes[i]; /* -1 or 1 */ if (new_state) - cur->node->key.specified &= ~AVTAB_ENABLED; + avnode->key.specified &= ~AVTAB_ENABLED; else - cur->node->key.specified |= AVTAB_ENABLED; + avnode->key.specified |= AVTAB_ENABLED; } } } @@ -128,16 +131,6 @@ int cond_policydb_init(struct policydb *p) return 0; } -static void cond_av_list_destroy(struct cond_av_list *list) -{ - struct cond_av_list *cur, *next; - for (cur = list; cur; cur = next) { - next = cur->next; - /* the avtab_ptr_t node is destroy by the avtab */ - kfree(cur); - } -} - static void cond_node_destroy(struct cond_node *node) { struct cond_expr *cur_expr, *next_expr; @@ -146,8 +139,9 @@ static void cond_node_destroy(struct cond_node *node) next_expr = cur_expr->next; kfree(cur_expr); } - cond_av_list_destroy(node->true_list); - cond_av_list_destroy(node->false_list); + /* the avtab_ptr_t nodes are destroyed by the avtab */ + kfree(node->true_list.nodes); + kfree(node->false_list.nodes); } static void cond_list_destroy(struct policydb *p) @@ -255,19 +249,18 @@ int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp) struct cond_insertf_data { struct policydb *p; + struct avtab_node **dst; struct cond_av_list *other; - struct cond_av_list *head; - struct cond_av_list *tail; }; static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum *d, void *ptr) { struct cond_insertf_data *data = ptr; struct policydb *p = data->p; - struct cond_av_list *other = data->other, *list, *cur; + struct cond_av_list *other = data->other; struct avtab_node *node_ptr; - u8 found; - int rc = -EINVAL; + u32 i; + bool found; /* * For type rules we have to make certain there aren't any @@ -277,7 +270,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum if (k->specified & AVTAB_TYPE) { if (avtab_search(&p->te_avtab, k)) { pr_err("SELinux: type rule already exists outside of a conditional.\n"); - goto err; + return -EINVAL; } /* * If we are reading the false list other will be a pointer to @@ -292,24 +285,24 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum if (node_ptr) { if (avtab_search_node_next(node_ptr, k->specified)) { pr_err("SELinux: too many conflicting type rules.\n"); - goto err; + return -EINVAL; } - found = 0; - for (cur = other; cur; cur = cur->next) { - if (cur->node == node_ptr) { - found = 1; + found = false; + for (i = 0; i < other->len; i++) { + if (other->nodes[i] == node_ptr) { + found = true; break; } } if (!found) { pr_err("SELinux: conflicting type rules.\n"); - goto err; + return -EINVAL; } } } else { if (avtab_search(&p->te_cond_avtab, k)) { pr_err("SELinux: conflicting type rules when adding type rule for true.\n"); - goto err; + return -EINVAL; } } } @@ -317,39 +310,22 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum node_ptr = avtab_insert_nonunique(&p->te_cond_avtab, k, d); if (!node_ptr) { pr_err("SELinux: could not insert rule.\n"); - rc = -ENOMEM; - goto err; + return -ENOMEM; } - list = kzalloc(sizeof(*list), GFP_KERNEL); - if (!list) { - rc = -ENOMEM; - goto err; - } - - list->node = node_ptr; - if (!data->head) - data->head = list; - else - data->tail->next = list; - data->tail = list; + *data->dst = node_ptr; return 0; - -err: - cond_av_list_destroy(data->head); - data->head = NULL; - return rc; } -static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list **ret_list, struct cond_av_list *other) +static int cond_read_av_list(struct policydb *p, void *fp, + struct cond_av_list *list, + struct cond_av_list *other) { - int i, rc; + int rc; __le32 buf[1]; - u32 len; + u32 i, len; struct cond_insertf_data data; - *ret_list = NULL; - rc = next_entry(buf, fp, sizeof(u32)); if (rc) return rc; @@ -358,18 +334,24 @@ static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list * if (len == 0) return 0; + list->nodes = kcalloc(len, sizeof(*list->nodes), GFP_KERNEL); + if (!list->nodes) + return -ENOMEM; + data.p = p; data.other = other; - data.head = NULL; - data.tail = NULL; for (i = 0; i < len; i++) { + data.dst = &list->nodes[i]; rc = avtab_read_item(&p->te_cond_avtab, fp, p, cond_insertf, &data); - if (rc) + if (rc) { + kfree(list->nodes); + list->nodes = NULL; return rc; + } } - *ret_list = data.head; + list->len = len; return 0; } @@ -432,7 +414,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp) rc = cond_read_av_list(p, fp, &node->true_list, NULL); if (rc) goto err; - rc = cond_read_av_list(p, fp, &node->false_list, node->true_list); + rc = cond_read_av_list(p, fp, &node->false_list, &node->true_list); if (rc) goto err; return 0; @@ -511,24 +493,16 @@ static int cond_write_av_list(struct policydb *p, struct cond_av_list *list, struct policy_file *fp) { __le32 buf[1]; - struct cond_av_list *cur_list; - u32 len; + u32 i; int rc; - len = 0; - for (cur_list = list; cur_list != NULL; cur_list = cur_list->next) - len++; - - buf[0] = cpu_to_le32(len); + buf[0] = cpu_to_le32(list->len); rc = put_entry(buf, sizeof(u32), 1, fp); if (rc) return rc; - if (len == 0) - return 0; - - for (cur_list = list; cur_list != NULL; cur_list = cur_list->next) { - rc = avtab_write_item(p, cur_list->node, fp); + for (i = 0; i < list->len; i++) { + rc = avtab_write_item(p, list->nodes[i], fp); if (rc) return rc; } @@ -565,10 +539,10 @@ static int cond_write_node(struct policydb *p, struct cond_node *node, return rc; } - rc = cond_write_av_list(p, node->true_list, fp); + rc = cond_write_av_list(p, &node->true_list, fp); if (rc) return rc; - rc = cond_write_av_list(p, node->false_list, fp); + rc = cond_write_av_list(p, &node->false_list, fp); if (rc) return rc; diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h index e474bdd3a0ed..5f97f678440e 100644 --- a/security/selinux/ss/conditional.h +++ b/security/selinux/ss/conditional.h @@ -39,8 +39,8 @@ struct cond_expr { * struct is for that list. */ struct cond_av_list { - struct avtab_node *node; - struct cond_av_list *next; + struct avtab_node **nodes; + u32 len; }; /* @@ -53,8 +53,8 @@ struct cond_av_list { struct cond_node { int cur_state; struct cond_expr *expr; - struct cond_av_list *true_list; - struct cond_av_list *false_list; + struct cond_av_list true_list; + struct cond_av_list false_list; }; int cond_policydb_init(struct policydb *p); From 8794d7839038fc018e51d0afbf309b71069d9691 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Mon, 3 Feb 2020 12:27:22 +0100 Subject: [PATCH 08/20] selinux: convert cond_expr to array Since it is fixed-size after allocation and we know the size beforehand, using a plain old array is simpler and more efficient. Signed-off-by: Ondrej Mosnacek Reviewed-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/ss/conditional.c | 62 ++++++++++++------------------- security/selinux/ss/conditional.h | 14 ++++--- 2 files changed, 33 insertions(+), 43 deletions(-) diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 82002b90809c..669b766c260b 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -23,18 +23,19 @@ */ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr) { - - struct cond_expr *cur; + u32 i; int s[COND_EXPR_MAXDEPTH]; int sp = -1; - for (cur = expr; cur; cur = cur->next) { - switch (cur->expr_type) { + for (i = 0; i < expr->len; i++) { + struct cond_expr_node *node = &expr->nodes[i]; + + switch (node->expr_type) { case COND_BOOL: if (sp == (COND_EXPR_MAXDEPTH - 1)) return -1; sp++; - s[sp] = p->bool_val_to_struct[cur->bool - 1]->state; + s[sp] = p->bool_val_to_struct[node->bool - 1]->state; break; case COND_NOT: if (sp < 0) @@ -91,7 +92,7 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node) int new_state; u32 i; - new_state = cond_evaluate_expr(p, node->expr); + new_state = cond_evaluate_expr(p, &node->expr); if (new_state != node->cur_state) { node->cur_state = new_state; if (new_state == -1) @@ -133,12 +134,7 @@ int cond_policydb_init(struct policydb *p) static void cond_node_destroy(struct cond_node *node) { - struct cond_expr *cur_expr, *next_expr; - - for (cur_expr = node->expr; cur_expr; cur_expr = next_expr) { - next_expr = cur_expr->next; - kfree(cur_expr); - } + kfree(node->expr.nodes); /* the avtab_ptr_t nodes are destroyed by the avtab */ kfree(node->true_list.nodes); kfree(node->false_list.nodes); @@ -355,7 +351,7 @@ static int cond_read_av_list(struct policydb *p, void *fp, return 0; } -static int expr_isvalid(struct policydb *p, struct cond_expr *expr) +static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr) { if (expr->expr_type <= 0 || expr->expr_type > COND_LAST) { pr_err("SELinux: conditional expressions uses unknown operator.\n"); @@ -372,43 +368,37 @@ static int expr_isvalid(struct policydb *p, struct cond_expr *expr) static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp) { __le32 buf[2]; - u32 len, i; + u32 i, len; int rc; - struct cond_expr *expr = NULL, *last = NULL; rc = next_entry(buf, fp, sizeof(u32) * 2); if (rc) - goto err; + return rc; node->cur_state = le32_to_cpu(buf[0]); /* expr */ len = le32_to_cpu(buf[1]); + node->expr.nodes = kcalloc(len, sizeof(*node->expr.nodes), GFP_KERNEL); + if (!node->expr.nodes) + return -ENOMEM; + + node->expr.len = len; for (i = 0; i < len; i++) { + struct cond_expr_node *expr = &node->expr.nodes[i]; + rc = next_entry(buf, fp, sizeof(u32) * 2); if (rc) goto err; - rc = -ENOMEM; - expr = kzalloc(sizeof(*expr), GFP_KERNEL); - if (!expr) - goto err; - expr->expr_type = le32_to_cpu(buf[0]); expr->bool = le32_to_cpu(buf[1]); - if (!expr_isvalid(p, expr)) { + if (!expr_node_isvalid(p, expr)) { rc = -EINVAL; - kfree(expr); goto err; } - - if (i == 0) - node->expr = expr; - else - last->next = expr; - last = expr; } rc = cond_read_av_list(p, fp, &node->true_list, NULL); @@ -513,27 +503,23 @@ static int cond_write_av_list(struct policydb *p, static int cond_write_node(struct policydb *p, struct cond_node *node, struct policy_file *fp) { - struct cond_expr *cur_expr; __le32 buf[2]; int rc; - u32 len = 0; + u32 i; buf[0] = cpu_to_le32(node->cur_state); rc = put_entry(buf, sizeof(u32), 1, fp); if (rc) return rc; - for (cur_expr = node->expr; cur_expr != NULL; cur_expr = cur_expr->next) - len++; - - buf[0] = cpu_to_le32(len); + buf[0] = cpu_to_le32(node->expr.len); rc = put_entry(buf, sizeof(u32), 1, fp); if (rc) return rc; - for (cur_expr = node->expr; cur_expr != NULL; cur_expr = cur_expr->next) { - buf[0] = cpu_to_le32(cur_expr->expr_type); - buf[1] = cpu_to_le32(cur_expr->bool); + for (i = 0; i < node->expr.len; i++) { + buf[0] = cpu_to_le32(node->expr.nodes[i].expr_type); + buf[1] = cpu_to_le32(node->expr.nodes[i].bool); rc = put_entry(buf, sizeof(u32), 2, fp); if (rc) return rc; diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h index 5f97f678440e..4677c6ff7450 100644 --- a/security/selinux/ss/conditional.h +++ b/security/selinux/ss/conditional.h @@ -19,7 +19,7 @@ * A conditional expression is a list of operators and operands * in reverse polish notation. */ -struct cond_expr { +struct cond_expr_node { #define COND_BOOL 1 /* plain bool */ #define COND_NOT 2 /* !bool */ #define COND_OR 3 /* bool || bool */ @@ -28,9 +28,13 @@ struct cond_expr { #define COND_EQ 6 /* bool == bool */ #define COND_NEQ 7 /* bool != bool */ #define COND_LAST COND_NEQ - __u32 expr_type; - __u32 bool; - struct cond_expr *next; + u32 expr_type; + u32 bool; +}; + +struct cond_expr { + struct cond_expr_node *nodes; + u32 len; }; /* @@ -52,7 +56,7 @@ struct cond_av_list { */ struct cond_node { int cur_state; - struct cond_expr *expr; + struct cond_expr expr; struct cond_av_list true_list; struct cond_av_list false_list; }; From 89d4d7c88d2b4f252adb434a28ea9b84d629aeb1 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Mon, 3 Feb 2020 12:27:23 +0100 Subject: [PATCH 09/20] selinux: generalize evaluate_cond_node() Both callers iterate the cond_list and call it for each node - turn it into evaluate_cond_nodes(), which does the iteration for them. Signed-off-by: Ondrej Mosnacek Signed-off-by: Paul Moore --- security/selinux/ss/conditional.c | 10 +++++++++- security/selinux/ss/conditional.h | 2 +- security/selinux/ss/services.c | 6 ++---- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 669b766c260b..cce4a75fb3e7 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -86,7 +86,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr) * list appropriately. If the result of the expression is undefined * all of the rules are disabled for safety. */ -void evaluate_cond_node(struct policydb *p, struct cond_node *node) +static void evaluate_cond_node(struct policydb *p, struct cond_node *node) { struct avtab_node *avnode; int new_state; @@ -117,6 +117,14 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node) } } +void evaluate_cond_nodes(struct policydb *p) +{ + u32 i; + + for (i = 0; i < p->cond_list_len; i++) + evaluate_cond_node(p, &p->cond_list[i]); +} + int cond_policydb_init(struct policydb *p) { int rc; diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h index 4677c6ff7450..b9eb888ffa76 100644 --- a/security/selinux/ss/conditional.h +++ b/security/selinux/ss/conditional.h @@ -78,6 +78,6 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key, struct av_decision *avd, struct extended_perms *xperms); void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key, struct extended_perms_decision *xpermd); -void evaluate_cond_node(struct policydb *p, struct cond_node *node); +void evaluate_cond_nodes(struct policydb *p); #endif /* _CONDITIONAL_H_ */ diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 1e652d6ed8cd..57b2c6252799 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2957,8 +2957,7 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values) policydb->bool_val_to_struct[i]->state = 0; } - for (i = 0; i < policydb->cond_list_len; i++) - evaluate_cond_node(policydb, &policydb->cond_list[i]); + evaluate_cond_nodes(policydb); seqno = ++state->ss->latest_granting; rc = 0; @@ -3011,8 +3010,7 @@ static int security_preserve_bools(struct selinux_state *state, if (booldatum) booldatum->state = bvalues[i]; } - for (i = 0; i < policydb->cond_list_len; i++) - evaluate_cond_node(policydb, &policydb->cond_list[i]); + evaluate_cond_nodes(policydb); out: if (bnames) { From 4ca54d3d3022ce27170b50e4bdecc3a42f05dbdc Mon Sep 17 00:00:00 2001 From: Connor O'Brien Date: Fri, 7 Feb 2020 10:01:49 -0800 Subject: [PATCH 10/20] security: selinux: allow per-file labeling for bpffs Add support for genfscon per-file labeling of bpffs files. This allows for separate permissions for different pinned bpf objects, which may be completely unrelated to each other. Signed-off-by: Connor O'Brien Signed-off-by: Steven Moreland Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/hooks.c | 1 + 1 file changed, 1 insertion(+) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 7c37cdb3aba0..44f6f4e20cba 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -702,6 +702,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, if (!strcmp(sb->s_type->name, "debugfs") || !strcmp(sb->s_type->name, "tracefs") || !strcmp(sb->s_type->name, "binderfs") || + !strcmp(sb->s_type->name, "bpf") || !strcmp(sb->s_type->name, "pstore")) sbsec->flags |= SE_SBGENFS; From 253050f57c7afe87d182f4029645568c2fc837f7 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Wed, 12 Feb 2020 12:22:54 +0100 Subject: [PATCH 11/20] selinux: factor out loop body from filename_trans_read() It simplifies cleanup in the error path. This will be extra useful in later patch. Signed-off-by: Ondrej Mosnacek Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/ss/policydb.c | 128 +++++++++++++++++---------------- 1 file changed, 66 insertions(+), 62 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 8ac9b9ffc83c..4b4efd3c0b3c 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1880,13 +1880,73 @@ static int range_read(struct policydb *p, void *fp) return rc; } -static int filename_trans_read(struct policydb *p, void *fp) +static int filename_trans_read_one(struct policydb *p, void *fp) { struct filename_trans *ft; - struct filename_trans_datum *otype; - char *name; - u32 nel, len; + struct filename_trans_datum *otype = NULL; + char *name = NULL; + u32 len; __le32 buf[4]; + int rc; + + ft = kzalloc(sizeof(*ft), GFP_KERNEL); + if (!ft) + return -ENOMEM; + + rc = -ENOMEM; + otype = kmalloc(sizeof(*otype), GFP_KERNEL); + if (!otype) + goto out; + + /* length of the path component string */ + rc = next_entry(buf, fp, sizeof(u32)); + if (rc) + goto out; + len = le32_to_cpu(buf[0]); + + /* path component string */ + rc = str_read(&name, GFP_KERNEL, fp, len); + if (rc) + goto out; + + ft->name = name; + + rc = next_entry(buf, fp, sizeof(u32) * 4); + if (rc) + goto out; + + ft->stype = le32_to_cpu(buf[0]); + ft->ttype = le32_to_cpu(buf[1]); + ft->tclass = le32_to_cpu(buf[2]); + + otype->otype = le32_to_cpu(buf[3]); + + rc = ebitmap_set_bit(&p->filename_trans_ttypes, ft->ttype, 1); + if (rc) + goto out; + + rc = hashtab_insert(p->filename_trans, ft, otype); + if (rc) { + /* + * Do not return -EEXIST to the caller, or the system + * will not boot. + */ + if (rc == -EEXIST) + rc = 0; + goto out; + } + return 0; +out: + kfree(ft); + kfree(name); + kfree(otype); + return rc; +} + +static int filename_trans_read(struct policydb *p, void *fp) +{ + u32 nel; + __le32 buf[1]; int rc, i; if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS) @@ -1898,68 +1958,12 @@ static int filename_trans_read(struct policydb *p, void *fp) nel = le32_to_cpu(buf[0]); for (i = 0; i < nel; i++) { - otype = NULL; - name = NULL; - - rc = -ENOMEM; - ft = kzalloc(sizeof(*ft), GFP_KERNEL); - if (!ft) - goto out; - - rc = -ENOMEM; - otype = kmalloc(sizeof(*otype), GFP_KERNEL); - if (!otype) - goto out; - - /* length of the path component string */ - rc = next_entry(buf, fp, sizeof(u32)); + rc = filename_trans_read_one(p, fp); if (rc) - goto out; - len = le32_to_cpu(buf[0]); - - /* path component string */ - rc = str_read(&name, GFP_KERNEL, fp, len); - if (rc) - goto out; - - ft->name = name; - - rc = next_entry(buf, fp, sizeof(u32) * 4); - if (rc) - goto out; - - ft->stype = le32_to_cpu(buf[0]); - ft->ttype = le32_to_cpu(buf[1]); - ft->tclass = le32_to_cpu(buf[2]); - - otype->otype = le32_to_cpu(buf[3]); - - rc = ebitmap_set_bit(&p->filename_trans_ttypes, ft->ttype, 1); - if (rc) - goto out; - - rc = hashtab_insert(p->filename_trans, ft, otype); - if (rc) { - /* - * Do not return -EEXIST to the caller, or the system - * will not boot. - */ - if (rc != -EEXIST) - goto out; - /* But free memory to avoid memory leak. */ - kfree(ft); - kfree(name); - kfree(otype); - } + return rc; } hash_eval(p->filename_trans, "filenametr"); return 0; -out: - kfree(ft); - kfree(name); - kfree(otype); - - return rc; } static int genfs_read(struct policydb *p, void *fp) From c3a276111ea2572399281988b3129683e2a6b60b Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Tue, 18 Feb 2020 12:27:34 +0100 Subject: [PATCH 12/20] selinux: optimize storage of filename transitions In these rules, each rule with the same (target type, target class, filename) values is (in practice) always mapped to the same result type. Therefore, it is much more efficient to group the rules by (ttype, tclass, filename). Thus, this patch drops the stype field from the key and changes the datum to be a linked list of one or more structures that contain a result type and an ebitmap of source types that map the given target to the given result type under the given filename. The size of the hash table is also incremented to 2048 to be more optimal for Fedora policy (which currently has ~2500 unique (ttype, tclass, filename) tuples, regardless of whether the 'unconfined' module is enabled). Not only does this dramtically reduce memory usage when the policy contains a lot of unconfined domains (ergo a lot of filename based transitions), but it also slightly reduces memory usage of strongly confined policies (modeled on Fedora policy with 'unconfined' module disabled) and significantly reduces lookup times of these rules on Fedora (roughly matches the performance of the rhashtable conversion patch [1] posted recently to selinux@vger.kernel.org). An obvious next step is to change binary policy format to match this layout, so that disk space is also saved. However, since that requires more work (including matching userspace changes) and this patch is already beneficial on its own, I'm posting it separately. Performance/memory usage comparison: Kernel | Policy load | Policy load | Mem usage | Mem usage | openbench | | (-unconfined) | | (-unconfined) | (createfiles) -----------------|-------------|---------------|-----------|---------------|-------------- reference | 1,30s | 0,91s | 90MB | 77MB | 55 us/file rhashtable patch | 0.98s | 0,85s | 85MB | 75MB | 38 us/file this patch | 0,95s | 0,87s | 75MB | 75MB | 40 us/file (Memory usage is measured after boot. With SELinux disabled the memory usage was ~60MB on the same system.) [1] https://lore.kernel.org/selinux/20200116213937.77795-1-dev@lynxeye.de/T/ Signed-off-by: Ondrej Mosnacek Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/ss/policydb.c | 168 +++++++++++++++++++-------------- security/selinux/ss/policydb.h | 8 +- security/selinux/ss/services.c | 16 ++-- 3 files changed, 111 insertions(+), 81 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 4b4efd3c0b3c..32b3a8acf96f 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -336,11 +336,17 @@ static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap) = static int filenametr_destroy(void *key, void *datum, void *p) { - struct filename_trans *ft = key; + struct filename_trans_key *ft = key; + struct filename_trans_datum *next, *d = datum; kfree(ft->name); kfree(key); - kfree(datum); + do { + ebitmap_destroy(&d->stypes); + next = d->next; + kfree(d); + d = next; + } while (unlikely(d)); cond_resched(); return 0; } @@ -406,12 +412,12 @@ static int roles_init(struct policydb *p) static u32 filenametr_hash(struct hashtab *h, const void *k) { - const struct filename_trans *ft = k; + const struct filename_trans_key *ft = k; unsigned long hash; unsigned int byte_num; unsigned char focus; - hash = ft->stype ^ ft->ttype ^ ft->tclass; + hash = ft->ttype ^ ft->tclass; byte_num = 0; while ((focus = ft->name[byte_num++])) @@ -421,14 +427,10 @@ static u32 filenametr_hash(struct hashtab *h, const void *k) static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2) { - const struct filename_trans *ft1 = k1; - const struct filename_trans *ft2 = k2; + const struct filename_trans_key *ft1 = k1; + const struct filename_trans_key *ft2 = k2; int v; - v = ft1->stype - ft2->stype; - if (v) - return v; - v = ft1->ttype - ft2->ttype; if (v) return v; @@ -495,7 +497,7 @@ static int policydb_init(struct policydb *p) goto out; p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp, - (1 << 10)); + (1 << 11)); if (!p->filename_trans) { rc = -ENOMEM; goto out; @@ -1882,64 +1884,84 @@ static int range_read(struct policydb *p, void *fp) static int filename_trans_read_one(struct policydb *p, void *fp) { - struct filename_trans *ft; - struct filename_trans_datum *otype = NULL; + struct filename_trans_key key, *ft = NULL; + struct filename_trans_datum *last, *datum = NULL; char *name = NULL; - u32 len; + u32 len, stype, otype; __le32 buf[4]; int rc; - ft = kzalloc(sizeof(*ft), GFP_KERNEL); - if (!ft) - return -ENOMEM; - - rc = -ENOMEM; - otype = kmalloc(sizeof(*otype), GFP_KERNEL); - if (!otype) - goto out; - /* length of the path component string */ rc = next_entry(buf, fp, sizeof(u32)); if (rc) - goto out; + return rc; len = le32_to_cpu(buf[0]); /* path component string */ rc = str_read(&name, GFP_KERNEL, fp, len); if (rc) - goto out; - - ft->name = name; + return rc; rc = next_entry(buf, fp, sizeof(u32) * 4); if (rc) goto out; - ft->stype = le32_to_cpu(buf[0]); - ft->ttype = le32_to_cpu(buf[1]); - ft->tclass = le32_to_cpu(buf[2]); + stype = le32_to_cpu(buf[0]); + key.ttype = le32_to_cpu(buf[1]); + key.tclass = le32_to_cpu(buf[2]); + key.name = name; - otype->otype = le32_to_cpu(buf[3]); + otype = le32_to_cpu(buf[3]); - rc = ebitmap_set_bit(&p->filename_trans_ttypes, ft->ttype, 1); - if (rc) - goto out; - - rc = hashtab_insert(p->filename_trans, ft, otype); - if (rc) { - /* - * Do not return -EEXIST to the caller, or the system - * will not boot. - */ - if (rc == -EEXIST) - rc = 0; - goto out; + last = NULL; + datum = hashtab_search(p->filename_trans, &key); + while (datum) { + if (unlikely(ebitmap_get_bit(&datum->stypes, stype - 1))) { + /* conflicting/duplicate rules are ignored */ + datum = NULL; + goto out; + } + if (likely(datum->otype == otype)) + break; + last = datum; + datum = datum->next; } - return 0; + if (!datum) { + rc = -ENOMEM; + datum = kmalloc(sizeof(*datum), GFP_KERNEL); + if (!datum) + goto out; + + ebitmap_init(&datum->stypes); + datum->otype = otype; + datum->next = NULL; + + if (unlikely(last)) { + last->next = datum; + } else { + rc = -ENOMEM; + ft = kmemdup(&key, sizeof(key), GFP_KERNEL); + if (!ft) + goto out; + + rc = hashtab_insert(p->filename_trans, ft, datum); + if (rc) + goto out; + name = NULL; + + rc = ebitmap_set_bit(&p->filename_trans_ttypes, + key.ttype, 1); + if (rc) + return rc; + } + } + kfree(name); + return ebitmap_set_bit(&datum->stypes, stype - 1, 1); + out: kfree(ft); kfree(name); - kfree(otype); + kfree(datum); return rc; } @@ -1957,6 +1979,8 @@ static int filename_trans_read(struct policydb *p, void *fp) return rc; nel = le32_to_cpu(buf[0]); + p->filename_trans_count = nel; + for (i = 0; i < nel; i++) { rc = filename_trans_read_one(p, fp); if (rc) @@ -3334,50 +3358,50 @@ static int range_write(struct policydb *p, void *fp) static int filename_write_helper(void *key, void *data, void *ptr) { - __le32 buf[4]; - struct filename_trans *ft = key; - struct filename_trans_datum *otype = data; + struct filename_trans_key *ft = key; + struct filename_trans_datum *datum = data; + struct ebitmap_node *node; void *fp = ptr; + __le32 buf[4]; int rc; - u32 len; + u32 bit, len = strlen(ft->name); - len = strlen(ft->name); - buf[0] = cpu_to_le32(len); - rc = put_entry(buf, sizeof(u32), 1, fp); - if (rc) - return rc; + do { + ebitmap_for_each_positive_bit(&datum->stypes, node, bit) { + buf[0] = cpu_to_le32(len); + rc = put_entry(buf, sizeof(u32), 1, fp); + if (rc) + return rc; - rc = put_entry(ft->name, sizeof(char), len, fp); - if (rc) - return rc; + rc = put_entry(ft->name, sizeof(char), len, fp); + if (rc) + return rc; - buf[0] = cpu_to_le32(ft->stype); - buf[1] = cpu_to_le32(ft->ttype); - buf[2] = cpu_to_le32(ft->tclass); - buf[3] = cpu_to_le32(otype->otype); + buf[0] = cpu_to_le32(bit + 1); + buf[1] = cpu_to_le32(ft->ttype); + buf[2] = cpu_to_le32(ft->tclass); + buf[3] = cpu_to_le32(datum->otype); - rc = put_entry(buf, sizeof(u32), 4, fp); - if (rc) - return rc; + rc = put_entry(buf, sizeof(u32), 4, fp); + if (rc) + return rc; + } + + datum = datum->next; + } while (unlikely(datum)); return 0; } static int filename_trans_write(struct policydb *p, void *fp) { - u32 nel; __le32 buf[1]; int rc; if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS) return 0; - nel = 0; - rc = hashtab_map(p->filename_trans, hashtab_cnt, &nel); - if (rc) - return rc; - - buf[0] = cpu_to_le32(nel); + buf[0] = cpu_to_le32(p->filename_trans_count); rc = put_entry(buf, sizeof(u32), 1, fp); if (rc) return rc; diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 6459616f8487..41ad78a1f17b 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -89,15 +89,16 @@ struct role_trans { struct role_trans *next; }; -struct filename_trans { - u32 stype; /* current process */ +struct filename_trans_key { u32 ttype; /* parent dir context */ u16 tclass; /* class of new object */ const char *name; /* last path component */ }; struct filename_trans_datum { - u32 otype; /* expected of new object */ + struct ebitmap stypes; /* bitmap of source types for this otype */ + u32 otype; /* resulting type of new object */ + struct filename_trans_datum *next; /* record for next otype*/ }; struct role_allow { @@ -267,6 +268,7 @@ struct policydb { struct ebitmap filename_trans_ttypes; /* actual set of filename_trans rules */ struct hashtab *filename_trans; + u32 filename_trans_count; /* bools indexed by (value - 1) */ struct cond_bool_datum **bool_val_to_struct; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 57b2c6252799..f90e6550eec8 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -1692,8 +1692,8 @@ static void filename_compute_type(struct policydb *policydb, u32 stype, u32 ttype, u16 tclass, const char *objname) { - struct filename_trans ft; - struct filename_trans_datum *otype; + struct filename_trans_key ft; + struct filename_trans_datum *datum; /* * Most filename trans rules are going to live in specific directories @@ -1703,14 +1703,18 @@ static void filename_compute_type(struct policydb *policydb, if (!ebitmap_get_bit(&policydb->filename_trans_ttypes, ttype)) return; - ft.stype = stype; ft.ttype = ttype; ft.tclass = tclass; ft.name = objname; - otype = hashtab_search(policydb->filename_trans, &ft); - if (otype) - newcontext->type = otype->otype; + datum = hashtab_search(policydb->filename_trans, &ft); + while (datum) { + if (ebitmap_get_bit(&datum->stypes, stype - 1)) { + newcontext->type = datum->otype; + return; + } + datum = datum->next; + } } static int security_compute_sid(struct selinux_state *state, From e4cfa05e9bfe286457082477b32ecd17737bdbce Mon Sep 17 00:00:00 2001 From: Richard Haines Date: Thu, 20 Feb 2020 15:32:34 +0000 Subject: [PATCH 13/20] selinux: Add xfs quota command types Add Q_XQUOTAOFF, Q_XQUOTAON and Q_XSETQLIM to trigger filesystem quotamod permission check. Add Q_XGETQUOTA, Q_XGETQSTAT, Q_XGETQSTATV and Q_XGETNEXTQUOTA to trigger filesystem quotaget permission check. Signed-off-by: Richard Haines Reviewed-by: Christoph Hellwig Signed-off-by: Paul Moore --- security/selinux/hooks.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 44f6f4e20cba..b8e09aedbc56 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2145,11 +2145,18 @@ static int selinux_quotactl(int cmds, int type, int id, struct super_block *sb) case Q_QUOTAOFF: case Q_SETINFO: case Q_SETQUOTA: + case Q_XQUOTAOFF: + case Q_XQUOTAON: + case Q_XSETQLIM: rc = superblock_has_perm(cred, sb, FILESYSTEM__QUOTAMOD, NULL); break; case Q_GETFMT: case Q_GETINFO: case Q_GETQUOTA: + case Q_XGETQUOTA: + case Q_XGETQSTAT: + case Q_XGETQSTATV: + case Q_XGETNEXTQUOTA: rc = superblock_has_perm(cred, sb, FILESYSTEM__QUOTAGET, NULL); break; default: From e0ac568de1fa0a38bea6d3c69a894d913a5ca59d Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Wed, 26 Feb 2020 16:54:52 +0100 Subject: [PATCH 14/20] selinux: reduce the use of hard-coded hash sizes Instead allocate hash tables with just the right size based on the actual number of elements (which is almost always known beforehand, we just need to defer the hashtab allocation to the right time). The only case when we don't know the size (with the current policy format) is the new filename transitions hashtable. Here I just left the existing value. After this patch, the time to load Fedora policy on x86_64 decreases from 790 ms to 167 ms. If the unconfined module is removed, it decreases from 750 ms to 122 ms. It is also likely that other operations are going to be faster, mainly string_to_context_struct() or mls_compute_sid(), but I didn't try to quantify that. The memory usage of all hash table arrays increases from ~58 KB to ~163 KB (with Fedora policy on x86_64). Signed-off-by: Ondrej Mosnacek Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/ss/hashtab.c | 28 +++++++++++++++--- security/selinux/ss/hashtab.h | 2 +- security/selinux/ss/policydb.c | 53 +++++++++++++--------------------- security/selinux/ss/policydb.h | 2 -- 4 files changed, 45 insertions(+), 40 deletions(-) diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c index ebfdaa31ee32..883f19d32c28 100644 --- a/security/selinux/ss/hashtab.c +++ b/security/selinux/ss/hashtab.c @@ -12,12 +12,29 @@ static struct kmem_cache *hashtab_node_cachep; +/* + * Here we simply round the number of elements up to the nearest power of two. + * I tried also other options like rouding down or rounding to the closest + * power of two (up or down based on which is closer), but I was unable to + * find any significant difference in lookup/insert performance that would + * justify switching to a different (less intuitive) formula. It could be that + * a different formula is actually more optimal, but any future changes here + * should be supported with performance/memory usage data. + * + * The total memory used by the htable arrays (only) with Fedora policy loaded + * is approximately 163 KB at the time of writing. + */ +static u32 hashtab_compute_size(u32 nel) +{ + return nel == 0 ? 0 : roundup_pow_of_two(nel); +} + 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) + u32 nel_hint) { struct hashtab *p; - u32 i; + u32 i, size = hashtab_compute_size(nel_hint); p = kzalloc(sizeof(*p), GFP_KERNEL); if (!p) @@ -27,6 +44,9 @@ struct hashtab *hashtab_create(u32 (*hash_value)(struct hashtab *h, const void * p->nel = 0; p->hash_value = hash_value; p->keycmp = keycmp; + if (!size) + return p; + p->htable = kmalloc_array(size, sizeof(*p->htable), GFP_KERNEL); if (!p->htable) { kfree(p); @@ -46,7 +66,7 @@ int hashtab_insert(struct hashtab *h, void *key, void *datum) cond_resched(); - if (!h || h->nel == HASHTAB_MAX_NODES) + if (!h || !h->size || h->nel == HASHTAB_MAX_NODES) return -EINVAL; hvalue = h->hash_value(h, key); @@ -82,7 +102,7 @@ void *hashtab_search(struct hashtab *h, const void *key) u32 hvalue; struct hashtab_node *cur; - if (!h) + if (!h || !h->size) return NULL; hvalue = h->hash_value(h, key); diff --git a/security/selinux/ss/hashtab.h b/security/selinux/ss/hashtab.h index 3e3e42bfd150..dde54d9ff01c 100644 --- a/security/selinux/ss/hashtab.h +++ b/security/selinux/ss/hashtab.h @@ -42,7 +42,7 @@ struct hashtab_info { */ 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); + u32 nel_hint); /* * Inserts the specified (key, datum) pair into the specified hash table. diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 32b3a8acf96f..7ca8c74efba3 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -56,17 +56,6 @@ static const char *symtab_name[SYM_NUM] = { }; #endif -static unsigned int symtab_sizes[SYM_NUM] = { - 2, - 32, - 16, - 512, - 128, - 16, - 16, - 16, -}; - struct policydb_compat_info { int version; int sym_num; @@ -478,20 +467,10 @@ static int policydb_init(struct policydb *p) memset(p, 0, sizeof(*p)); - for (i = 0; i < SYM_NUM; i++) { - rc = symtab_init(&p->symtab[i], symtab_sizes[i]); - if (rc) - goto out; - } - rc = avtab_init(&p->te_avtab); if (rc) goto out; - rc = roles_init(p); - if (rc) - goto out; - rc = cond_policydb_init(p); if (rc) goto out; @@ -503,20 +482,12 @@ static int policydb_init(struct policydb *p) goto out; } - p->range_tr = hashtab_create(rangetr_hash, rangetr_cmp, 256); - if (!p->range_tr) { - rc = -ENOMEM; - goto out; - } - ebitmap_init(&p->filename_trans_ttypes); ebitmap_init(&p->policycaps); ebitmap_init(&p->permissive_map); return 0; out: - hashtab_destroy(p->filename_trans); - hashtab_destroy(p->range_tr); for (i = 0; i < SYM_NUM; i++) { hashtab_map(p->symtab[i].table, destroy_f[i], NULL); hashtab_destroy(p->symtab[i].table); @@ -1142,12 +1113,12 @@ static int common_read(struct policydb *p, struct hashtab *h, void *fp) len = le32_to_cpu(buf[0]); comdatum->value = le32_to_cpu(buf[1]); + nel = le32_to_cpu(buf[3]); - rc = symtab_init(&comdatum->permissions, PERM_SYMTAB_SIZE); + rc = symtab_init(&comdatum->permissions, nel); if (rc) goto bad; comdatum->permissions.nprim = le32_to_cpu(buf[2]); - nel = le32_to_cpu(buf[3]); rc = str_read(&key, GFP_KERNEL, fp, len); if (rc) @@ -1308,12 +1279,12 @@ static int class_read(struct policydb *p, struct hashtab *h, void *fp) len = le32_to_cpu(buf[0]); len2 = le32_to_cpu(buf[1]); cladatum->value = le32_to_cpu(buf[2]); + nel = le32_to_cpu(buf[4]); - rc = symtab_init(&cladatum->permissions, PERM_SYMTAB_SIZE); + rc = symtab_init(&cladatum->permissions, nel); if (rc) goto bad; cladatum->permissions.nprim = le32_to_cpu(buf[3]); - nel = le32_to_cpu(buf[4]); ncons = le32_to_cpu(buf[5]); @@ -1826,6 +1797,11 @@ static int range_read(struct policydb *p, void *fp) return rc; nel = le32_to_cpu(buf[0]); + + p->range_tr = hashtab_create(rangetr_hash, rangetr_cmp, nel); + if (!p->range_tr) + return -ENOMEM; + for (i = 0; i < nel; i++) { rc = -ENOMEM; rt = kzalloc(sizeof(*rt), GFP_KERNEL); @@ -2418,6 +2394,17 @@ int policydb_read(struct policydb *p, void *fp) goto bad; nprim = le32_to_cpu(buf[0]); nel = le32_to_cpu(buf[1]); + + rc = symtab_init(&p->symtab[i], nel); + if (rc) + goto out; + + if (i == SYM_ROLES) { + rc = roles_init(p); + if (rc) + goto out; + } + for (j = 0; j < nel; j++) { rc = read_f[i](p, p->symtab[i].table, fp); if (rc) diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 41ad78a1f17b..72e2932fb12d 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -321,8 +321,6 @@ extern int policydb_role_isvalid(struct policydb *p, unsigned int role); extern int policydb_read(struct policydb *p, void *fp); extern int policydb_write(struct policydb *p, void *fp); -#define PERM_SYMTAB_SIZE 32 - #define POLICYDB_CONFIG_MLS 1 /* the config flags related to unknown classes/perms are bits 2 and 3 */ From e3e0b582c321aefd72db0e7083a0adfe285e96b5 Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Mon, 24 Feb 2020 11:10:23 -0500 Subject: [PATCH 15/20] selinux: remove unused initial SIDs and improve handling Remove initial SIDs that have never been used or are no longer used by the kernel from its string table, which is also used to generate the SECINITSID_* symbols referenced in code. Update the code to gracefully handle the fact that these can now be NULL. Stop treating it as an error if a policy defines additional initial SIDs unknown to the kernel. Do not load unused initial SID contexts into the sidtab. Fix the incorrect usage of the name from the ocontext in error messages when loading initial SIDs since these are not presently written to the kernel policy and are therefore always NULL. After this change, it is possible to safely reclaim and reuse some of the unused initial SIDs without compatibility issues. Specifically, unused initial SIDs that were being assigned the same context as the unlabeled initial SID in policies can be reclaimed and reused for another purpose, with existing policies still treating them as having the unlabeled context and future policies having the option of mapping them to a more specific context. For example, this could have been used when the infiniband labeling support was introduced to define initial SIDs for the default pkey and endport SIDs similar to the handling of port/netif/node SIDs rather than always using SECINITSID_UNLABELED as the default. The set of safely reclaimable unused initial SIDs across all known policies is igmp_packet (13), icmp_socket (14), tcp_socket (15), kmod (24), policy (25), and scmp_packet (26); these initial SIDs were assigned the same context as unlabeled in all known policies including mls. If only considering non-mls policies (i.e. assuming that mls users always upgrade policy with their kernels), the set of safely reclaimable unused initial SIDs further includes file_labels (6), init (7), sysctl_modprobe (16), and sysctl_fs (18) through sysctl_dev (23). Adding new initial SIDs beyond SECINITSID_NUM to policy unfortunately became a fatal error in commit 24ed7fdae669 ("selinux: use separate table for initial SID lookup") and even before that it could cause problems on a policy reload (collision between the new initial SID and one allocated at runtime) ever since commit 42596eafdd75 ("selinux: load the initial SIDs upon every policy load") so we cannot safely start adding new initial SIDs to policies beyond SECINITSID_NUM (27) until such a time as all such kernels do not need to be supported and only those that include this commit are relevant. That is not a big deal since we haven't added a new initial SID since 2004 (v2.6.7) and we have plenty of unused ones we can reclaim if we truly need one. If we want to avoid the wasted storage in initial_sid_to_string[] and/or sidtab->isids[] for the unused initial SIDs, we could introduce an indirection between the kernel initial SID values and the policy initial SID values and just map the policy SID values in the ocontexts to the kernel values during policy_load_isids(). Originally I thought we'd do this by preserving the initial SID names in the kernel policy and creating a mapping at load time like we do for the security classes and permissions but that would require a new kernel policy format version and associated changes to libsepol/checkpolicy and I'm not sure it is justified. Simpler approach is just to create a fixed mapping table in the kernel from the existing fixed policy values to the kernel values. Less flexible but probably sufficient. A separate selinux userspace change was applied in https://github.com/SELinuxProject/selinux/commit/8677ce5e8f592950ae6f14cea1b68a20ddc1ac25 to enable removal of most of the unused initial SID contexts from policies, but there is no dependency between that change and this one. That change permits removing all of the unused initial SID contexts from policy except for the fs and sysctl SID contexts. The initial SID declarations themselves would remain in policy to preserve the values of subsequent ones but the contexts can be dropped. If/when the kernel decides to reuse one of them, future policies can change the name and start assigning a context again without breaking compatibility. Here is how I would envision staging changes to the initial SIDs in a compatible manner after this commit is applied: 1. At any time after this commit is applied, the kernel could choose to reclaim one of the safely reclaimable unused initial SIDs listed above for a new purpose (i.e. replace its NULL entry in the initial_sid_to_string[] table with a new name and start using the newly generated SECINITSID_name symbol in code), and refpolicy could at that time rename its declaration of that initial SID to reflect its new purpose and start assigning it a context going forward. Existing/old policies would map the reclaimed initial SID to the unlabeled context, so that would be the initial default behavior until policies are updated. This doesn't depend on the selinux userspace change; it will work with existing policies and userspace. 2. In 6 months or so we'll have another SELinux userspace release that will include the libsepol/checkpolicy support for omitting unused initial SID contexts. 3. At any time after that release, refpolicy can make that release its minimum build requirement and drop the sid context statements (but not the sid declarations) for all of the unused initial SIDs except for fs and sysctl, which must remain for compatibility on policy reload with old kernels and for compatibility with kernels that were still using SECINITSID_SYSCTL (< 2.6.39). This doesn't depend on this kernel commit; it will work with previous kernels as well. 4. After N years for some value of N, refpolicy decides that it no longer cares about policy reload compatibility for kernels that predate this kernel commit, and refpolicy drops the fs and sysctl SID contexts from policy too (but retains the declarations). 5. After M years for some value of M, the kernel decides that it no longer cares about compatibility with refpolicies that predate step 4 (dropping the fs and sysctl SIDs), and those two SIDs also become safely reclaimable. This step is optional and need not ever occur unless we decide that the need to reclaim those two SIDs outweighs the compatibility cost. 6. After O years for some value of O, refpolicy decides that it no longer cares about policy load (not just reload) compatibility for kernels that predate this kernel commit, and both kernel and refpolicy can then start adding and using new initial SIDs beyond 27. This does not depend on the previous change (step 5) and can occur independent of it. Fixes: https://github.com/SELinuxProject/selinux-kernel/issues/12 Signed-off-by: Stephen Smalley Signed-off-by: Paul Moore --- scripts/selinux/genheaders/genheaders.c | 11 +++- .../selinux/include/initial_sid_to_string.h | 57 +++++++++---------- security/selinux/selinuxfs.c | 6 +- security/selinux/ss/policydb.c | 25 ++++---- security/selinux/ss/services.c | 26 ++++----- 5 files changed, 66 insertions(+), 59 deletions(-) diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c index 544ca126a8a8..f355b3e0e968 100644 --- a/scripts/selinux/genheaders/genheaders.c +++ b/scripts/selinux/genheaders/genheaders.c @@ -67,8 +67,12 @@ int main(int argc, char *argv[]) } isids_len = sizeof(initial_sid_to_string) / sizeof (char *); - for (i = 1; i < isids_len; i++) - initial_sid_to_string[i] = stoupperx(initial_sid_to_string[i]); + for (i = 1; i < isids_len; i++) { + const char *s = initial_sid_to_string[i]; + + if (s) + initial_sid_to_string[i] = stoupperx(s); + } fprintf(fout, "/* This file is automatically generated. Do not edit. */\n"); fprintf(fout, "#ifndef _SELINUX_FLASK_H_\n#define _SELINUX_FLASK_H_\n\n"); @@ -82,7 +86,8 @@ int main(int argc, char *argv[]) for (i = 1; i < isids_len; i++) { const char *s = initial_sid_to_string[i]; - fprintf(fout, "#define SECINITSID_%-39s %2d\n", s, i); + if (s) + fprintf(fout, "#define SECINITSID_%-39s %2d\n", s, i); } fprintf(fout, "\n#define SECINITSID_NUM %d\n", i-1); fprintf(fout, "\nstatic inline bool security_is_socket_class(u16 kern_tclass)\n"); diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h index 4f93f697f71c..5d332aeb8b6c 100644 --- a/security/selinux/include/initial_sid_to_string.h +++ b/security/selinux/include/initial_sid_to_string.h @@ -1,34 +1,33 @@ /* SPDX-License-Identifier: GPL-2.0 */ -/* This file is automatically generated. Do not edit. */ static const char *initial_sid_to_string[] = { - "null", - "kernel", - "security", - "unlabeled", - "fs", - "file", - "file_labels", - "init", - "any_socket", - "port", - "netif", - "netmsg", - "node", - "igmp_packet", - "icmp_socket", - "tcp_socket", - "sysctl_modprobe", - "sysctl", - "sysctl_fs", - "sysctl_kernel", - "sysctl_net", - "sysctl_net_unix", - "sysctl_vm", - "sysctl_dev", - "kmod", - "policy", - "scmp_packet", - "devnull", + NULL, + "kernel", + "security", + "unlabeled", + NULL, + "file", + NULL, + NULL, + "any_socket", + "port", + "netif", + "netmsg", + "node", + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + "devnull", }; diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 533ab170ad52..4781314c2510 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -1701,7 +1701,11 @@ static int sel_make_initcon_files(struct dentry *dir) for (i = 1; i <= SECINITSID_NUM; i++) { struct inode *inode; struct dentry *dentry; - dentry = d_alloc_name(dir, security_get_initial_sid_context(i)); + const char *s = security_get_initial_sid_context(i); + + if (!s) + continue; + dentry = d_alloc_name(dir, s); if (!dentry) return -ENOMEM; diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 7ca8c74efba3..7739369f5d9a 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -838,29 +838,28 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s) head = p->ocontexts[OCON_ISID]; for (c = head; c; c = c->next) { - rc = -EINVAL; - if (!c->context[0].user) { - pr_err("SELinux: SID %s was never defined.\n", - c->u.name); - sidtab_destroy(s); - goto out; - } - if (c->sid[0] == SECSID_NULL || c->sid[0] > SECINITSID_NUM) { - pr_err("SELinux: Initial SID %s out of range.\n", - c->u.name); + u32 sid = c->sid[0]; + const char *name = security_get_initial_sid_context(sid); + + if (sid == SECSID_NULL) { + pr_err("SELinux: SID 0 was assigned a context.\n"); sidtab_destroy(s); goto out; } + + /* Ignore initial SIDs unused by this kernel. */ + if (!name) + continue; + rc = context_add_hash(p, &c->context[0]); if (rc) { sidtab_destroy(s); goto out; } - - rc = sidtab_set_initial(s, c->sid[0], &c->context[0]); + rc = sidtab_set_initial(s, sid, &c->context[0]); if (rc) { pr_err("SELinux: unable to load initial SID %s.\n", - c->u.name); + name); sidtab_destroy(s); goto out; } diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index f90e6550eec8..8ad34fd031d1 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -1322,23 +1322,22 @@ static int security_sid_to_context_core(struct selinux_state *state, if (!selinux_initialized(state)) { if (sid <= SECINITSID_NUM) { char *scontextp; + const char *s = initial_sid_to_string[sid]; - *scontext_len = strlen(initial_sid_to_string[sid]) + 1; + if (!s) + return -EINVAL; + *scontext_len = strlen(s) + 1; if (!scontext) - goto out; - scontextp = kmemdup(initial_sid_to_string[sid], - *scontext_len, GFP_ATOMIC); - if (!scontextp) { - rc = -ENOMEM; - goto out; - } + return 0; + scontextp = kmemdup(s, *scontext_len, GFP_ATOMIC); + if (!scontextp) + return -ENOMEM; *scontext = scontextp; - goto out; + return 0; } pr_err("SELinux: %s: called before initial " "load_policy on unknown SID %d\n", __func__, sid); - rc = -EINVAL; - goto out; + return -EINVAL; } read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; @@ -1362,7 +1361,6 @@ static int security_sid_to_context_core(struct selinux_state *state, out_unlock: read_unlock(&state->ss->policy_rwlock); -out: return rc; } @@ -1552,7 +1550,9 @@ static int security_context_to_sid_core(struct selinux_state *state, int i; for (i = 1; i < SECINITSID_NUM; i++) { - if (!strcmp(initial_sid_to_string[i], scontext2)) { + const char *s = initial_sid_to_string[i]; + + if (s && !strcmp(s, scontext2)) { *sid = i; goto out; } From 34a2dab488bcaf2ac2198d7b305794280d73207b Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Tue, 3 Mar 2020 12:29:10 +0100 Subject: [PATCH 16/20] selinux: clean up error path in policydb_init() Commit e0ac568de1fa ("selinux: reduce the use of hard-coded hash sizes") moved symtab initialization out of policydb_init(), but left the cleanup of symtabs from the error path. This patch fixes the oversight. Suggested-by: Stephen Smalley Signed-off-by: Ondrej Mosnacek Acked-by: Stephen Smalley Signed-off-by: Paul Moore --- security/selinux/ss/policydb.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 7739369f5d9a..00edcd216aaa 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -463,36 +463,28 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2) */ static int policydb_init(struct policydb *p) { - int i, rc; + int rc; memset(p, 0, sizeof(*p)); rc = avtab_init(&p->te_avtab); if (rc) - goto out; + return rc; rc = cond_policydb_init(p); if (rc) - goto out; + return rc; p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp, (1 << 11)); - if (!p->filename_trans) { - rc = -ENOMEM; - goto out; - } + if (!p->filename_trans) + return -ENOMEM; ebitmap_init(&p->filename_trans_ttypes); ebitmap_init(&p->policycaps); ebitmap_init(&p->permissive_map); return 0; -out: - for (i = 0; i < SYM_NUM; i++) { - hashtab_map(p->symtab[i].table, destroy_f[i], NULL); - hashtab_destroy(p->symtab[i].table); - } - return rc; } /* From 5e729e111eaf37b7941c678cb84af62539a4799a Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Thu, 5 Mar 2020 14:55:43 -0500 Subject: [PATCH 17/20] selinux: avtab_init() and cond_policydb_init() return void The avtab_init() and cond_policydb_init() functions always return zero so mark them as returning void and update the callers not to check for a return value. Suggested-by: Stephen Smalley Reviewed-by: Ondrej Mosnacek Signed-off-by: Paul Moore --- security/selinux/ss/avtab.c | 3 +-- security/selinux/ss/avtab.h | 2 +- security/selinux/ss/conditional.c | 10 ++-------- security/selinux/ss/conditional.h | 2 +- security/selinux/ss/policydb.c | 11 ++--------- 5 files changed, 7 insertions(+), 21 deletions(-) diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c index 8c5800750fa8..01b300a4a882 100644 --- a/security/selinux/ss/avtab.c +++ b/security/selinux/ss/avtab.c @@ -299,12 +299,11 @@ void avtab_destroy(struct avtab *h) h->mask = 0; } -int avtab_init(struct avtab *h) +void avtab_init(struct avtab *h) { kvfree(h->htable); h->htable = NULL; h->nel = 0; - return 0; } int avtab_alloc(struct avtab *h, u32 nrules) diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h index 837e938798ef..5fdcb6696bcc 100644 --- a/security/selinux/ss/avtab.h +++ b/security/selinux/ss/avtab.h @@ -87,7 +87,7 @@ struct avtab { u32 mask; /* mask to compute hash func */ }; -int avtab_init(struct avtab *); +void avtab_init(struct avtab *h); int avtab_alloc(struct avtab *, u32); struct avtab_datum *avtab_search(struct avtab *h, struct avtab_key *k); void avtab_destroy(struct avtab *h); diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index cce4a75fb3e7..939a74fd8fb4 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -125,19 +125,13 @@ void evaluate_cond_nodes(struct policydb *p) evaluate_cond_node(p, &p->cond_list[i]); } -int cond_policydb_init(struct policydb *p) +void cond_policydb_init(struct policydb *p) { - int rc; - p->bool_val_to_struct = NULL; p->cond_list = NULL; p->cond_list_len = 0; - rc = avtab_init(&p->te_cond_avtab); - if (rc) - return rc; - - return 0; + avtab_init(&p->te_cond_avtab); } static void cond_node_destroy(struct cond_node *node) diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h index b9eb888ffa76..90c9c964f5f5 100644 --- a/security/selinux/ss/conditional.h +++ b/security/selinux/ss/conditional.h @@ -61,7 +61,7 @@ struct cond_node { struct cond_av_list false_list; }; -int cond_policydb_init(struct policydb *p); +void cond_policydb_init(struct policydb *p); void cond_policydb_destroy(struct policydb *p); int cond_init_bool_indexes(struct policydb *p); diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 00edcd216aaa..932b2b9bcdb2 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -463,17 +463,10 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2) */ static int policydb_init(struct policydb *p) { - int rc; - memset(p, 0, sizeof(*p)); - rc = avtab_init(&p->te_avtab); - if (rc) - return rc; - - rc = cond_policydb_init(p); - if (rc) - return rc; + avtab_init(&p->te_avtab); + cond_policydb_init(p); p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp, (1 << 11)); From 27978872179b815105082902b22c516359576673 Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Wed, 11 Mar 2020 16:05:51 -0400 Subject: [PATCH 18/20] MAINTAINERS: Update my email address Signed-off-by: Stephen Smalley Signed-off-by: Paul Moore --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 0b370797e8a6..e343b2c75fbc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14974,7 +14974,7 @@ X: security/selinux/ SELINUX SECURITY MODULE M: Paul Moore -M: Stephen Smalley +M: Stephen Smalley M: Eric Paris L: selinux@vger.kernel.org W: https://selinuxproject.org From 779df6a5480f1307d51b66ea72352be592265cad Mon Sep 17 00:00:00 2001 From: Scott Mayhew Date: Tue, 3 Mar 2020 17:58:37 -0500 Subject: [PATCH 19/20] NFS: Ensure security label is set for root inode When using NFSv4.2, the security label for the root inode should be set via a call to nfs_setsecurity() during the mount process, otherwise the inode will appear as unlabeled for up to acdirmin seconds. Currently the label for the root inode is allocated, retrieved, and freed entirely witin nfs4_proc_get_root(). Add a field for the label to the nfs_fattr struct, and allocate & free the label in nfs_get_root(), where we also add a call to nfs_setsecurity(). Note that for the call to nfs_setsecurity() to succeed, it's necessary to also move the logic calling security_sb_{set,clone}_security() from nfs_get_tree_common() down into nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in the super_block's security flags and nfs_setsecurity() will silently fail. Reported-by: Richard Haines Signed-off-by: Scott Mayhew Acked-by: Stephen Smalley Tested-by: Stephen Smalley [PM: fixed 80-char line width problems] Signed-off-by: Paul Moore --- fs/nfs/getroot.c | 39 +++++++++++++++++++++++++++++++++++---- fs/nfs/nfs4proc.c | 12 +++--------- fs/nfs/super.c | 25 ------------------------- include/linux/nfs_xdr.h | 1 + 4 files changed, 39 insertions(+), 38 deletions(-) diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c index b012c2668a1f..aaeeb4659bff 100644 --- a/fs/nfs/getroot.c +++ b/fs/nfs/getroot.c @@ -73,6 +73,7 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) struct inode *inode; char *name; int error = -ENOMEM; + unsigned long kflags = 0, kflags_out = 0; name = kstrdup(fc->source, GFP_KERNEL); if (!name) @@ -83,11 +84,14 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) if (fsinfo.fattr == NULL) goto out_name; + fsinfo.fattr->label = nfs4_label_alloc(server, GFP_KERNEL); + if (IS_ERR(fsinfo.fattr->label)) + goto out_fattr; error = server->nfs_client->rpc_ops->getroot(server, ctx->mntfh, &fsinfo); if (error < 0) { dprintk("nfs_get_root: getattr error = %d\n", -error); nfs_errorf(fc, "NFS: Couldn't getattr on root"); - goto out_fattr; + goto out_label; } inode = nfs_fhget(s, ctx->mntfh, fsinfo.fattr, NULL); @@ -95,12 +99,12 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) dprintk("nfs_get_root: get root inode failed\n"); error = PTR_ERR(inode); nfs_errorf(fc, "NFS: Couldn't get root inode"); - goto out_fattr; + goto out_label; } error = nfs_superblock_set_dummy_root(s, inode); if (error != 0) - goto out_fattr; + goto out_label; /* root dentries normally start off anonymous and get spliced in later * if the dentry tree reaches them; however if the dentry already @@ -111,7 +115,7 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) dprintk("nfs_get_root: get root dentry failed\n"); error = PTR_ERR(root); nfs_errorf(fc, "NFS: Couldn't get root dentry"); - goto out_fattr; + goto out_label; } security_d_instantiate(root, inode); @@ -123,12 +127,39 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) } spin_unlock(&root->d_lock); fc->root = root; + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL) + kflags |= SECURITY_LSM_NATIVE_LABELS; + if (ctx->clone_data.sb) { + if (d_inode(fc->root)->i_fop != &nfs_dir_operations) { + error = -ESTALE; + goto error_splat_root; + } + /* clone lsm security options from the parent to the new sb */ + error = security_sb_clone_mnt_opts(ctx->clone_data.sb, + s, kflags, &kflags_out); + } else { + error = security_sb_set_mnt_opts(s, fc->security, + kflags, &kflags_out); + } + if (error) + goto error_splat_root; + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL && + !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) + NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL; + + nfs_setsecurity(inode, fsinfo.fattr, fsinfo.fattr->label); error = 0; +out_label: + nfs4_label_free(fsinfo.fattr->label); out_fattr: nfs_free_fattr(fsinfo.fattr); out_name: kfree(name); out: return error; +error_splat_root: + dput(fc->root); + fc->root = NULL; + goto out_label; } diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 95d07a3dc5d1..9a82066cf26b 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -3988,7 +3988,7 @@ static int nfs4_proc_get_root(struct nfs_server *server, struct nfs_fh *mntfh, { int error; struct nfs_fattr *fattr = info->fattr; - struct nfs4_label *label = NULL; + struct nfs4_label *label = fattr->label; error = nfs4_server_capabilities(server, mntfh); if (error < 0) { @@ -3996,23 +3996,17 @@ static int nfs4_proc_get_root(struct nfs_server *server, struct nfs_fh *mntfh, return error; } - label = nfs4_label_alloc(server, GFP_KERNEL); - if (IS_ERR(label)) - return PTR_ERR(label); - error = nfs4_proc_getattr(server, mntfh, fattr, label, NULL); if (error < 0) { dprintk("nfs4_get_root: getattr error = %d\n", -error); - goto err_free_label; + goto out; } if (fattr->valid & NFS_ATTR_FATTR_FSID && !nfs_fsid_equal(&server->fsid, &fattr->fsid)) memcpy(&server->fsid, &fattr->fsid, sizeof(server->fsid)); -err_free_label: - nfs4_label_free(label); - +out: return error; } diff --git a/fs/nfs/super.c b/fs/nfs/super.c index dada09b391c6..bb14bede6da5 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -1179,7 +1179,6 @@ int nfs_get_tree_common(struct fs_context *fc) struct super_block *s; int (*compare_super)(struct super_block *, struct fs_context *) = nfs_compare_super; struct nfs_server *server = ctx->server; - unsigned long kflags = 0, kflags_out = 0; int error; ctx->server = NULL; @@ -1239,26 +1238,6 @@ int nfs_get_tree_common(struct fs_context *fc) goto error_splat_super; } - if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL) - kflags |= SECURITY_LSM_NATIVE_LABELS; - if (ctx->clone_data.sb) { - if (d_inode(fc->root)->i_fop != &nfs_dir_operations) { - error = -ESTALE; - goto error_splat_root; - } - /* clone any lsm security options from the parent to the new sb */ - error = security_sb_clone_mnt_opts(ctx->clone_data.sb, s, kflags, - &kflags_out); - } else { - error = security_sb_set_mnt_opts(s, fc->security, - kflags, &kflags_out); - } - if (error) - goto error_splat_root; - if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL && - !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) - NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL; - s->s_flags |= SB_ACTIVE; error = 0; @@ -1268,10 +1247,6 @@ int nfs_get_tree_common(struct fs_context *fc) out_err_nosb: nfs_free_server(server); goto out; - -error_splat_root: - dput(fc->root); - fc->root = NULL; error_splat_super: deactivate_locked_super(s); goto out; diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h index 94c77ed55ce1..6838c149f335 100644 --- a/include/linux/nfs_xdr.h +++ b/include/linux/nfs_xdr.h @@ -75,6 +75,7 @@ struct nfs_fattr { struct nfs4_string *owner_name; struct nfs4_string *group_name; struct nfs4_threshold *mdsthreshold; /* pNFS threshold hints */ + struct nfs4_label *label; }; #define NFS_ATTR_FATTR_TYPE (1U << 0) From c753924b628551564b6eea3c9896e4a95aa25ed9 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Fri, 27 Mar 2020 17:44:02 +0000 Subject: [PATCH 20/20] selinux: clean up indentation issue with assignment statement The assignment of e->type_names is indented one level too deep, clean this up by removing the extraneous tab. Signed-off-by: Colin Ian King Signed-off-by: Paul Moore --- security/selinux/ss/policydb.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 932b2b9bcdb2..70ecdc78efbd 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1219,10 +1219,9 @@ static int read_cons_helper(struct policydb *p, if (rc) return rc; if (p->policyvers >= - POLICYDB_VERSION_CONSTRAINT_NAMES) { - e->type_names = kzalloc(sizeof - (*e->type_names), - GFP_KERNEL); + POLICYDB_VERSION_CONSTRAINT_NAMES) { + e->type_names = kzalloc(sizeof + (*e->type_names), GFP_KERNEL); if (!e->type_names) return -ENOMEM; type_set_init(e->type_names);