From 45525b26a46cd593cb72070304c4cd7c8391bd37 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 16 Oct 2012 13:30:07 -0400 Subject: fix a leak in replace_fd() users replace_fd() began with "eats a reference, tries to insert into descriptor table" semantics; at some point I'd switched it to much saner current behaviour ("try to insert into descriptor table, grabbing a new reference if inserted; caller should do fput() in any case"), but forgot to update the callers. Mea culpa... [Spotted by Pavel Roskin, who has really weird system with pipe-fed coredumps as part of what he considers a normal boot ;-)] Signed-off-by: Al Viro --- security/selinux/hooks.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) (limited to 'security') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 24ab4148547..61a53367d02 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2132,18 +2132,14 @@ static inline void flush_unauthorized_files(const struct cred *cred, return; devnull = dentry_open(&selinux_null, O_RDWR, cred); - if (!IS_ERR(devnull)) { - /* replace all the matching ones with this */ - do { - replace_fd(n - 1, get_file(devnull), 0); - } while ((n = iterate_fd(files, n, match_file, cred)) != 0); + if (IS_ERR(devnull)) + devnull = NULL; + /* replace all the matching ones with this */ + do { + replace_fd(n - 1, devnull, 0); + } while ((n = iterate_fd(files, n, match_file, cred)) != 0); + if (devnull) fput(devnull); - } else { - /* just close all the matching ones */ - do { - replace_fd(n - 1, NULL, 0); - } while ((n = iterate_fd(files, n, match_file, cred)) != 0); - } } /* -- cgit v1.2.3-70-g09d2 From 43c422eda99b894f18d1cca17bcd2401efaf7bd0 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 17 Oct 2012 13:29:33 -0700 Subject: apparmor: fix apparmor OOPS in audit_log_untrustedstring+0x1c/0x40 The capability defines have moved causing the auto generated names of capabilities that apparmor uses in logging to be incorrect. Fix the autogenerated table source to uapi/linux/capability.h Reported-by: YanHong Reported-by: Krzysztof Kolasa Analyzed-by: Al Viro Signed-off-by: John Johansen Acked-by: David Howells Acked-by: James Morris Signed-off-by: Linus Torvalds --- security/apparmor/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/Makefile b/security/apparmor/Makefile index 7b3021cebbe..5706b74c857 100644 --- a/security/apparmor/Makefile +++ b/security/apparmor/Makefile @@ -57,7 +57,7 @@ cmd_make-rlim = echo "static const char *const rlim_names[RLIM_NLIMITS] = {" \ $(obj)/capability.o : $(obj)/capability_names.h $(obj)/resource.o : $(obj)/rlim_names.h -$(obj)/capability_names.h : $(srctree)/include/linux/capability.h \ +$(obj)/capability_names.h : $(srctree)/include/uapi/linux/capability.h \ $(src)/Makefile $(call cmd,make-caps) $(obj)/rlim_names.h : $(srctree)/include/uapi/asm-generic/resource.h \ -- cgit v1.2.3-70-g09d2 From 2e680dd61e80592385338bfbeb86833d1c60546c Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 24 Oct 2012 06:27:32 -0700 Subject: apparmor: fix IRQ stack overflow during free_profile BugLink: http://bugs.launchpad.net/bugs/1056078 Profile replacement can cause long chains of profiles to build up when the profile being replaced is pinned. When the pinned profile is finally freed, it puts the reference to its replacement, which may in turn nest another call to free_profile on the stack. Because this may happen for each profile in the replacedby chain this can result in a recusion that causes the stack to overflow. Break this nesting by directly walking the chain of replacedby profiles (ie. use iteration instead of recursion to free the list). This results in at most 2 levels of free_profile being called, while freeing a replacedby chain. Signed-off-by: John Johansen Signed-off-by: James Morris --- security/apparmor/policy.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index cf5fd220309..813200384d9 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -724,6 +724,8 @@ fail: */ static void free_profile(struct aa_profile *profile) { + struct aa_profile *p; + AA_DEBUG("%s(%p)\n", __func__, profile); if (!profile) @@ -751,7 +753,27 @@ static void free_profile(struct aa_profile *profile) aa_put_dfa(profile->xmatch); aa_put_dfa(profile->policy.dfa); - aa_put_profile(profile->replacedby); + /* put the profile reference for replacedby, but not via + * put_profile(kref_put). + * replacedby can form a long chain that can result in cascading + * frees that blows the stack because kref_put makes a nested fn + * call (it looks like recursion, with free_profile calling + * free_profile) for each profile in the chain lp#1056078. + */ + for (p = profile->replacedby; p; ) { + if (atomic_dec_and_test(&p->base.count.refcount)) { + /* no more refs on p, grab its replacedby */ + struct aa_profile *next = p->replacedby; + /* break the chain */ + p->replacedby = NULL; + /* now free p, chain is broken */ + free_profile(p); + + /* follow up with next profile in the chain */ + p = next; + } else + break; + } kzfree(profile); } -- cgit v1.2.3-70-g09d2 From 8c9506d16925f1b1314d93af383ca3134eb534d8 Mon Sep 17 00:00:00 2001 From: Jiri Slaby Date: Thu, 25 Oct 2012 13:37:34 -0700 Subject: cgroup: fix invalid rcu dereference Commit ad676077a2ae ("device_cgroup: convert device_cgroup internally to policy + exceptions") removed rcu locks which are needed in task_devcgroup called in this chain: devcgroup_inode_mknod OR __devcgroup_inode_permission -> __devcgroup_inode_permission -> task_devcgroup -> task_subsys_state -> task_subsys_state_check. Change the code so that task_devcgroup is safely called with rcu read lock held. =============================== [ INFO: suspicious RCU usage. ] 3.6.0-rc5-next-20120913+ #42 Not tainted ------------------------------- include/linux/cgroup.h:553 suspicious rcu_dereference_check() usage! other info that might help us debug this: rcu_scheduler_active = 1, debug_locks = 0 2 locks held by kdevtmpfs/23: #0: (sb_writers){.+.+.+}, at: [] mnt_want_write+0x1f/0x50 #1: (&sb->s_type->i_mutex_key#3/1){+.+.+.}, at: [] kern_path_create+0x7f/0x170 stack backtrace: Pid: 23, comm: kdevtmpfs Not tainted 3.6.0-rc5-next-20120913+ #42 Call Trace: lockdep_rcu_suspicious+0xfd/0x130 devcgroup_inode_mknod+0x19d/0x240 vfs_mknod+0x71/0xf0 handle_create.isra.2+0x72/0x200 devtmpfsd+0x114/0x140 ? handle_create.isra.2+0x200/0x200 kthread+0xd6/0xe0 kernel_thread_helper+0x4/0x10 Signed-off-by: Jiri Slaby Cc: Dave Jones Cc: Tejun Heo Cc: Li Zefan Cc: James Morris Cc: Pavel Emelyanov Acked-by: Serge Hallyn Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- security/device_cgroup.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) (limited to 'security') diff --git a/security/device_cgroup.c b/security/device_cgroup.c index 44dfc415a37..46d01fcc0d1 100644 --- a/security/device_cgroup.c +++ b/security/device_cgroup.c @@ -533,10 +533,10 @@ struct cgroup_subsys devices_subsys = { * * returns 0 on success, -EPERM case the operation is not permitted */ -static int __devcgroup_check_permission(struct dev_cgroup *dev_cgroup, - short type, u32 major, u32 minor, +static int __devcgroup_check_permission(short type, u32 major, u32 minor, short access) { + struct dev_cgroup *dev_cgroup; struct dev_exception_item ex; int rc; @@ -547,6 +547,7 @@ static int __devcgroup_check_permission(struct dev_cgroup *dev_cgroup, ex.access = access; rcu_read_lock(); + dev_cgroup = task_devcgroup(current); rc = may_access(dev_cgroup, &ex); rcu_read_unlock(); @@ -558,7 +559,6 @@ static int __devcgroup_check_permission(struct dev_cgroup *dev_cgroup, int __devcgroup_inode_permission(struct inode *inode, int mask) { - struct dev_cgroup *dev_cgroup = task_devcgroup(current); short type, access = 0; if (S_ISBLK(inode->i_mode)) @@ -570,13 +570,12 @@ int __devcgroup_inode_permission(struct inode *inode, int mask) if (mask & MAY_READ) access |= ACC_READ; - return __devcgroup_check_permission(dev_cgroup, type, imajor(inode), - iminor(inode), access); + return __devcgroup_check_permission(type, imajor(inode), iminor(inode), + access); } int devcgroup_inode_mknod(int mode, dev_t dev) { - struct dev_cgroup *dev_cgroup = task_devcgroup(current); short type; if (!S_ISBLK(mode) && !S_ISCHR(mode)) @@ -587,7 +586,7 @@ int devcgroup_inode_mknod(int mode, dev_t dev) else type = DEV_CHAR; - return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev), - MINOR(dev), ACC_MKNOD); + return __devcgroup_check_permission(type, MAJOR(dev), MINOR(dev), + ACC_MKNOD); } -- cgit v1.2.3-70-g09d2 From 5b7aa7d5bb2c5cf7fc05aaa41561af321706ab5f Mon Sep 17 00:00:00 2001 From: Aristeu Rozanski Date: Thu, 25 Oct 2012 13:37:38 -0700 Subject: device_cgroup: rename deny_all to behavior This was done in a v2 patch but v1 ended up being committed. The variable name is less confusing and stores the default behavior when no matching exception exists. Signed-off-by: Aristeu Rozanski Cc: Dave Jones Cc: Tejun Heo Cc: Li Zefan Cc: James Morris Cc: Pavel Emelyanov Acked-by: Serge Hallyn Cc: Jiri Slaby Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- security/device_cgroup.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) (limited to 'security') diff --git a/security/device_cgroup.c b/security/device_cgroup.c index 46d01fcc0d1..76503df2377 100644 --- a/security/device_cgroup.c +++ b/security/device_cgroup.c @@ -42,7 +42,10 @@ struct dev_exception_item { struct dev_cgroup { struct cgroup_subsys_state css; struct list_head exceptions; - bool deny_all; + enum { + DEVCG_DEFAULT_ALLOW, + DEVCG_DEFAULT_DENY, + } behavior; }; static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s) @@ -182,13 +185,13 @@ static struct cgroup_subsys_state *devcgroup_create(struct cgroup *cgroup) parent_cgroup = cgroup->parent; if (parent_cgroup == NULL) - dev_cgroup->deny_all = false; + dev_cgroup->behavior = DEVCG_DEFAULT_ALLOW; else { parent_dev_cgroup = cgroup_to_devcgroup(parent_cgroup); mutex_lock(&devcgroup_mutex); ret = dev_exceptions_copy(&dev_cgroup->exceptions, &parent_dev_cgroup->exceptions); - dev_cgroup->deny_all = parent_dev_cgroup->deny_all; + dev_cgroup->behavior = parent_dev_cgroup->behavior; mutex_unlock(&devcgroup_mutex); if (ret) { kfree(dev_cgroup); @@ -260,7 +263,7 @@ static int devcgroup_seq_read(struct cgroup *cgroup, struct cftype *cft, * - List the exceptions in case the default policy is to deny * This way, the file remains as a "whitelist of devices" */ - if (devcgroup->deny_all == false) { + if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) { set_access(acc, ACC_MASK); set_majmin(maj, ~0); set_majmin(min, ~0); @@ -314,12 +317,12 @@ static int may_access(struct dev_cgroup *dev_cgroup, * In two cases we'll consider this new exception valid: * - the dev cgroup has its default policy to allow + exception list: * the new exception should *not* match any of the exceptions - * (!deny_all, !match) + * (behavior == DEVCG_DEFAULT_ALLOW, !match) * - the dev cgroup has its default policy to deny + exception list: * the new exception *should* match the exceptions - * (deny_all, match) + * (behavior == DEVCG_DEFAULT_DENY, match) */ - if (dev_cgroup->deny_all == match) + if ((dev_cgroup->behavior == DEVCG_DEFAULT_DENY) == match) return 1; return 0; } @@ -375,11 +378,11 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, if (!parent_has_perm(devcgroup, &ex)) return -EPERM; dev_exception_clean(devcgroup); - devcgroup->deny_all = false; + devcgroup->behavior = DEVCG_DEFAULT_ALLOW; break; case DEVCG_DENY: dev_exception_clean(devcgroup); - devcgroup->deny_all = true; + devcgroup->behavior = DEVCG_DEFAULT_DENY; break; default: return -EINVAL; @@ -452,7 +455,7 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, * an matching exception instead. And be silent about it: we * don't want to break compatibility */ - if (devcgroup->deny_all == false) { + if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) { dev_exception_rm(devcgroup, &ex); return 0; } @@ -463,7 +466,7 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, * an matching exception instead. And be silent about it: we * don't want to break compatibility */ - if (devcgroup->deny_all == true) { + if (devcgroup->behavior == DEVCG_DEFAULT_DENY) { dev_exception_rm(devcgroup, &ex); return 0; } -- cgit v1.2.3-70-g09d2 From 26fd8405dd470cb8b54cb96859b7dd437e5e1391 Mon Sep 17 00:00:00 2001 From: Aristeu Rozanski Date: Thu, 25 Oct 2012 13:37:41 -0700 Subject: device_cgroup: stop using simple_strtoul() Convert the code to use kstrtou32() instead of simple_strtoul() which is deprecated. The real size of the variables are u32, so use kstrtou32 instead of kstrtoul Signed-off-by: Aristeu Rozanski Cc: Dave Jones Cc: Tejun Heo Cc: Li Zefan Cc: James Morris Cc: Pavel Emelyanov Acked-by: Serge Hallyn Cc: Jiri Slaby Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- security/device_cgroup.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) (limited to 'security') diff --git a/security/device_cgroup.c b/security/device_cgroup.c index 76503df2377..4fbae8d0b36 100644 --- a/security/device_cgroup.c +++ b/security/device_cgroup.c @@ -361,8 +361,8 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, int filetype, const char *buffer) { const char *b; - char *endp; - int count; + char temp[12]; /* 11 + 1 characters needed for a u32 */ + int count, rc; struct dev_exception_item ex; if (!capable(CAP_SYS_ADMIN)) @@ -405,8 +405,16 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, ex.major = ~0; b++; } else if (isdigit(*b)) { - ex.major = simple_strtoul(b, &endp, 10); - b = endp; + memset(temp, 0, sizeof(temp)); + for (count = 0; count < sizeof(temp) - 1; count++) { + temp[count] = *b; + b++; + if (!isdigit(*b)) + break; + } + rc = kstrtou32(temp, 10, &ex.major); + if (rc) + return -EINVAL; } else { return -EINVAL; } @@ -419,8 +427,16 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, ex.minor = ~0; b++; } else if (isdigit(*b)) { - ex.minor = simple_strtoul(b, &endp, 10); - b = endp; + memset(temp, 0, sizeof(temp)); + for (count = 0; count < sizeof(temp) - 1; count++) { + temp[count] = *b; + b++; + if (!isdigit(*b)) + break; + } + rc = kstrtou32(temp, 10, &ex.minor); + if (rc) + return -EINVAL; } else { return -EINVAL; } -- cgit v1.2.3-70-g09d2 From 4cef7299b4786879a3e113e84084a72b24590c5b Mon Sep 17 00:00:00 2001 From: Aristeu Rozanski Date: Thu, 25 Oct 2012 13:37:45 -0700 Subject: device_cgroup: add proper checking when changing default behavior Before changing a group's default behavior to ALLOW, we must check if its parent's behavior is also ALLOW. Signed-off-by: Aristeu Rozanski Cc: Tejun Heo Cc: Li Zefan Cc: James Morris Cc: Pavel Emelyanov Acked-by: Serge Hallyn Cc: Jiri Slaby Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- security/device_cgroup.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/device_cgroup.c b/security/device_cgroup.c index 4fbae8d0b36..842c254396d 100644 --- a/security/device_cgroup.c +++ b/security/device_cgroup.c @@ -344,6 +344,17 @@ static int parent_has_perm(struct dev_cgroup *childcg, return may_access(parent, ex); } +/** + * may_allow_all - checks if it's possible to change the behavior to + * allow based on parent's rules. + * @parent: device cgroup's parent + * returns: != 0 in case it's allowed, 0 otherwise + */ +static inline int may_allow_all(struct dev_cgroup *parent) +{ + return parent->behavior == DEVCG_DEFAULT_ALLOW; +} + /* * Modify the exception list using allow/deny rules. * CAP_SYS_ADMIN is needed for this. It's at least separate from CAP_MKNOD @@ -364,6 +375,8 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, char temp[12]; /* 11 + 1 characters needed for a u32 */ int count, rc; struct dev_exception_item ex; + struct cgroup *p = devcgroup->css.cgroup; + struct dev_cgroup *parent = cgroup_to_devcgroup(p->parent); if (!capable(CAP_SYS_ADMIN)) return -EPERM; @@ -375,9 +388,13 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, case 'a': switch (filetype) { case DEVCG_ALLOW: - if (!parent_has_perm(devcgroup, &ex)) + if (!may_allow_all(parent)) return -EPERM; dev_exception_clean(devcgroup); + rc = dev_exceptions_copy(&devcgroup->exceptions, + &parent->exceptions); + if (rc) + return rc; devcgroup->behavior = DEVCG_DEFAULT_ALLOW; break; case DEVCG_DENY: -- cgit v1.2.3-70-g09d2 From 64e104771351d365e51e588a0e9a656ae6ed2f50 Mon Sep 17 00:00:00 2001 From: Aristeu Rozanski Date: Tue, 6 Nov 2012 07:25:04 -0800 Subject: device_cgroup: fix unchecked cgroup parent usage In 4cef7299b478687 ("device_cgroup: add proper checking when changing default behavior") the cgroup parent usage is unchecked. root will not have a parent and trying to use device.{allow,deny} will cause problems. For some reason my stressing scripts didn't test the root directory so I didn't catch it on my regular tests. Signed-off-by: Aristeu Rozanski Cc: Li Zefan Cc: James Morris Cc: Pavel Emelyanov Acked-by: Serge E. Hallyn Cc: Jiri Slaby Cc: Tejun Heo Signed-off-by: Tejun Heo --- security/device_cgroup.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/device_cgroup.c b/security/device_cgroup.c index 842c254396d..96d87eab166 100644 --- a/security/device_cgroup.c +++ b/security/device_cgroup.c @@ -352,6 +352,8 @@ static int parent_has_perm(struct dev_cgroup *childcg, */ static inline int may_allow_all(struct dev_cgroup *parent) { + if (!parent) + return 1; return parent->behavior == DEVCG_DEFAULT_ALLOW; } @@ -376,11 +378,14 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, int count, rc; struct dev_exception_item ex; struct cgroup *p = devcgroup->css.cgroup; - struct dev_cgroup *parent = cgroup_to_devcgroup(p->parent); + struct dev_cgroup *parent = NULL; if (!capable(CAP_SYS_ADMIN)) return -EPERM; + if (p->parent) + parent = cgroup_to_devcgroup(p->parent); + memset(&ex, 0, sizeof(ex)); b = buffer; @@ -391,11 +396,14 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, if (!may_allow_all(parent)) return -EPERM; dev_exception_clean(devcgroup); + devcgroup->behavior = DEVCG_DEFAULT_ALLOW; + if (!parent) + break; + rc = dev_exceptions_copy(&devcgroup->exceptions, &parent->exceptions); if (rc) return rc; - devcgroup->behavior = DEVCG_DEFAULT_ALLOW; break; case DEVCG_DENY: dev_exception_clean(devcgroup); -- cgit v1.2.3-70-g09d2 From 201e72acb2d3821e2de9ce6091e98859c316b29a Mon Sep 17 00:00:00 2001 From: Tejun Heo Date: Tue, 6 Nov 2012 09:17:37 -0800 Subject: device_cgroup: fix RCU usage dev_cgroup->exceptions is protected with devcgroup_mutex for writes and RCU for reads; however, RCU usage isn't correct. * dev_exception_clean() doesn't use RCU variant of list_del() and kfree(). The function can race with may_access() and may_access() may end up dereferencing already freed memory. Use list_del_rcu() and kfree_rcu() instead. * may_access() may be called only with RCU read locked but doesn't use RCU safe traversal over ->exceptions. Use list_for_each_entry_rcu(). Signed-off-by: Tejun Heo Acked-by: Serge E. Hallyn Cc: stable@vger.kernel.org Cc: Aristeu Rozanski Cc: Li Zefan --- security/device_cgroup.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'security') diff --git a/security/device_cgroup.c b/security/device_cgroup.c index 96d87eab166..b08d20c66c2 100644 --- a/security/device_cgroup.c +++ b/security/device_cgroup.c @@ -164,8 +164,8 @@ static void dev_exception_clean(struct dev_cgroup *dev_cgroup) struct dev_exception_item *ex, *tmp; list_for_each_entry_safe(ex, tmp, &dev_cgroup->exceptions, list) { - list_del(&ex->list); - kfree(ex); + list_del_rcu(&ex->list); + kfree_rcu(ex, rcu); } } @@ -298,7 +298,7 @@ static int may_access(struct dev_cgroup *dev_cgroup, struct dev_exception_item *ex; bool match = false; - list_for_each_entry(ex, &dev_cgroup->exceptions, list) { + list_for_each_entry_rcu(ex, &dev_cgroup->exceptions, list) { if ((refex->type & DEV_BLOCK) && !(ex->type & DEV_BLOCK)) continue; if ((refex->type & DEV_CHAR) && !(ex->type & DEV_CHAR)) -- cgit v1.2.3-70-g09d2