From 3b0c2d3eaa83da259d7726192cf55a137769012f Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Fri, 12 Mar 2021 15:07:09 -0600 Subject: Revert 95ebabde382c ("capabilities: Don't allow writing ambiguous v3 file capabilities") It turns out that there are in fact userspace implementations that care and this recent change caused a regression. https://github.com/containers/buildah/issues/3071 As the motivation for the original change was future development, and the impact is existing real world code just revert this change and allow the ambiguity in v3 file caps. Cc: stable@vger.kernel.org Fixes: 95ebabde382c ("capabilities: Don't allow writing ambiguous v3 file capabilities") Signed-off-by: Eric W. Biederman --- security/commoncap.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) (limited to 'security') diff --git a/security/commoncap.c b/security/commoncap.c index 28f4d25480df..1c519c875217 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -543,8 +543,7 @@ int cap_convert_nscap(struct user_namespace *mnt_userns, struct dentry *dentry, __u32 magic, nsmagic; struct inode *inode = d_backing_inode(dentry); struct user_namespace *task_ns = current_user_ns(), - *fs_ns = inode->i_sb->s_user_ns, - *ancestor; + *fs_ns = inode->i_sb->s_user_ns; kuid_t rootid; size_t newsize; @@ -567,15 +566,6 @@ int cap_convert_nscap(struct user_namespace *mnt_userns, struct dentry *dentry, if (nsrootid == -1) return -EINVAL; - /* - * Do not allow allow adding a v3 filesystem capability xattr - * if the rootid field is ambiguous. - */ - for (ancestor = task_ns->parent; ancestor; ancestor = ancestor->parent) { - if (from_kuid(ancestor, rootid) == 0) - return -EINVAL; - } - newsize = sizeof(struct vfs_ns_cap_data); nscap = kmalloc(newsize, GFP_ATOMIC); if (!nscap) -- cgit v1.2.3 From 519dad3bcd809dc1523bf80ab0310ddb3bf00ade Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Thu, 18 Mar 2021 22:53:01 +0100 Subject: selinux: don't log MAC_POLICY_LOAD record on failed policy load If sel_make_policy_nodes() fails, we should jump to 'out', not 'out1', as the latter would incorrectly log an MAC_POLICY_LOAD audit record, even though the policy hasn't actually been reloaded. The 'out1' jump label now becomes unused and can be removed. Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs") Cc: stable@vger.kernel.org Signed-off-by: Ondrej Mosnacek Signed-off-by: Paul Moore --- security/selinux/selinuxfs.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'security') diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 01a7d50ed39b..340711e3dc9a 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -651,14 +651,13 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, length = sel_make_policy_nodes(fsi, newpolicy); if (length) { selinux_policy_cancel(fsi->state, newpolicy); - goto out1; + goto out; } selinux_policy_commit(fsi->state, newpolicy); length = count; -out1: audit_log(audit_context(), GFP_KERNEL, AUDIT_MAC_POLICY_LOAD, "auid=%u ses=%u lsm=selinux res=1", from_kuid(&init_user_ns, audit_get_loginuid(current)), -- cgit v1.2.3 From 6406887a12ee5dcdaffff1a8508d91113d545559 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Thu, 18 Mar 2021 22:53:02 +0100 Subject: selinux: fix variable scope issue in live sidtab conversion Commit 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs") moved the selinux_policy_commit() call out of security_load_policy() into sel_write_load(), which caused a subtle yet rather serious bug. The problem is that security_load_policy() passes a reference to the convert_params local variable to sidtab_convert(), which stores it in the sidtab, where it may be accessed until the policy is swapped over and RCU synchronized. Before 02a52c5c8c3b, selinux_policy_commit() was called directly from security_load_policy(), so the convert_params pointer remained valid all the way until the old sidtab was destroyed, but now that's no longer the case and calls to sidtab_context_to_sid() on the old sidtab after security_load_policy() returns may cause invalid memory accesses. This can be easily triggered using the stress test from commit ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve performance"): ``` function rand_cat() { echo $(( $RANDOM % 1024 )) } function do_work() { while true; do echo -n "system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \ >/sys/fs/selinux/context 2>/dev/null || true done } do_work >/dev/null & do_work >/dev/null & do_work >/dev/null & while load_policy; do echo -n .; sleep 0.1; done kill %1 kill %2 kill %3 ``` Fix this by allocating the temporary sidtab convert structures dynamically and passing them among the selinux_policy_{load,cancel,commit} functions. Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs") Cc: stable@vger.kernel.org Tested-by: Tyler Hicks Reviewed-by: Tyler Hicks Signed-off-by: Ondrej Mosnacek [PM: merge fuzz in security.h and services.c] Signed-off-by: Paul Moore --- security/selinux/include/security.h | 15 ++++++--- security/selinux/selinuxfs.c | 10 +++--- security/selinux/ss/services.c | 63 +++++++++++++++++++++++-------------- 3 files changed, 55 insertions(+), 33 deletions(-) (limited to 'security') diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 765a258a899e..25db66e0ac51 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -219,14 +219,21 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void) return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]); } +struct selinux_policy_convert_data; + +struct selinux_load_state { + struct selinux_policy *policy; + struct selinux_policy_convert_data *convert_data; +}; + int security_mls_enabled(struct selinux_state *state); int security_load_policy(struct selinux_state *state, - void *data, size_t len, - struct selinux_policy **newpolicyp); + void *data, size_t len, + struct selinux_load_state *load_state); void selinux_policy_commit(struct selinux_state *state, - struct selinux_policy *newpolicy); + struct selinux_load_state *load_state); void selinux_policy_cancel(struct selinux_state *state, - struct selinux_policy *policy); + struct selinux_load_state *load_state); int security_read_policy(struct selinux_state *state, void **data, size_t *len); diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 340711e3dc9a..158d44ea93f4 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -616,7 +616,7 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, { struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info; - struct selinux_policy *newpolicy; + struct selinux_load_state load_state; ssize_t length; void *data = NULL; @@ -642,19 +642,19 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, if (copy_from_user(data, buf, count) != 0) goto out; - length = security_load_policy(fsi->state, data, count, &newpolicy); + length = security_load_policy(fsi->state, data, count, &load_state); if (length) { pr_warn_ratelimited("SELinux: failed to load policy\n"); goto out; } - length = sel_make_policy_nodes(fsi, newpolicy); + length = sel_make_policy_nodes(fsi, load_state.policy); if (length) { - selinux_policy_cancel(fsi->state, newpolicy); + selinux_policy_cancel(fsi->state, &load_state); goto out; } - selinux_policy_commit(fsi->state, newpolicy); + selinux_policy_commit(fsi->state, &load_state); length = count; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 5e08ce2c5994..4a907e008a98 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -66,6 +66,17 @@ #include "audit.h" #include "policycap_names.h" +struct convert_context_args { + struct selinux_state *state; + struct policydb *oldp; + struct policydb *newp; +}; + +struct selinux_policy_convert_data { + struct convert_context_args args; + struct sidtab_convert_params sidtab_params; +}; + /* Forward declaration. */ static int context_struct_to_string(struct policydb *policydb, struct context *context, @@ -1973,12 +1984,6 @@ static inline int convert_context_handle_invalid_context( return 0; } -struct convert_context_args { - struct selinux_state *state; - struct policydb *oldp; - struct policydb *newp; -}; - /* * Convert the values in the security context * structure `oldc' from the values specified @@ -2158,7 +2163,7 @@ static void selinux_policy_cond_free(struct selinux_policy *policy) } void selinux_policy_cancel(struct selinux_state *state, - struct selinux_policy *policy) + struct selinux_load_state *load_state) { struct selinux_policy *oldpolicy; @@ -2166,7 +2171,8 @@ void selinux_policy_cancel(struct selinux_state *state, lockdep_is_held(&state->policy_mutex)); sidtab_cancel_convert(oldpolicy->sidtab); - selinux_policy_free(policy); + selinux_policy_free(load_state->policy); + kfree(load_state->convert_data); } static void selinux_notify_policy_change(struct selinux_state *state, @@ -2181,9 +2187,9 @@ static void selinux_notify_policy_change(struct selinux_state *state, } void selinux_policy_commit(struct selinux_state *state, - struct selinux_policy *newpolicy) + struct selinux_load_state *load_state) { - struct selinux_policy *oldpolicy; + struct selinux_policy *oldpolicy, *newpolicy = load_state->policy; u32 seqno; oldpolicy = rcu_dereference_protected(state->policy, @@ -2223,6 +2229,7 @@ void selinux_policy_commit(struct selinux_state *state, /* Free the old policy */ synchronize_rcu(); selinux_policy_free(oldpolicy); + kfree(load_state->convert_data); /* Notify others of the policy change */ selinux_notify_policy_change(state, seqno); @@ -2239,11 +2246,10 @@ void selinux_policy_commit(struct selinux_state *state, * loading the new policy. */ int security_load_policy(struct selinux_state *state, void *data, size_t len, - struct selinux_policy **newpolicyp) + struct selinux_load_state *load_state) { struct selinux_policy *newpolicy, *oldpolicy; - struct sidtab_convert_params convert_params; - struct convert_context_args args; + struct selinux_policy_convert_data *convert_data; int rc = 0; struct policy_file file = { data, len }, *fp = &file; @@ -2273,10 +2279,10 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len, goto err_mapping; } - if (!selinux_initialized(state)) { /* First policy load, so no need to preserve state from old policy */ - *newpolicyp = newpolicy; + load_state->policy = newpolicy; + load_state->convert_data = NULL; return 0; } @@ -2290,29 +2296,38 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len, goto err_free_isids; } + convert_data = kmalloc(sizeof(*convert_data), GFP_KERNEL); + if (!convert_data) { + rc = -ENOMEM; + goto err_free_isids; + } + /* * Convert the internal representations of contexts * in the new SID table. */ - args.state = state; - args.oldp = &oldpolicy->policydb; - args.newp = &newpolicy->policydb; + convert_data->args.state = state; + convert_data->args.oldp = &oldpolicy->policydb; + convert_data->args.newp = &newpolicy->policydb; - convert_params.func = convert_context; - convert_params.args = &args; - convert_params.target = newpolicy->sidtab; + convert_data->sidtab_params.func = convert_context; + convert_data->sidtab_params.args = &convert_data->args; + convert_data->sidtab_params.target = newpolicy->sidtab; - rc = sidtab_convert(oldpolicy->sidtab, &convert_params); + rc = sidtab_convert(oldpolicy->sidtab, &convert_data->sidtab_params); if (rc) { pr_err("SELinux: unable to convert the internal" " representation of contexts in the new SID" " table\n"); - goto err_free_isids; + goto err_free_convert_data; } - *newpolicyp = newpolicy; + load_state->policy = newpolicy; + load_state->convert_data = convert_data; return 0; +err_free_convert_data: + kfree(convert_data); err_free_isids: sidtab_destroy(newpolicy->sidtab); err_mapping: -- cgit v1.2.3 From ee5de60a08b7d8d255722662da461ea159c15538 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Thu, 18 Mar 2021 22:53:03 +0100 Subject: selinuxfs: unify policy load error reporting Let's drop the pr_err()s from sel_make_policy_nodes() and just add one pr_warn_ratelimited() call to the sel_make_policy_nodes() error path in sel_write_load(). Changing from error to warning makes sense, since after 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs"), this error path no longer leads to a broken selinuxfs tree (it's just kept in the original state and policy load is aborted). I also added _ratelimited to be consistent with the other prtin in the same function (it's probably not necessary, but can't really hurt... there are likely more important error messages to be printed when filesystem entry creation starts erroring out). Suggested-by: Paul Moore Signed-off-by: Ondrej Mosnacek Signed-off-by: Paul Moore --- security/selinux/selinuxfs.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) (limited to 'security') diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 158d44ea93f4..fff6babeeae6 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -563,17 +563,13 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi, ret = sel_make_bools(newpolicy, tmp_bool_dir, &tmp_bool_num, &tmp_bool_names, &tmp_bool_values); - if (ret) { - pr_err("SELinux: failed to load policy booleans\n"); + if (ret) goto out; - } ret = sel_make_classes(newpolicy, tmp_class_dir, &fsi->last_class_ino); - if (ret) { - pr_err("SELinux: failed to load policy classes\n"); + if (ret) goto out; - } /* booleans */ old_dentry = fsi->bool_dir; @@ -650,6 +646,7 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, length = sel_make_policy_nodes(fsi, load_state.policy); if (length) { + pr_warn_ratelimited("SELinux: failed to initialize selinuxfs\n"); selinux_policy_cancel(fsi->state, &load_state); goto out; } -- cgit v1.2.3 From 92063f3ca73aab794bd5408d3361fd5b5ea33079 Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Fri, 19 Mar 2021 11:17:23 -0400 Subject: integrity: double check iint_cache was initialized The kernel may be built with multiple LSMs, but only a subset may be enabled on the boot command line by specifying "lsm=". Not including "integrity" on the ordered LSM list may result in a NULL deref. As reported by Dmitry Vyukov: in qemu: qemu-system-x86_64 -enable-kvm -machine q35,nvdimm -cpu max,migratable=off -smp 4 -m 4G,slots=4,maxmem=16G -hda wheezy.img -kernel arch/x86/boot/bzImage -nographic -vga std -soundhw all -usb -usbdevice tablet -bt hci -bt device:keyboard -net user,host=10.0.2.10,hostfwd=tcp::10022-:22 -net nic,model=virtio-net-pci -object memory-backend-file,id=pmem1,share=off,mem-path=/dev/zero,size=64M -device nvdimm,id=nvdimm1,memdev=pmem1 -append "console=ttyS0 root=/dev/sda earlyprintk=serial rodata=n oops=panic panic_on_warn=1 panic=86400 lsm=smack numa=fake=2 nopcid dummy_hcd.num=8" -pidfile vm_pid -m 2G -cpu host But it crashes on NULL deref in integrity_inode_get during boot: Run /sbin/init as init process BUG: kernel NULL pointer dereference, address: 000000000000001c PGD 0 P4D 0 Oops: 0000 [#1] PREEMPT SMP KASAN CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2+ #97 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-44-g88ab0c15525c-prebuilt.qemu.org 04/01/2014 RIP: 0010:kmem_cache_alloc+0x2b/0x370 mm/slub.c:2920 Code: 57 41 56 41 55 41 54 41 89 f4 55 48 89 fd 53 48 83 ec 10 44 8b 3d d9 1f 90 0b 65 48 8b 04 25 28 00 00 00 48 89 44 24 08 31 c0 <8b> 5f 1c 4cf RSP: 0000:ffffc9000032f9d8 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff888017fc4f00 RCX: 0000000000000000 RDX: ffff888040220000 RSI: 0000000000000c40 RDI: 0000000000000000 RBP: 0000000000000000 R08: 0000000000000000 R09: ffff888019263627 R10: ffffffff83937cd1 R11: 0000000000000000 R12: 0000000000000c40 R13: ffff888019263538 R14: 0000000000000000 R15: 0000000000ffffff FS: 0000000000000000(0000) GS:ffff88802d180000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000000000000001c CR3: 000000000b48e000 CR4: 0000000000750ee0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 PKRU: 55555554 Call Trace: integrity_inode_get+0x47/0x260 security/integrity/iint.c:105 process_measurement+0x33d/0x17e0 security/integrity/ima/ima_main.c:237 ima_bprm_check+0xde/0x210 security/integrity/ima/ima_main.c:474 security_bprm_check+0x7d/0xa0 security/security.c:845 search_binary_handler fs/exec.c:1708 [inline] exec_binprm fs/exec.c:1761 [inline] bprm_execve fs/exec.c:1830 [inline] bprm_execve+0x764/0x19a0 fs/exec.c:1792 kernel_execve+0x370/0x460 fs/exec.c:1973 try_to_run_init_process+0x14/0x4e init/main.c:1366 kernel_init+0x11d/0x1b8 init/main.c:1477 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 Modules linked in: CR2: 000000000000001c ---[ end trace 22d601a500de7d79 ]--- Since LSMs and IMA may be configured at build time, but not enabled at run time, panic the system if "integrity" was not initialized before use. Reported-by: Dmitry Vyukov Fixes: 79f7865d844c ("LSM: Introduce "lsm=" for boottime LSM selection") Cc: stable@vger.kernel.org Signed-off-by: Mimi Zohar --- security/integrity/iint.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'security') diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 1d20003243c3..0ba01847e836 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -98,6 +98,14 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) struct rb_node *node, *parent = NULL; struct integrity_iint_cache *iint, *test_iint; + /* + * The integrity's "iint_cache" is initialized at security_init(), + * unless it is not included in the ordered list of LSMs enabled + * on the boot command line. + */ + if (!iint_cache) + panic("%s: lsm=integrity required.\n", __func__); + iint = integrity_iint_find(inode); if (iint) return iint; -- cgit v1.2.3