From 5933a62708fbae49931694314f3c98fbe91bb178 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Thu, 24 Aug 2017 09:31:45 +0100 Subject: apparmor: fix spelling mistake: "resoure" -> "resource" Trivial fix to spelling mistake in comment and also with text in audit_resource call. Signed-off-by: Colin Ian King Signed-off-by: John Johansen --- security/apparmor/resource.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/security/apparmor/resource.c b/security/apparmor/resource.c index d8bc842594ed..cf4d234febe9 100644 --- a/security/apparmor/resource.c +++ b/security/apparmor/resource.c @@ -47,7 +47,7 @@ static void audit_cb(struct audit_buffer *ab, void *va) /** * audit_resource - audit setting resource limit * @profile: profile being enforced (NOT NULL) - * @resoure: rlimit being auditing + * @resource: rlimit being auditing * @value: value being set * @error: error value * @@ -128,7 +128,7 @@ int aa_task_setrlimit(struct aa_label *label, struct task_struct *task, error = fn_for_each(label, profile, audit_resource(profile, resource, new_rlim->rlim_max, peer, - "cap_sys_resoure", -EACCES)); + "cap_sys_resource", -EACCES)); else error = fn_for_each_confined(label, profile, profile_setrlimit(profile, resource, new_rlim)); -- cgit From 7bba39ae52c4d7b467d0a6f74cc067a561aac043 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Fri, 15 Sep 2017 21:55:46 +0200 Subject: apparmor: initialized returned struct aa_perms gcc-4.4 points out suspicious code in compute_mnt_perms, where the aa_perms structure is only partially initialized before getting returned: security/apparmor/mount.c: In function 'compute_mnt_perms': security/apparmor/mount.c:227: error: 'perms.prompt' is used uninitialized in this function security/apparmor/mount.c:227: error: 'perms.hide' is used uninitialized in this function security/apparmor/mount.c:227: error: 'perms.cond' is used uninitialized in this function security/apparmor/mount.c:227: error: 'perms.complain' is used uninitialized in this function security/apparmor/mount.c:227: error: 'perms.stop' is used uninitialized in this function security/apparmor/mount.c:227: error: 'perms.deny' is used uninitialized in this function Returning or assigning partially initialized structures is a bit tricky, in particular it is explicitly allowed in c99 to assign a partially initialized structure to another, as long as only members are read that have been initialized earlier. Looking at what various compilers do here, the version that produced the warning copied uninitialized stack data, while newer versions (and also clang) either set the other members to zero or don't update the parts of the return buffer that are not modified in the temporary structure, but they never warn about this. In case of apparmor, it seems better to be a little safer and always initialize the aa_perms structure. Most users already do that, this changes the remaining ones, including the one instance that I got the warning for. Fixes: fa488437d0f9 ("apparmor: add mount mediation") Signed-off-by: Arnd Bergmann Reviewed-by: Seth Arnold Acked-by: Geert Uytterhoeven Signed-off-by: John Johansen --- security/apparmor/file.c | 8 +------- security/apparmor/lib.c | 13 +++++-------- security/apparmor/mount.c | 13 ++++++------- 3 files changed, 12 insertions(+), 22 deletions(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index 3382518b87fa..e79bf44396a3 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -226,18 +226,12 @@ static u32 map_old_perms(u32 old) struct aa_perms aa_compute_fperms(struct aa_dfa *dfa, unsigned int state, struct path_cond *cond) { - struct aa_perms perms; - /* FIXME: change over to new dfa format * currently file perms are encoded in the dfa, new format * splits the permissions from the dfa. This mapping can be * done at profile load */ - perms.deny = 0; - perms.kill = perms.stop = 0; - perms.complain = perms.cond = 0; - perms.hide = 0; - perms.prompt = 0; + struct aa_perms perms = { }; if (uid_eq(current_fsuid(), cond->uid)) { perms.allow = map_old_perms(dfa_user_allow(dfa, state)); diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 08ca26bcca77..3d0a2bf87abd 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -317,14 +317,11 @@ static u32 map_other(u32 x) void aa_compute_perms(struct aa_dfa *dfa, unsigned int state, struct aa_perms *perms) { - perms->deny = 0; - perms->kill = perms->stop = 0; - perms->complain = perms->cond = 0; - perms->hide = 0; - perms->prompt = 0; - perms->allow = dfa_user_allow(dfa, state); - perms->audit = dfa_user_audit(dfa, state); - perms->quiet = dfa_user_quiet(dfa, state); + *perms = (struct aa_perms) { + .allow = dfa_user_allow(dfa, state), + .audit = dfa_user_audit(dfa, state), + .quiet = dfa_user_quiet(dfa, state), + }; /* for v5 perm mapping in the policydb, the other set is used * to extend the general perm set diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c index 82a64b58041d..ed9b4d0f9f7e 100644 --- a/security/apparmor/mount.c +++ b/security/apparmor/mount.c @@ -216,13 +216,12 @@ static unsigned int match_mnt_flags(struct aa_dfa *dfa, unsigned int state, static struct aa_perms compute_mnt_perms(struct aa_dfa *dfa, unsigned int state) { - struct aa_perms perms; - - perms.kill = 0; - perms.allow = dfa_user_allow(dfa, state); - perms.audit = dfa_user_audit(dfa, state); - perms.quiet = dfa_user_quiet(dfa, state); - perms.xindex = dfa_user_xindex(dfa, state); + struct aa_perms perms = { + .allow = dfa_user_allow(dfa, state), + .audit = dfa_user_audit(dfa, state), + .quiet = dfa_user_quiet(dfa, state), + .xindex = dfa_user_xindex(dfa, state), + }; return perms; } -- cgit From 954317fef2ee789af55f82903dbc574905250f7c Mon Sep 17 00:00:00 2001 From: Thomas Meyer Date: Sat, 7 Oct 2017 16:02:21 +0200 Subject: apparmor: Fix bool initialization/comparison Bool initializations should use true and false. Bool tests don't need comparisons. Signed-off-by: Thomas Meyer Signed-off-by: John Johansen --- security/apparmor/lsm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 1346ee5be04f..a2fb2de5611a 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -846,7 +846,7 @@ module_param_call(audit, param_set_audit, param_get_audit, /* Determines if audit header is included in audited messages. This * provides more context if the audit daemon is not running */ -bool aa_g_audit_header = 1; +bool aa_g_audit_header = true; module_param_named(audit_header, aa_g_audit_header, aabool, S_IRUSR | S_IWUSR); @@ -871,7 +871,7 @@ module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR); * DEPRECATED: read only as strict checking of load is always done now * that none root users (user namespaces) can load policy. */ -bool aa_g_paranoid_load = 1; +bool aa_g_paranoid_load = true; module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO); /* Boot time disable flag */ @@ -1119,7 +1119,7 @@ static int __init apparmor_init(void) if (!apparmor_enabled || !security_module_enable("apparmor")) { aa_info_message("AppArmor disabled by boot time parameter"); - apparmor_enabled = 0; + apparmor_enabled = false; return 0; } @@ -1175,7 +1175,7 @@ alloc_out: aa_destroy_aafs(); aa_teardown_dfa_engine(); - apparmor_enabled = 0; + apparmor_enabled = false; return error; } -- cgit From e3bcfc148588e409685479f3d20ba3d66ae30035 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Sat, 14 Oct 2017 13:14:38 +0100 Subject: apparmor: remove unused redundant variable stop The boolean variable 'stop' is being set but never read. This is a redundant variable and can be removed. Cleans up clang warning: Value stored to 'stop' is never read Signed-off-by: Colin Ian King Signed-off-by: John Johansen --- security/apparmor/lib.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 3d0a2bf87abd..4d5e98e49d5e 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -423,7 +423,6 @@ int aa_check_perms(struct aa_profile *profile, struct aa_perms *perms, void (*cb)(struct audit_buffer *, void *)) { int type, error; - bool stop = false; u32 denied = request & (~perms->allow | perms->deny); if (likely(!denied)) { @@ -444,8 +443,6 @@ int aa_check_perms(struct aa_profile *profile, struct aa_perms *perms, else type = AUDIT_APPARMOR_DENIED; - if (denied & perms->stop) - stop = true; if (denied == (denied & perms->hide)) error = -ENOENT; -- cgit From 4633307e5ed6128975595df43f796a10c41d11c1 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 15 Nov 2017 15:25:30 -0800 Subject: apparmor: fix leak of null profile name if profile allocation fails Fixes: d07881d2edb0 ("apparmor: move new_null_profile to after profile lookup fns()") Reported-by: Seth Arnold Signed-off-by: John Johansen --- security/apparmor/policy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 4243b0c3f0e4..586b249d3b46 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -502,7 +502,7 @@ struct aa_profile *aa_new_null_profile(struct aa_profile *parent, bool hat, { struct aa_profile *p, *profile; const char *bname; - char *name; + char *name = NULL; AA_BUG(!parent); @@ -562,6 +562,7 @@ out: return profile; fail: + kfree(name); aa_free_profile(profile); return NULL; } -- cgit From 844b8292b6311ecd30ae63db1471edb26e01d895 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 17 Nov 2017 17:42:42 -0800 Subject: apparmor: ensure that undecidable profile attachments fail Profiles that have an undecidable overlap in their attachments are being incorrectly handled. Instead of failing to attach the first one encountered is being used. eg. profile A /** { .. } profile B /*foo { .. } have an unresolvable longest left attachment, they both have an exact match on / and then have an overlapping expression that has no clear winner. Currently the winner will be the profile that is loaded first which can result in non-deterministic behavior. Instead in this situation the exec should fail. Fixes: 898127c34ec0 ("AppArmor: functions for domain transitions") Signed-off-by: John Johansen --- security/apparmor/domain.c | 46 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index dd754b7850a8..9527adc11c6d 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -305,6 +305,7 @@ static int change_profile_perms(struct aa_profile *profile, * __attach_match_ - find an attachment match * @name - to match against (NOT NULL) * @head - profile list to walk (NOT NULL) + * @info - info message if there was an error (NOT NULL) * * Do a linear search on the profiles in the list. There is a matching * preference where an exact match is preferred over a name which uses @@ -316,28 +317,44 @@ static int change_profile_perms(struct aa_profile *profile, * Returns: profile or NULL if no match found */ static struct aa_profile *__attach_match(const char *name, - struct list_head *head) + struct list_head *head, + const char **info) { int len = 0; + bool conflict = false; struct aa_profile *profile, *candidate = NULL; list_for_each_entry_rcu(profile, head, base.list) { if (profile->label.flags & FLAG_NULL) continue; - if (profile->xmatch && profile->xmatch_len > len) { - unsigned int state = aa_dfa_match(profile->xmatch, - DFA_START, name); - u32 perm = dfa_user_allow(profile->xmatch, state); - /* any accepting state means a valid match. */ - if (perm & MAY_EXEC) { - candidate = profile; - len = profile->xmatch_len; + if (profile->xmatch) { + if (profile->xmatch_len == len) { + conflict = true; + continue; + } else if (profile->xmatch_len > len) { + unsigned int state; + u32 perm; + + state = aa_dfa_match(profile->xmatch, + DFA_START, name); + perm = dfa_user_allow(profile->xmatch, state); + /* any accepting state means a valid match. */ + if (perm & MAY_EXEC) { + candidate = profile; + len = profile->xmatch_len; + conflict = false; + } } } else if (!strcmp(profile->base.name, name)) /* exact non-re match, no more searching required */ return profile; } + if (conflict) { + *info = "conflicting profile attachments"; + return NULL; + } + return candidate; } @@ -346,16 +363,17 @@ static struct aa_profile *__attach_match(const char *name, * @ns: the current namespace (NOT NULL) * @list: list to search (NOT NULL) * @name: the executable name to match against (NOT NULL) + * @info: info message if there was an error * * Returns: label or NULL if no match found */ static struct aa_label *find_attach(struct aa_ns *ns, struct list_head *list, - const char *name) + const char *name, const char **info) { struct aa_profile *profile; rcu_read_lock(); - profile = aa_get_profile(__attach_match(name, list)); + profile = aa_get_profile(__attach_match(name, list, info)); rcu_read_unlock(); return profile ? &profile->label : NULL; @@ -448,11 +466,11 @@ static struct aa_label *x_to_label(struct aa_profile *profile, if (xindex & AA_X_CHILD) /* released by caller */ new = find_attach(ns, &profile->base.profiles, - name); + name, info); else /* released by caller */ new = find_attach(ns, &ns->base.profiles, - name); + name, info); *lookupname = name; break; } @@ -516,7 +534,7 @@ static struct aa_label *profile_transition(struct aa_profile *profile, if (profile_unconfined(profile)) { new = find_attach(profile->ns, &profile->ns->base.profiles, - name); + name, &info); if (new) { AA_DEBUG("unconfined attached to new label"); return new; -- cgit From 06d426d113fe0b3107939e81db920ca7b097e97c Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 17 Nov 2017 18:04:37 -0800 Subject: apparmor: fix profile attachment for special unconfined profiles It used to be that unconfined would never attach. However that is not the case anymore as some special profiles can be marked as unconfined, that are not the namespaces unconfined profile, and may have an attachment. Fixes: f1bd904175e8 ("apparmor: add the base fns() for domain labels") Signed-off-by: John Johansen --- security/apparmor/domain.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 9527adc11c6d..ad456546df5b 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -325,8 +325,10 @@ static struct aa_profile *__attach_match(const char *name, struct aa_profile *profile, *candidate = NULL; list_for_each_entry_rcu(profile, head, base.list) { - if (profile->label.flags & FLAG_NULL) + if (profile->label.flags & FLAG_NULL && + &profile->label == ns_unconfined(profile->ns)) continue; + if (profile->xmatch) { if (profile->xmatch_len == len) { conflict = true; -- cgit From 5d7c44ef5e4f0149c9fb99faeae41e930485a1ec Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 20 Nov 2017 22:26:12 -0800 Subject: apparmor: fix locking when creating a new complain profile. Break the per cpu buffer atomic section when creating a new null complain profile. In learning mode this won't matter and we can safely re-aquire the buffer. This fixes the following lockdep BUG trace nov. 14 14:09:09 cyclope audit[7152]: AVC apparmor="ALLOWED" operation="exec" profile="/usr/sbin/sssd" name="/usr/sbin/adcli" pid=7152 comm="sssd_be" requested_mask="x" denied_mask="x" fsuid=0 ouid=0 target="/usr/sbin/sssd//null-/usr/sbin/adcli" nov. 14 14:09:09 cyclope kernel: BUG: sleeping function called from invalid context at kernel/locking/mutex.c:747 nov. 14 14:09:09 cyclope kernel: in_atomic(): 1, irqs_disabled(): 0, pid: 7152, name: sssd_be nov. 14 14:09:09 cyclope kernel: 1 lock held by sssd_be/7152: nov. 14 14:09:09 cyclope kernel: #0: (&sig->cred_guard_mutex){....}, at: [] prepare_bprm_creds+0x4e/0x100 nov. 14 14:09:09 cyclope kernel: CPU: 3 PID: 7152 Comm: sssd_be Not tainted 4.14.0prahal+intel #150 nov. 14 14:09:09 cyclope kernel: Hardware name: LENOVO 20CDCTO1WW/20CDCTO1WW, BIOS GQET53WW (1.33 ) 09/15/2017 nov. 14 14:09:09 cyclope kernel: Call Trace: nov. 14 14:09:09 cyclope kernel: dump_stack+0xb0/0x135 nov. 14 14:09:09 cyclope kernel: ? _atomic_dec_and_lock+0x15b/0x15b nov. 14 14:09:09 cyclope kernel: ? lockdep_print_held_locks+0xc4/0x130 nov. 14 14:09:09 cyclope kernel: ___might_sleep+0x29c/0x320 nov. 14 14:09:09 cyclope kernel: ? rq_clock+0xf0/0xf0 nov. 14 14:09:09 cyclope kernel: ? __kernel_text_address+0xd/0x40 nov. 14 14:09:09 cyclope kernel: __might_sleep+0x95/0x190 nov. 14 14:09:09 cyclope kernel: ? aa_new_null_profile+0x50a/0x960 nov. 14 14:09:09 cyclope kernel: __mutex_lock+0x13e/0x1a20 nov. 14 14:09:09 cyclope kernel: ? aa_new_null_profile+0x50a/0x960 nov. 14 14:09:09 cyclope kernel: ? save_stack+0x43/0xd0 nov. 14 14:09:09 cyclope kernel: ? kmem_cache_alloc_trace+0x13f/0x290 nov. 14 14:09:09 cyclope kernel: ? mutex_lock_io_nested+0x1880/0x1880 nov. 14 14:09:09 cyclope kernel: ? profile_transition+0x932/0x2d40 nov. 14 14:09:09 cyclope kernel: ? apparmor_bprm_set_creds+0x1479/0x1f70 nov. 14 14:09:09 cyclope kernel: ? security_bprm_set_creds+0x5a/0x80 nov. 14 14:09:09 cyclope kernel: ? prepare_binprm+0x366/0x980 nov. 14 14:09:09 cyclope kernel: ? do_execveat_common.isra.30+0x12a9/0x2350 nov. 14 14:09:09 cyclope kernel: ? SyS_execve+0x2c/0x40 nov. 14 14:09:09 cyclope kernel: ? do_syscall_64+0x228/0x650 nov. 14 14:09:09 cyclope kernel: ? entry_SYSCALL64_slow_path+0x25/0x25 nov. 14 14:09:09 cyclope kernel: ? deactivate_slab.isra.62+0x49d/0x5e0 nov. 14 14:09:09 cyclope kernel: ? save_stack_trace+0x16/0x20 nov. 14 14:09:09 cyclope kernel: ? init_object+0x88/0x90 nov. 14 14:09:09 cyclope kernel: ? ___slab_alloc+0x520/0x590 nov. 14 14:09:09 cyclope kernel: ? ___slab_alloc+0x520/0x590 nov. 14 14:09:09 cyclope kernel: ? aa_alloc_proxy+0xab/0x200 nov. 14 14:09:09 cyclope kernel: ? lock_downgrade+0x7e0/0x7e0 nov. 14 14:09:09 cyclope kernel: ? memcg_kmem_get_cache+0x970/0x970 nov. 14 14:09:09 cyclope kernel: ? kasan_unpoison_shadow+0x35/0x50 nov. 14 14:09:09 cyclope kernel: ? kasan_unpoison_shadow+0x35/0x50 nov. 14 14:09:09 cyclope kernel: ? kasan_kmalloc+0xad/0xe0 nov. 14 14:09:09 cyclope kernel: ? aa_alloc_proxy+0xab/0x200 nov. 14 14:09:09 cyclope kernel: ? kmem_cache_alloc_trace+0x13f/0x290 nov. 14 14:09:09 cyclope kernel: ? aa_alloc_proxy+0xab/0x200 nov. 14 14:09:09 cyclope kernel: ? aa_alloc_proxy+0xab/0x200 nov. 14 14:09:09 cyclope kernel: ? _raw_spin_unlock+0x22/0x30 nov. 14 14:09:09 cyclope kernel: ? vec_find+0xa0/0xa0 nov. 14 14:09:09 cyclope kernel: ? aa_label_init+0x6f/0x230 nov. 14 14:09:09 cyclope kernel: ? __label_insert+0x3e0/0x3e0 nov. 14 14:09:09 cyclope kernel: ? kmem_cache_alloc_trace+0x13f/0x290 nov. 14 14:09:09 cyclope kernel: ? aa_alloc_profile+0x58/0x200 nov. 14 14:09:09 cyclope kernel: mutex_lock_nested+0x16/0x20 nov. 14 14:09:09 cyclope kernel: ? mutex_lock_nested+0x16/0x20 nov. 14 14:09:09 cyclope kernel: aa_new_null_profile+0x50a/0x960 nov. 14 14:09:09 cyclope kernel: ? aa_fqlookupn_profile+0xdc0/0xdc0 nov. 14 14:09:09 cyclope kernel: ? aa_compute_fperms+0x4b5/0x640 nov. 14 14:09:09 cyclope kernel: ? disconnect.isra.2+0x1b0/0x1b0 nov. 14 14:09:09 cyclope kernel: ? aa_str_perms+0x8d/0xe0 nov. 14 14:09:09 cyclope kernel: profile_transition+0x932/0x2d40 nov. 14 14:09:09 cyclope kernel: ? up_read+0x1a/0x40 nov. 14 14:09:09 cyclope kernel: ? ext4_xattr_get+0x15c/0xaf0 [ext4] nov. 14 14:09:09 cyclope kernel: ? x_table_lookup+0x190/0x190 nov. 14 14:09:09 cyclope kernel: ? ext4_xattr_ibody_get+0x590/0x590 [ext4] nov. 14 14:09:09 cyclope kernel: ? sched_clock+0x9/0x10 nov. 14 14:09:09 cyclope kernel: ? sched_clock+0x9/0x10 nov. 14 14:09:09 cyclope kernel: ? ext4_xattr_security_get+0x1a/0x20 [ext4] nov. 14 14:09:09 cyclope kernel: ? __vfs_getxattr+0x6d/0xa0 nov. 14 14:09:09 cyclope kernel: ? get_vfs_caps_from_disk+0x114/0x720 nov. 14 14:09:09 cyclope kernel: ? sched_clock+0x9/0x10 nov. 14 14:09:09 cyclope kernel: ? sched_clock+0x9/0x10 nov. 14 14:09:09 cyclope kernel: ? tsc_resume+0x10/0x10 nov. 14 14:09:09 cyclope kernel: ? get_vfs_caps_from_disk+0x720/0x720 nov. 14 14:09:09 cyclope kernel: ? native_sched_clock_from_tsc+0x201/0x2b0 nov. 14 14:09:09 cyclope kernel: ? sched_clock+0x9/0x10 nov. 14 14:09:09 cyclope kernel: ? sched_clock_cpu+0x1b/0x170 nov. 14 14:09:09 cyclope kernel: ? find_held_lock+0x3c/0x1e0 nov. 14 14:09:09 cyclope kernel: ? rb_insert_color_cached+0x1660/0x1660 nov. 14 14:09:09 cyclope kernel: apparmor_bprm_set_creds+0x1479/0x1f70 nov. 14 14:09:09 cyclope kernel: ? sched_clock+0x9/0x10 nov. 14 14:09:09 cyclope kernel: ? handle_onexec+0x31d0/0x31d0 nov. 14 14:09:09 cyclope kernel: ? tsc_resume+0x10/0x10 nov. 14 14:09:09 cyclope kernel: ? graph_lock+0xd0/0xd0 nov. 14 14:09:09 cyclope kernel: ? tsc_resume+0x10/0x10 nov. 14 14:09:09 cyclope kernel: ? sched_clock_cpu+0x1b/0x170 nov. 14 14:09:09 cyclope kernel: ? sched_clock+0x9/0x10 nov. 14 14:09:09 cyclope kernel: ? sched_clock+0x9/0x10 nov. 14 14:09:09 cyclope kernel: ? sched_clock_cpu+0x1b/0x170 nov. 14 14:09:09 cyclope kernel: ? find_held_lock+0x3c/0x1e0 nov. 14 14:09:09 cyclope kernel: security_bprm_set_creds+0x5a/0x80 nov. 14 14:09:09 cyclope kernel: prepare_binprm+0x366/0x980 nov. 14 14:09:09 cyclope kernel: ? install_exec_creds+0x150/0x150 nov. 14 14:09:09 cyclope kernel: ? __might_fault+0x89/0xb0 nov. 14 14:09:09 cyclope kernel: ? up_read+0x40/0x40 nov. 14 14:09:09 cyclope kernel: ? get_user_arg_ptr.isra.18+0x2c/0x70 nov. 14 14:09:09 cyclope kernel: ? count.isra.20.constprop.32+0x7c/0xf0 nov. 14 14:09:09 cyclope kernel: do_execveat_common.isra.30+0x12a9/0x2350 nov. 14 14:09:09 cyclope kernel: ? prepare_bprm_creds+0x100/0x100 nov. 14 14:09:09 cyclope kernel: ? _raw_spin_unlock+0x22/0x30 nov. 14 14:09:09 cyclope kernel: ? deactivate_slab.isra.62+0x49d/0x5e0 nov. 14 14:09:09 cyclope kernel: ? save_stack_trace+0x16/0x20 nov. 14 14:09:09 cyclope kernel: ? init_object+0x88/0x90 nov. 14 14:09:09 cyclope kernel: ? ___slab_alloc+0x520/0x590 nov. 14 14:09:09 cyclope kernel: ? ___slab_alloc+0x520/0x590 nov. 14 14:09:09 cyclope kernel: ? kasan_check_write+0x14/0x20 nov. 14 14:09:09 cyclope kernel: ? memcg_kmem_get_cache+0x970/0x970 nov. 14 14:09:09 cyclope kernel: ? kasan_unpoison_shadow+0x35/0x50 nov. 14 14:09:09 cyclope kernel: ? glob_match+0x730/0x730 nov. 14 14:09:09 cyclope kernel: ? kmem_cache_alloc+0x225/0x280 nov. 14 14:09:09 cyclope kernel: ? getname_flags+0xb8/0x510 nov. 14 14:09:09 cyclope kernel: ? mm_fault_error+0x2e0/0x2e0 nov. 14 14:09:09 cyclope kernel: ? getname_flags+0xf6/0x510 nov. 14 14:09:09 cyclope kernel: ? ptregs_sys_vfork+0x10/0x10 nov. 14 14:09:09 cyclope kernel: SyS_execve+0x2c/0x40 nov. 14 14:09:09 cyclope kernel: do_syscall_64+0x228/0x650 nov. 14 14:09:09 cyclope kernel: ? syscall_return_slowpath+0x2f0/0x2f0 nov. 14 14:09:09 cyclope kernel: ? syscall_return_slowpath+0x167/0x2f0 nov. 14 14:09:09 cyclope kernel: ? prepare_exit_to_usermode+0x220/0x220 nov. 14 14:09:09 cyclope kernel: ? prepare_exit_to_usermode+0xda/0x220 nov. 14 14:09:09 cyclope kernel: ? perf_trace_sys_enter+0x1060/0x1060 nov. 14 14:09:09 cyclope kernel: ? __put_user_4+0x1c/0x30 nov. 14 14:09:09 cyclope kernel: entry_SYSCALL64_slow_path+0x25/0x25 nov. 14 14:09:09 cyclope kernel: RIP: 0033:0x7f9320f23637 nov. 14 14:09:09 cyclope kernel: RSP: 002b:00007fff783be338 EFLAGS: 00000202 ORIG_RAX: 000000000000003b nov. 14 14:09:09 cyclope kernel: RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f9320f23637 nov. 14 14:09:09 cyclope kernel: RDX: 0000558c35002a70 RSI: 0000558c3505bd10 RDI: 0000558c35018b90 nov. 14 14:09:09 cyclope kernel: RBP: 0000558c34b63ae8 R08: 0000558c3505bd10 R09: 0000000000000080 nov. 14 14:09:09 cyclope kernel: R10: 0000000000000095 R11: 0000000000000202 R12: 0000000000000001 nov. 14 14:09:09 cyclope kernel: R13: 0000558c35018b90 R14: 0000558c3505bd18 R15: 0000558c3505bd10 Fixes: 4227c333f65c ("apparmor: Move path lookup to using preallocated buffers") BugLink: http://bugs.launchpad.net/bugs/173228 Reported-by: Alban Browaeys Signed-off-by: John Johansen --- security/apparmor/domain.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index ad456546df5b..04ba9d0718ea 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -561,9 +561,21 @@ static struct aa_label *profile_transition(struct aa_profile *profile, } } else if (COMPLAIN_MODE(profile)) { /* no exec permission - learning mode */ - struct aa_profile *new_profile = aa_new_null_profile(profile, - false, name, - GFP_ATOMIC); + struct aa_profile *new_profile = NULL; + char *n = kstrdup(name, GFP_ATOMIC); + + if (n) { + /* name is ptr into buffer */ + long pos = name - buffer; + /* break per cpu buffer hold */ + put_buffers(buffer); + new_profile = aa_new_null_profile(profile, false, n, + GFP_KERNEL); + get_buffers(buffer); + name = buffer + pos; + strcpy((char *)name, n); + kfree(n); + } if (!new_profile) { error = -ENOMEM; info = "could not create null profile"; -- cgit From feb3c766a3ab32d233aaff7db13afd9ba5bc142d Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 20 Nov 2017 23:24:09 -0800 Subject: apparmor: fix possible recursive lock warning in __aa_create_ns Use mutex_lock_nested to provide lockdep the parent child lock ordering of the tree. This fixes the lockdep Warning [ 305.275177] ============================================ [ 305.275178] WARNING: possible recursive locking detected [ 305.275179] 4.14.0-rc7+ #320 Not tainted [ 305.275180] -------------------------------------------- [ 305.275181] apparmor_parser/1339 is trying to acquire lock: [ 305.275182] (&ns->lock){+.+.}, at: [] __aa_create_ns+0x6d/0x1e0 [ 305.275187] but task is already holding lock: [ 305.275187] (&ns->lock){+.+.}, at: [] aa_prepare_ns+0x3d/0xd0 [ 305.275190] other info that might help us debug this: [ 305.275191] Possible unsafe locking scenario: [ 305.275192] CPU0 [ 305.275193] ---- [ 305.275193] lock(&ns->lock); [ 305.275194] lock(&ns->lock); [ 305.275195] *** DEADLOCK *** [ 305.275196] May be due to missing lock nesting notation [ 305.275198] 2 locks held by apparmor_parser/1339: [ 305.275198] #0: (sb_writers#10){.+.+}, at: [] vfs_write+0x1a7/0x1d0 [ 305.275202] #1: (&ns->lock){+.+.}, at: [] aa_prepare_ns+0x3d/0xd0 [ 305.275205] stack backtrace: [ 305.275207] CPU: 1 PID: 1339 Comm: apparmor_parser Not tainted 4.14.0-rc7+ #320 [ 305.275208] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.1-1ubuntu1 04/01/2014 [ 305.275209] Call Trace: [ 305.275212] dump_stack+0x85/0xcb [ 305.275214] __lock_acquire+0x141c/0x1460 [ 305.275216] ? __aa_create_ns+0x6d/0x1e0 [ 305.275218] ? ___slab_alloc+0x183/0x540 [ 305.275219] ? ___slab_alloc+0x183/0x540 [ 305.275221] lock_acquire+0xed/0x1e0 [ 305.275223] ? lock_acquire+0xed/0x1e0 [ 305.275224] ? __aa_create_ns+0x6d/0x1e0 [ 305.275227] __mutex_lock+0x89/0x920 [ 305.275228] ? __aa_create_ns+0x6d/0x1e0 [ 305.275230] ? trace_hardirqs_on_caller+0x11f/0x190 [ 305.275231] ? __aa_create_ns+0x6d/0x1e0 [ 305.275233] ? __lockdep_init_map+0x57/0x1d0 [ 305.275234] ? lockdep_init_map+0x9/0x10 [ 305.275236] ? __rwlock_init+0x32/0x60 [ 305.275238] mutex_lock_nested+0x1b/0x20 [ 305.275240] ? mutex_lock_nested+0x1b/0x20 [ 305.275241] __aa_create_ns+0x6d/0x1e0 [ 305.275243] aa_prepare_ns+0xc2/0xd0 [ 305.275245] aa_replace_profiles+0x168/0xf30 [ 305.275247] ? __might_fault+0x85/0x90 [ 305.275250] policy_update+0xb9/0x380 [ 305.275252] profile_load+0x7e/0x90 [ 305.275254] __vfs_write+0x28/0x150 [ 305.275256] ? rcu_read_lock_sched_held+0x72/0x80 [ 305.275257] ? rcu_sync_lockdep_assert+0x2f/0x60 [ 305.275259] ? __sb_start_write+0xdc/0x1c0 [ 305.275261] ? vfs_write+0x1a7/0x1d0 [ 305.275262] vfs_write+0xca/0x1d0 [ 305.275264] ? trace_hardirqs_on_caller+0x11f/0x190 [ 305.275266] SyS_write+0x49/0xa0 [ 305.275268] entry_SYSCALL_64_fastpath+0x23/0xc2 [ 305.275271] RIP: 0033:0x7fa6b22e8c74 [ 305.275272] RSP: 002b:00007ffeaaee6288 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 [ 305.275273] RAX: ffffffffffffffda RBX: 00007ffeaaee62a4 RCX: 00007fa6b22e8c74 [ 305.275274] RDX: 0000000000000a51 RSI: 00005566a8198c10 RDI: 0000000000000004 [ 305.275275] RBP: 0000000000000a39 R08: 0000000000000a51 R09: 0000000000000000 [ 305.275276] R10: 0000000000000000 R11: 0000000000000246 R12: 00005566a8198c10 [ 305.275277] R13: 0000000000000004 R14: 00005566a72ecb88 R15: 00005566a72ec3a8 Fixes: 73688d1ed0b8 ("apparmor: refactor prepare_ns() and make usable from different views") Signed-off-by: John Johansen --- security/apparmor/apparmorfs.c | 22 +++++++++++----------- security/apparmor/label.c | 2 +- security/apparmor/policy.c | 8 ++++---- security/apparmor/policy_ns.c | 8 ++++---- security/apparmor/policy_unpack.c | 2 +- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index caaf51dda648..8542e9a55e1b 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -533,7 +533,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf, long last_read; int avail; - mutex_lock(&rev->ns->lock); + mutex_lock_nested(&rev->ns->lock, rev->ns->level); last_read = rev->last_read; if (last_read == rev->ns->revision) { mutex_unlock(&rev->ns->lock); @@ -543,7 +543,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf, last_read != READ_ONCE(rev->ns->revision))) return -ERESTARTSYS; - mutex_lock(&rev->ns->lock); + mutex_lock_nested(&rev->ns->lock, rev->ns->level); } avail = sprintf(buffer, "%ld\n", rev->ns->revision); @@ -577,7 +577,7 @@ static unsigned int ns_revision_poll(struct file *file, poll_table *pt) unsigned int mask = 0; if (rev) { - mutex_lock(&rev->ns->lock); + mutex_lock_nested(&rev->ns->lock, rev->ns->level); poll_wait(file, &rev->ns->wait, pt); if (rev->last_read < rev->ns->revision) mask |= POLLIN | POLLRDNORM; @@ -1643,7 +1643,7 @@ static int ns_mkdir_op(struct inode *dir, struct dentry *dentry, umode_t mode) */ inode_unlock(dir); error = simple_pin_fs(&aafs_ops, &aafs_mnt, &aafs_count); - mutex_lock(&parent->lock); + mutex_lock_nested(&parent->lock, parent->level); inode_lock_nested(dir, I_MUTEX_PARENT); if (error) goto out; @@ -1692,7 +1692,7 @@ static int ns_rmdir_op(struct inode *dir, struct dentry *dentry) inode_unlock(dir); inode_unlock(dentry->d_inode); - mutex_lock(&parent->lock); + mutex_lock_nested(&parent->lock, parent->level); ns = aa_get_ns(__aa_findn_ns(&parent->sub_ns, dentry->d_name.name, dentry->d_name.len)); if (!ns) { @@ -1747,7 +1747,7 @@ void __aafs_ns_rmdir(struct aa_ns *ns) __aafs_profile_rmdir(child); list_for_each_entry(sub, &ns->sub_ns, base.list) { - mutex_lock(&sub->lock); + mutex_lock_nested(&sub->lock, sub->level); __aafs_ns_rmdir(sub); mutex_unlock(&sub->lock); } @@ -1877,7 +1877,7 @@ int __aafs_ns_mkdir(struct aa_ns *ns, struct dentry *parent, const char *name, /* subnamespaces */ list_for_each_entry(sub, &ns->sub_ns, base.list) { - mutex_lock(&sub->lock); + mutex_lock_nested(&sub->lock, sub->level); error = __aafs_ns_mkdir(sub, ns_subns_dir(ns), NULL, NULL); mutex_unlock(&sub->lock); if (error) @@ -1921,7 +1921,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns) /* is next namespace a child */ if (!list_empty(&ns->sub_ns)) { next = list_first_entry(&ns->sub_ns, typeof(*ns), base.list); - mutex_lock(&next->lock); + mutex_lock_nested(&next->lock, next->level); return next; } @@ -1931,7 +1931,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns) mutex_unlock(&ns->lock); next = list_next_entry(ns, base.list); if (!list_entry_is_head(next, &parent->sub_ns, base.list)) { - mutex_lock(&next->lock); + mutex_lock_nested(&next->lock, next->level); return next; } ns = parent; @@ -2039,7 +2039,7 @@ static void *p_start(struct seq_file *f, loff_t *pos) f->private = root; /* find the first profile */ - mutex_lock(&root->lock); + mutex_lock_nested(&root->lock, root->level); profile = __first_profile(root, root); /* skip to position */ @@ -2491,7 +2491,7 @@ static int __init aa_create_aafs(void) ns_subrevision(root_ns) = dent; /* policy tree referenced by magic policy symlink */ - mutex_lock(&root_ns->lock); + mutex_lock_nested(&root_ns->lock, root_ns->level); error = __aafs_ns_mkdir(root_ns, aafs_mnt->mnt_root, ".policy", aafs_mnt->mnt_root); mutex_unlock(&root_ns->lock); diff --git a/security/apparmor/label.c b/security/apparmor/label.c index c5b99b954580..f5df9eaba4df 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -2115,7 +2115,7 @@ void __aa_labelset_update_subtree(struct aa_ns *ns) __labelset_update(ns); list_for_each_entry(child, &ns->sub_ns, base.list) { - mutex_lock(&child->lock); + mutex_lock_nested(&child->lock, child->level); __aa_labelset_update_subtree(child); mutex_unlock(&child->lock); } diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 586b249d3b46..b0b58848c248 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -545,7 +545,7 @@ name: profile->file.dfa = aa_get_dfa(nulldfa); profile->policy.dfa = aa_get_dfa(nulldfa); - mutex_lock(&profile->ns->lock); + mutex_lock_nested(&profile->ns->lock, profile->ns->level); p = __find_child(&parent->base.profiles, bname); if (p) { aa_free_profile(profile); @@ -906,7 +906,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label, } else ns = aa_get_ns(policy_ns ? policy_ns : labels_ns(label)); - mutex_lock(&ns->lock); + mutex_lock_nested(&ns->lock, ns->level); /* check for duplicate rawdata blobs: space and file dedup */ list_for_each_entry(rawdata_ent, &ns->rawdata_list, list) { if (aa_rawdata_eq(rawdata_ent, udata)) { @@ -1117,13 +1117,13 @@ ssize_t aa_remove_profiles(struct aa_ns *policy_ns, struct aa_label *subj, if (!name) { /* remove namespace - can only happen if fqname[0] == ':' */ - mutex_lock(&ns->parent->lock); + mutex_lock_nested(&ns->parent->lock, ns->level); __aa_remove_ns(ns); __aa_bump_ns_revision(ns); mutex_unlock(&ns->parent->lock); } else { /* remove profile */ - mutex_lock(&ns->lock); + mutex_lock_nested(&ns->lock, ns->level); profile = aa_get_profile(__lookup_profile(&ns->base, name)); if (!profile) { error = -ENOENT; diff --git a/security/apparmor/policy_ns.c b/security/apparmor/policy_ns.c index 62a3589c62ab..b1e629cba70b 100644 --- a/security/apparmor/policy_ns.c +++ b/security/apparmor/policy_ns.c @@ -256,7 +256,8 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name, ns = alloc_ns(parent->base.hname, name); if (!ns) return NULL; - mutex_lock(&ns->lock); + ns->level = parent->level + 1; + mutex_lock_nested(&ns->lock, ns->level); error = __aafs_ns_mkdir(ns, ns_subns_dir(parent), name, dir); if (error) { AA_ERROR("Failed to create interface for ns %s\n", @@ -266,7 +267,6 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name, return ERR_PTR(error); } ns->parent = aa_get_ns(parent); - ns->level = parent->level + 1; list_add_rcu(&ns->base.list, &parent->sub_ns); /* add list ref */ aa_get_ns(ns); @@ -313,7 +313,7 @@ struct aa_ns *aa_prepare_ns(struct aa_ns *parent, const char *name) { struct aa_ns *ns; - mutex_lock(&parent->lock); + mutex_lock_nested(&parent->lock, parent->level); /* try and find the specified ns and if it doesn't exist create it */ /* released by caller */ ns = aa_get_ns(__aa_find_ns(&parent->sub_ns, name)); @@ -336,7 +336,7 @@ static void destroy_ns(struct aa_ns *ns) if (!ns) return; - mutex_lock(&ns->lock); + mutex_lock_nested(&ns->lock, ns->level); /* release all profiles in this namespace */ __aa_profile_list_release(&ns->base.profiles); diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 4ede87c30f8b..59a1a25b7d43 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -157,7 +157,7 @@ static void do_loaddata_free(struct work_struct *work) struct aa_ns *ns = aa_get_ns(d->ns); if (ns) { - mutex_lock(&ns->lock); + mutex_lock_nested(&ns->lock, ns->level); __aa_fs_remove_rawdata(d); mutex_unlock(&ns->lock); aa_put_ns(ns); -- cgit