[apparmor] [PATCH] apparmor: grab ns lock and refresh when looking up changehat child profiles

[John Johansen]

On 2/13/26 11:29, Ryan Lee wrote:
> There was a race condition involving change_hat and profile replacement in
> which replacement of the parent profile during a changehat operation could
> result in the list of children becoming empty and the changehat operation
> failing. To prevent this:
>   - grab the namespace lock until we've built the hat transition, and
>   - use aa_get_newest_profile to avoid using stale profile objects.
> 
> Link: https://bugs.launchpad.net/bugs/2139664
> Signed-off-by: Ryan Lee <ryan.lee at canonical.com>

Acked-by: John Johansen <john.johansen at canonical.com>

> ---
>   security/apparmor/domain.c | 33 +++++++++++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
> 
> This version of the patch applies cleanly to the Ubuntu 6.17 kernel.
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 7d81111f628c..2d3b80423d20 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -12,6 +12,7 @@
>   #include <linux/fs.h>
>   #include <linux/file.h>
>   #include <linux/mount.h>
> +#include <linux/mutex.h>
>   #include <linux/syscalls.h>
>   #include <linux/personality.h>
>   #include <linux/xattr.h>
> @@ -1128,6 +1129,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred,
>   				   int count, int flags)
>   {
>   	struct aa_profile *profile, *root, *hat = NULL;
> +	struct aa_ns *ns, *new_ns;
>   	struct aa_label *new;
>   	struct label_it it;
>   	bool sibling = false;
> @@ -1138,6 +1140,32 @@ static struct aa_label *change_hat(const struct cred *subj_cred,
>   	AA_BUG(!hats);
>   	AA_BUG(count < 1);
>   
> +	/*
> +	 * Acquire the newest label and then hold the lock until we choose a
> +	 * hat, so that profile replacement doesn't atomically truncate the
> +	 * list of potential hats. Because we are getting the namespaces from
> +	 * the profiles and label, we can rely on the namespaces being live
> +	 * and avoid incrementing their refcounts while grabbing the lock.
> +	 */
> +	label = aa_get_label(label);
> +	ns = labels_ns(label);
> +
> +retry:
> +	mutex_lock_nested(&ns->lock, ns->level);
> +	if (label_is_stale(label)) {
> +		new = aa_get_newest_label(label);
> +		new_ns = labels_ns(new);
> +		if (new_ns != ns) {
> +			aa_put_label(new);
> +			mutex_unlock(&ns->lock);
> +			ns = new_ns;
> +			label = new;
> +			goto retry;
> +		}
> +		aa_put_label(label);
> +		label = new;
> +	}
> +
>   	if (PROFILE_IS_HAT(labels_profile(label)))
>   		sibling = true;
>   
> @@ -1146,7 +1174,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred,
>   		name = hats[i];
>   		label_for_each_in_ns(it, labels_ns(label), label, profile) {
>   			if (sibling && PROFILE_IS_HAT(profile)) {
> -				root = aa_get_profile_rcu(&profile->parent);
> +				root = aa_get_profile(profile->parent);
>   			} else if (!sibling && !PROFILE_IS_HAT(profile)) {
>   				root = aa_get_profile(profile);
>   			} else {	/* conflicting change type */
> @@ -1206,6 +1234,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred,
>   				      GLOBAL_ROOT_UID, info, error, false);
>   		}
>   	}
> +	mutex_unlock(&ns->lock);
>   	return ERR_PTR(error);
>   
>   build:
> @@ -1218,7 +1247,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred,
>   		error = -ENOMEM;
>   		goto fail;
>   	} /* else if (IS_ERR) build_change_hat has logged error so return new */
> -
> +	mutex_unlock(&ns->lock);
>   	return new;
>   }
>