From e819ff519b2d74373eca4a9a2b417ebf4c1e1b29 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 27 Aug 2010 18:33:26 -0700 Subject: [PATCH 1/5] AppArmor: Drop hack to remove appended " (deleted)" string The 2.6.36 kernel has refactored __d_path() so that it no longer appends " (deleted)" to unlinked paths. So drop the hack that was used to detect and remove the appended string. Signed-off-by: John Johansen Signed-off-by: James Morris --- security/apparmor/path.c | 38 +++++++++++--------------------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/security/apparmor/path.c b/security/apparmor/path.c index 19358dc14605b..82396050f1864 100644 --- a/security/apparmor/path.c +++ b/security/apparmor/path.c @@ -59,8 +59,7 @@ static int d_namespace_path(struct path *path, char *buf, int buflen, { struct path root, tmp; char *res; - int deleted, connected; - int error = 0; + int connected, error = 0; /* Get the root we want to resolve too, released below */ if (flags & PATH_CHROOT_REL) { @@ -74,19 +73,8 @@ static int d_namespace_path(struct path *path, char *buf, int buflen, } spin_lock(&dcache_lock); - /* There is a race window between path lookup here and the - * need to strip the " (deleted) string that __d_path applies - * Detect the race and relookup the path - * - * The stripping of (deleted) is a hack that could be removed - * with an updated __d_path - */ - do { - tmp = root; - deleted = d_unlinked(path->dentry); - res = __d_path(path, &tmp, buf, buflen); - - } while (deleted != d_unlinked(path->dentry)); + tmp = root; + res = __d_path(path, &tmp, buf, buflen); spin_unlock(&dcache_lock); *name = res; @@ -98,21 +86,17 @@ static int d_namespace_path(struct path *path, char *buf, int buflen, *name = buf; goto out; } - if (deleted) { - /* On some filesystems, newly allocated dentries appear to the - * security_path hooks as a deleted dentry except without an - * inode allocated. - * - * Remove the appended deleted text and return as string for - * normal mediation, or auditing. The (deleted) string is - * guaranteed to be added in this case, so just strip it. - */ - buf[buflen - 11] = 0; /* - (len(" (deleted)") +\0) */ - if (path->dentry->d_inode && !(flags & PATH_MEDIATE_DELETED)) { + /* Handle two cases: + * 1. A deleted dentry && profile is not allowing mediation of deleted + * 2. On some filesystems, newly allocated dentries appear to the + * security_path hooks as a deleted dentry except without an inode + * allocated. + */ + if (d_unlinked(path->dentry) && path->dentry->d_inode && + !(flags & PATH_MEDIATE_DELETED)) { error = -ENOENT; goto out; - } } /* Determine if the path is connected to the expected root */ From 3a2dc8382a3e85a51ed9c6f57ea80665ea7a0c95 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 6 Sep 2010 10:10:20 -0700 Subject: [PATCH 2/5] AppArmor: Fix security_task_setrlimit logic for 2.6.36 changes 2.6.36 introduced the abilitiy to specify the task that is having its rlimits set. Update mediation to ensure that confined tasks can only set their own group_leader as expected by current policy. Add TODO note about extending policy to support setting other tasks rlimits. Signed-off-by: John Johansen Signed-off-by: James Morris --- security/apparmor/include/resource.h | 4 ++-- security/apparmor/lsm.c | 2 +- security/apparmor/resource.c | 20 ++++++++++++-------- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/security/apparmor/include/resource.h b/security/apparmor/include/resource.h index 3c88be9464940..02baec732bb51 100644 --- a/security/apparmor/include/resource.h +++ b/security/apparmor/include/resource.h @@ -33,8 +33,8 @@ struct aa_rlimit { }; int aa_map_resource(int resource); -int aa_task_setrlimit(struct aa_profile *profile, unsigned int resource, - struct rlimit *new_rlim); +int aa_task_setrlimit(struct aa_profile *profile, struct task_struct *, + unsigned int resource, struct rlimit *new_rlim); void __aa_transition_rlimits(struct aa_profile *old, struct aa_profile *new); diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index f73e2c2042185..cf1de4462ccd3 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -614,7 +614,7 @@ static int apparmor_task_setrlimit(struct task_struct *task, int error = 0; if (!unconfined(profile)) - error = aa_task_setrlimit(profile, resource, new_rlim); + error = aa_task_setrlimit(profile, task, resource, new_rlim); return error; } diff --git a/security/apparmor/resource.c b/security/apparmor/resource.c index 4a368f1fd36dd..a4136c10b1c62 100644 --- a/security/apparmor/resource.c +++ b/security/apparmor/resource.c @@ -72,6 +72,7 @@ int aa_map_resource(int resource) /** * aa_task_setrlimit - test permission to set an rlimit * @profile - profile confining the task (NOT NULL) + * @task - task the resource is being set on * @resource - the resource being set * @new_rlim - the new resource limit (NOT NULL) * @@ -79,18 +80,21 @@ int aa_map_resource(int resource) * * Returns: 0 or error code if setting resource failed */ -int aa_task_setrlimit(struct aa_profile *profile, unsigned int resource, - struct rlimit *new_rlim) +int aa_task_setrlimit(struct aa_profile *profile, struct task_struct *task, + unsigned int resource, struct rlimit *new_rlim) { int error = 0; - if (profile->rlimits.mask & (1 << resource) && - new_rlim->rlim_max > profile->rlimits.limits[resource].rlim_max) - - error = audit_resource(profile, resource, new_rlim->rlim_max, - -EACCES); + /* TODO: extend resource control to handle other (non current) + * processes. AppArmor rules currently have the implicit assumption + * that the task is setting the resource of the current process + */ + if ((task != current->group_leader) || + (profile->rlimits.mask & (1 << resource) && + new_rlim->rlim_max > profile->rlimits.limits[resource].rlim_max)) + error = -EACCES; - return error; + return audit_resource(profile, resource, new_rlim->rlim_max, error); } /** From 04ccd53f09741c4bc54ab36db000bc1383e4812e Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 27 Aug 2010 18:33:28 -0700 Subject: [PATCH 3/5] AppArmor: Fix splitting an fqname into separate namespace and profile names As per Dan Carpenter If we have a ns name without a following profile then in the original code it did "*ns_name = &name[1];". "name" is NULL so "*ns_name" is 0x1. That isn't useful and could cause an oops when this function is called from aa_remove_profiles(). Beyond this the assignment of the namespace name was wrong in the case where the profile name was provided as it was being set to &name[1] after name = skip_spaces(split + 1); Move the ns_name assignment before updating name for the split and also add skip_spaces, making the interface more robust. Signed-off-by: John Johansen Signed-off-by: James Morris --- security/apparmor/lib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 6e85cdb4303f6..506d2baf61479 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -40,6 +40,7 @@ char *aa_split_fqname(char *fqname, char **ns_name) *ns_name = NULL; if (name[0] == ':') { char *split = strchr(&name[1], ':'); + *ns_name = skip_spaces(&name[1]); if (split) { /* overwrite ':' with \0 */ *split = 0; @@ -47,7 +48,6 @@ char *aa_split_fqname(char *fqname, char **ns_name) } else /* a ns name without a following profile is allowed */ name = NULL; - *ns_name = &name[1]; } if (name && *name == 0) name = NULL; From 999b4f0aa2314b76857775334cb94bafa053db64 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 27 Aug 2010 18:33:29 -0700 Subject: [PATCH 4/5] AppArmor: Fix locking from removal of profile namespace The locking for profile namespace removal is wrong, when removing a profile namespace, it needs to be removed from its parent's list. Lock the parent of namespace list instead of the namespace being removed. Signed-off-by: John Johansen Signed-off-by: James Morris --- security/apparmor/policy.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 3cdc1ad0787ec..52cc865f14645 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -1151,12 +1151,14 @@ ssize_t aa_remove_profiles(char *fqname, size_t size) /* released below */ ns = aa_get_namespace(root); - write_lock(&ns->lock); if (!name) { /* remove namespace - can only happen if fqname[0] == ':' */ + write_lock(&ns->parent->lock); __remove_namespace(ns); + write_unlock(&ns->parent->lock); } else { /* remove profile */ + write_lock(&ns->lock); profile = aa_get_profile(__lookup_profile(&ns->base, name)); if (!profile) { error = -ENOENT; @@ -1165,8 +1167,8 @@ ssize_t aa_remove_profiles(char *fqname, size_t size) } name = profile->base.hname; __remove_profile(profile); + write_unlock(&ns->lock); } - write_unlock(&ns->lock); /* don't fail removal if audit fails */ (void) audit_policy(OP_PROF_RM, GFP_KERNEL, name, info, error); From e950598d43dce8d97e7d5270808393425d1e5cbd Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Tue, 31 Aug 2010 09:38:51 -0400 Subject: [PATCH 5/5] ima: always maintain counters commit 8262bb85da allocated the inode integrity struct (iint) before any inodes were created. Only after IMA was initialized in late_initcall were the counters updated. This patch updates the counters, whether or not IMA has been initialized, to resolve 'imbalance' messages. This patch fixes the bug as reported in bugzilla: 15673. When the i915 is builtin, the ring_buffer is initialized before IMA, causing the imbalance message on suspend. Reported-by: Thomas Meyer Signed-off-by: Mimi Zohar Tested-by: Thomas Meyer Tested-by: David Safford Cc: Stable Kernel Signed-off-by: James Morris --- security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_iint.c | 4 +++- security/integrity/ima/ima_main.c | 8 +++++--- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 16d100d3fc38d..3fbcd1dda0ef6 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -35,6 +35,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 }; #define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS) /* set during initialization */ +extern int iint_initialized; extern int ima_initialized; extern int ima_used_chip; extern char *ima_hash; diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c index 7625b85c2274f..afba4aef812f6 100644 --- a/security/integrity/ima/ima_iint.c +++ b/security/integrity/ima/ima_iint.c @@ -22,9 +22,10 @@ RADIX_TREE(ima_iint_store, GFP_ATOMIC); DEFINE_SPINLOCK(ima_iint_lock); - static struct kmem_cache *iint_cache __read_mostly; +int iint_initialized = 0; + /* ima_iint_find_get - return the iint associated with an inode * * ima_iint_find_get gets a reference to the iint. Caller must @@ -141,6 +142,7 @@ static int __init ima_iintcache_init(void) iint_cache = kmem_cache_create("iint_cache", sizeof(struct ima_iint_cache), 0, SLAB_PANIC, init_once); + iint_initialized = 1; return 0; } security_initcall(ima_iintcache_init); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index f93641382e9f9..e662b89d40794 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -148,12 +148,14 @@ void ima_counts_get(struct file *file) struct ima_iint_cache *iint; int rc; - if (!ima_initialized || !S_ISREG(inode->i_mode)) + if (!iint_initialized || !S_ISREG(inode->i_mode)) return; iint = ima_iint_find_get(inode); if (!iint) return; mutex_lock(&iint->mutex); + if (!ima_initialized) + goto out; rc = ima_must_measure(iint, inode, MAY_READ, FILE_CHECK); if (rc < 0) goto out; @@ -213,7 +215,7 @@ void ima_file_free(struct file *file) struct inode *inode = file->f_dentry->d_inode; struct ima_iint_cache *iint; - if (!ima_initialized || !S_ISREG(inode->i_mode)) + if (!iint_initialized || !S_ISREG(inode->i_mode)) return; iint = ima_iint_find_get(inode); if (!iint) @@ -230,7 +232,7 @@ static int process_measurement(struct file *file, const unsigned char *filename, { struct inode *inode = file->f_dentry->d_inode; struct ima_iint_cache *iint; - int rc; + int rc = 0; if (!ima_initialized || !S_ISREG(inode->i_mode)) return 0;