* [patch] apparmor: issue with ns name without a following profile @ 2010-08-07 11:50 ` Dan Carpenter 0 siblings, 0 replies; 4+ messages in thread From: Dan Carpenter @ 2010-08-07 11:50 UTC (permalink / raw) To: John Johansen Cc: James Morris, linux-security-module, linux-kernel, kernel-janitors 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(). Signed-off-by: Dan Carpenter <error27@gmail.com> --- I'm not very familiar with this code and I haven't tested my fix. Sorry. Please review carefully. diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 6e85cdb..da34011 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -44,10 +44,12 @@ char *aa_split_fqname(char *fqname, char **ns_name) /* overwrite ':' with \0 */ *split = 0; name = skip_spaces(split + 1); - } else + *ns_name = &name[1]; + } else { /* a ns name without a following profile is allowed */ + *ns_name = &name[1]; name = NULL; - *ns_name = &name[1]; + } } if (name && *name = 0) name = NULL; ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [patch] apparmor: issue with ns name without a following profile @ 2010-08-07 11:50 ` Dan Carpenter 0 siblings, 0 replies; 4+ messages in thread From: Dan Carpenter @ 2010-08-07 11:50 UTC (permalink / raw) To: John Johansen Cc: James Morris, linux-security-module, linux-kernel, kernel-janitors 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(). Signed-off-by: Dan Carpenter <error27@gmail.com> --- I'm not very familiar with this code and I haven't tested my fix. Sorry. Please review carefully. diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 6e85cdb..da34011 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -44,10 +44,12 @@ char *aa_split_fqname(char *fqname, char **ns_name) /* overwrite ':' with \0 */ *split = 0; name = skip_spaces(split + 1); - } else + *ns_name = &name[1]; + } else { /* a ns name without a following profile is allowed */ + *ns_name = &name[1]; name = NULL; - *ns_name = &name[1]; + } } if (name && *name == 0) name = NULL; ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [patch] apparmor: issue with ns name without a following profile 2010-08-07 11:50 ` Dan Carpenter @ 2010-08-12 15:15 ` John Johansen -1 siblings, 0 replies; 4+ messages in thread From: John Johansen @ 2010-08-12 15:15 UTC (permalink / raw) To: Dan Carpenter, James Morris, linux-security-module, linux-kernel, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 732 bytes --] On 08/07/2010 04:50 AM, Dan Carpenter wrote: > 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(). > > Signed-off-by: Dan Carpenter <error27@gmail.com> Indeed. I am sorry to say this case was not enabled in the test suite :( However proposed patch is incorrect, in that it results in namespace name that starts at &name[1]. I've attached two patches, the first fixes this issue, and the second fixes a locking bug in namespace removal, for this case (ie. where there is no profile name specified. Thanks for catching this John [-- Attachment #2: 0001-AppArmor-Fix-splitting-an-fqname-into-separate-names.patch --] [-- Type: text/x-diff, Size: 1644 bytes --] From 07abf5de857614c13449031b9ac9e06c8efb7765 Mon Sep 17 00:00:00 2001 From: John Johansen <john.johansen@canonical.com> Date: Mon, 9 Aug 2010 08:53:51 -0400 Subject: [PATCH 1/2] AppArmor: Fix splitting an fqname into separate namespace and profile names As per Dan Carpenter <error27@gmail.com> 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 <john.johansen@canonical.com> --- security/apparmor/lib.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 6e85cdb..506d2ba 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; -- 1.7.1 [-- Attachment #3: 0002-AppArmor-Fix-locking-from-removal-of-profile-namespa.patch --] [-- Type: text/x-diff, Size: 1591 bytes --] From 8109a95a05dd6e5349e43a2a6bb7149565cc886e Mon Sep 17 00:00:00 2001 From: John Johansen <john.johansen@canonical.com> Date: Thu, 12 Aug 2010 07:49:12 -0400 Subject: [PATCH 2/2] 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 <john.johansen@canonical.com> --- security/apparmor/policy.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 3cdc1ad..52cc865 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); -- 1.7.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [patch] apparmor: issue with ns name without a following profile @ 2010-08-12 15:15 ` John Johansen 0 siblings, 0 replies; 4+ messages in thread From: John Johansen @ 2010-08-12 15:15 UTC (permalink / raw) To: Dan Carpenter, James Morris, linux-security-module, linux-kernel, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 732 bytes --] On 08/07/2010 04:50 AM, Dan Carpenter wrote: > 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(). > > Signed-off-by: Dan Carpenter <error27@gmail.com> Indeed. I am sorry to say this case was not enabled in the test suite :( However proposed patch is incorrect, in that it results in namespace name that starts at &name[1]. I've attached two patches, the first fixes this issue, and the second fixes a locking bug in namespace removal, for this case (ie. where there is no profile name specified. Thanks for catching this John [-- Attachment #2: 0001-AppArmor-Fix-splitting-an-fqname-into-separate-names.patch --] [-- Type: text/x-diff, Size: 1645 bytes --] >From 07abf5de857614c13449031b9ac9e06c8efb7765 Mon Sep 17 00:00:00 2001 From: John Johansen <john.johansen@canonical.com> Date: Mon, 9 Aug 2010 08:53:51 -0400 Subject: [PATCH 1/2] AppArmor: Fix splitting an fqname into separate namespace and profile names As per Dan Carpenter <error27@gmail.com> 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 <john.johansen@canonical.com> --- security/apparmor/lib.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 6e85cdb..506d2ba 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; -- 1.7.1 [-- Attachment #3: 0002-AppArmor-Fix-locking-from-removal-of-profile-namespa.patch --] [-- Type: text/x-diff, Size: 1592 bytes --] >From 8109a95a05dd6e5349e43a2a6bb7149565cc886e Mon Sep 17 00:00:00 2001 From: John Johansen <john.johansen@canonical.com> Date: Thu, 12 Aug 2010 07:49:12 -0400 Subject: [PATCH 2/2] 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 <john.johansen@canonical.com> --- security/apparmor/policy.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 3cdc1ad..52cc865 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); -- 1.7.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-08-12 15:15 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-07 11:50 [patch] apparmor: issue with ns name without a following profile Dan Carpenter 2010-08-07 11:50 ` Dan Carpenter 2010-08-12 15:15 ` John Johansen 2010-08-12 15:15 ` John Johansen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.