[Patch 0/4] AppArmor post 2.6.36 merge fixups

[Patch 0/4] AppArmor post 2.6.36 merge fixups

[John Johansen]

The first two patches are post 2.6.36 merge fixups.  And the last two
fix the bug pointed out by Dan carpenter.

[PATCH 1/4] AppArmor: Drop hack to remove appended " (deleted)" string

[John Johansen]

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 <john.johansen@canonical.com>
---
 security/apparmor/path.c |   38 +++++++++++---------------------------
 1 files changed, 11 insertions(+), 27 deletions(-)

diff --git a/security/apparmor/path.c b/security/apparmor/path.c
index 19358dc..8239605 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 */
-- 
1.7.0.4

[PATCH 2/4] AppArmor: Fix security_task_setrlimit logic for 2.6.36 changes

[John Johansen]

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 <john.johansen@canonical.com>
---
 security/apparmor/lsm.c      |    2 +-
 security/apparmor/resource.c |   20 ++++++++++++--------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index f73e2c2..cf1de44 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 4a368f1..5bc46e5 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 non group leader tasks.
+	 * AppArmor rules currently have the implicit assumption that
+	 * the task having its resource set is the group leader.
+	 */
+	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);
 }
 
 /**
-- 
1.7.0.4

Re: [PATCH 2/4] AppArmor: Fix security_task_setrlimit logic for 2.6.36 changes

[John Johansen]

On 08/27/2010 06:33 PM, John Johansen wrote:
> 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.
> 
the change to security/apparmor/include/resource.h is missing from the
previous patch.

find the corrected patch below

---

>From f09ade4027dc55b3c41f0dffa587789c43395610 Mon Sep 17 00:00:00 2001
From: John Johansen <john.johansen@canonical.com>
Date: Wed, 25 Aug 2010 16:13:07 -0700
Subject: [PATCH] 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 <john.johansen@canonical.com>
---
 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 3c88be9..02baec7 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 f73e2c2..cf1de44 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 4a368f1..5bc46e5 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 non group leader tasks.
+	 * AppArmor rules currently have the implicit assumption that
+	 * the task having its resource set is the group leader.
+	 */
+	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);
 }
 
 /**

Re: [PATCH 2/4] AppArmor: Fix security_task_setrlimit logic for 2.6.36 changes

[Jiri Slaby]

On 08/28/2010 07:10 PM, John Johansen wrote:
> 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.
...
> --- a/security/apparmor/resource.c
> +++ b/security/apparmor/resource.c
...
> @@ -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 non group leader tasks.
> +	 * AppArmor rules currently have the implicit assumption that
> +	 * the task having its resource set is the group leader.

Why would you want to do that? Limits are per process, so the 'task'
parameter is guaranteed to be the leader.

> +	 */
> +	if ((task != current->group_leader) ||
> +	    (profile->rlimits.mask & (1 << resource) &&
> +	     new_rlim->rlim_max > profile->rlimits.limits[resource].rlim_max))
> +		error = -EACCES;

regards,
-- 
js

Re: [PATCH 2/4] AppArmor: Fix security_task_setrlimit logic for 2.6.36 changes

[John Johansen]

On 08/28/2010 11:15 AM, Jiri Slaby wrote:
> On 08/28/2010 07:10 PM, John Johansen wrote:
>> 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.
> ...
>> --- a/security/apparmor/resource.c
>> +++ b/security/apparmor/resource.c
> ...
>> @@ -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 non group leader tasks.
>> +	 * AppArmor rules currently have the implicit assumption that
>> +	 * the task having its resource set is the group leader.
> 
> Why would you want to do that? Limits are per process, so the 'task'
> parameter is guaranteed to be the leader.
> 
That used to be the case,

commit c022a0acad534fd5f5d5f17280f6d4d135e74e81 add the prlimit64 syscall
which

    It also adds a possibility of changing limits of other processes. We
    check the user's permissions to do that and if it succeeds, the new
    limits are propagated online. This is good for large scale
    applications such as SAP or databases where administrators need to
    change limits time by time (e.g. on crashes increase core size). And
    it is unacceptable to restart the service.

so it seems we need to extend the apparmor rules to be able to deal with
this, but ensuring that the current assumption is enforced is enough
for now.

thanks
john

Re: [PATCH 2/4] AppArmor: Fix security_task_setrlimit logic for 2.6.36 changes

[Jiri Slaby]

On 08/28/2010 10:35 PM, John Johansen wrote:
> On 08/28/2010 11:15 AM, Jiri Slaby wrote:
>> On 08/28/2010 07:10 PM, John Johansen wrote:
>>> 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.
>> ...
>>> --- a/security/apparmor/resource.c
>>> +++ b/security/apparmor/resource.c
>> ...
>>> @@ -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 non group leader tasks.
>>> +	 * AppArmor rules currently have the implicit assumption that
>>> +	 * the task having its resource set is the group leader.
>>
>> Why would you want to do that? Limits are per process, so the 'task'
>> parameter is guaranteed to be the leader.
>>
> That used to be the case,

It is still the case. The limits (the same as signals or accounting) are
per-process, they are not per-thread. If you look into do_prlimit() how
security_task_setrlimit() is called, you'll see.

> commit c022a0acad534fd5f5d5f17280f6d4d135e74e81 add the prlimit64 syscall
> which
> 
>     It also adds a possibility of changing limits of other processes. We
>     check the user's permissions to do that and if it succeeds, the new
>     limits are propagated online.
...
> so it seems we need to extend the apparmor rules to be able to deal with
> this, but ensuring that the current assumption is enforced is enough
> for now.

Yeah, I remember, the other Jiri inside wrote that. You are guaranteed
to get the group leader right now. And if it ever changes, which is
unlikely, all users would have to be checked and fixed anyway.

regards,
-- 
js

Re: [PATCH 2/4] AppArmor: Fix security_task_setrlimit logic for 2.6.36 changes

[John Johansen]

On 08/28/2010 01:48 PM, Jiri Slaby wrote:
> On 08/28/2010 10:35 PM, John Johansen wrote:
>> On 08/28/2010 11:15 AM, Jiri Slaby wrote:
>>> On 08/28/2010 07:10 PM, John Johansen wrote:
>>>> 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.
>>> ...
>>>> --- a/security/apparmor/resource.c
>>>> +++ b/security/apparmor/resource.c
>>> ...
>>>> @@ -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 non group leader tasks.
>>>> +	 * AppArmor rules currently have the implicit assumption that
>>>> +	 * the task having its resource set is the group leader.
>>>
>>> Why would you want to do that? Limits are per process, so the 'task'
>>> parameter is guaranteed to be the leader.
>>>
>> That used to be the case,
> 
> It is still the case. The limits (the same as signals or accounting) are
> per-process, they are not per-thread. If you look into do_prlimit() how
> security_task_setrlimit() is called, you'll see.
>
>> commit c022a0acad534fd5f5d5f17280f6d4d135e74e81 add the prlimit64 syscall
>> which
>>
>>     It also adds a possibility of changing limits of other processes. We
>>     check the user's permissions to do that and if it succeeds, the new
>>     limits are propagated online.
> ...
>> so it seems we need to extend the apparmor rules to be able to deal with
>> this, but ensuring that the current assumption is enforced is enough
>> for now.
> 
> Yeah, I remember, the other Jiri inside wrote that. You are guaranteed
> to get the group leader right now. And if it ever changes, which is
> unlikely, all users would have to be checked and fixed anyway.
> 
Right, it is the same.  I wrote the comment after verifying that only the
group leader was being set, and for some reason I ended up substituting
group leader for process when writing the comment, which really make the
it confusing and wrong.

It should have been more along the lines of

/* 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
 */

I'll update asap

thanks Jiri

Re: [PATCH 2/4] AppArmor: Fix security_task_setrlimit logic for 2.6.36 changes

[Jiri Slaby]

On 08/30/2010 06:53 PM, John Johansen wrote:
> /* 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
>  */

Makes sense(TM) now.

thanks,
-- 
js

Re: [PATCH 2/4] AppArmor: Fix security_task_setrlimit logic for 2.6.36 changes

[John Johansen]

On 08/30/2010 02:33 PM, Jiri Slaby wrote:
> On 08/30/2010 06:53 PM, John Johansen wrote:
>> /* 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
>>  */
> 
> Makes sense(TM) now.
> 
This is the patch updated with the only comment changed to what was
discussed with Jiri above.

thanks again
john

---
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 <john.johansen@canonical.com>
---
 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 3c88be9..02baec7 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 f73e2c2..cf1de44 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 4a368f1..a4136c1 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);
 }
 
 /**
-- 
1.7.0.4

[PATCH 3/4] AppArmor: Fix splitting an fqname into separate namespace and profile names

[John Johansen]

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.0.4

[PATCH 4/4] AppArmor: Fix locking from removal of profile namespace

[John Johansen]

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.0.4