Re: [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()

On 7/14/26 08:38, Jann Horn wrote:
> AppArmor's begin_current_label_crit_section() is a scary function called

yep

> from lots of LSM hooks (in particular VFS/socket-related ones) that checks
> if the label referenced by the current creds is marked FLAG_STALE, and if
> so, attempts to use aa_replace_current_label() to replace the creds with an
> updated version that uses a new label.
> 
> The first problem with this is that it would directly lead to UAF of
> `struct cred` if anything in the kernel takes a pointer to the current
> creds and accesses these past a security hook invocation that replaces
> creds, like so:
> ```
> const struct cred *cred = current_cred();
> alloc_file_pseudo(...);
> uid_t uid = cred->euid;
> ```
> I don't know if anything in the kernel actually does this, but I think it
> is very surprising that this pattern could lead to UAF.
> 
> The second problem is that things go wrong when aa_replace_current_label()
> runs with overridden credentials. aa_replace_current_label() bails out if
> `current_cred() != current_real_cred()` (mirroring the check in
> proc_pid_attr_write()), but this check can't actually reliably detect
> overridden credentials because the overridden creds can be the same as the
> objective creds.
> 
> So in approximately the following scenario, things go wrong:
> 
> 1. task begins with <creds A> (as both objective and subjective creds),
>     with refcount=2
> 2. task grabs an extra reference on <creds A> for overriding
> 3. task calls override_creds(<creds A>), which returns a pointer to the old
>     subjective creds (<creds A>)
> 4. task enters AppArmor LSM hook
> 5. AppArmor checks that objective/subjective creds are equal
> 6. AppArmor replaces both cred pointers with <creds B> and drops 2 refs on
>     <creds A>
> 7. task leaves AppArmor LSM hook
> 8. task calls revert_creds(<creds A>)
> 9. now task->cred is <creds A> while task->real_cred is <creds B>, but the
>     task_struct logically holds two references to <creds B>
> 10. another task drops the extra reference on <creds A> that was used for
>      overriding, refcount drops to 0
> 11. now task->real_cred points to freed creds
> 
> At this point, any access to current_cred() will be UAF.
> 
> I have a test case where I run aa-disable on a profile while a process
> using that profile is blocked on splice() from a FUSE passthrough file into
> a full pipe; after the profile update, the pipe becomes empty, splice()
> resumes, the credentials go out of sync, and a subsequent getuid() syscall
> results in a KASAN UAF splat.
> 
> To fix this, instead of directly replacing creds, do it via task_work that
> will run at the end of the current syscall. (The point in time at which the
> cred replacement happens should have no correctness impact; it is just a
> performance optimization to avoid unnecessarily touching the refcount of
> the new label.)

right, we could even be a little looser in the update timing as we don't
make guarantees around the update timing.> 
> Note that AppArmor still performs direct cred replacements in the
> sb_pivotroot LSM hook after this change, and that direct cred replacements
> can still happen in VFS ->write() callbacks via proc_pid_attr_write().
> 
yep, I can take a stab at those

> Cc: [email protected]
> Fixes: c75afcd153f6 ("AppArmor: contexts used in attaching policy to system objects")
> Signed-off-by: Jann Horn <[email protected]>

this looks good to me, compiles, and has passed an abbreviated round of
testing.

Q. What do you think about making the callback_head part of the apparmor
security task blob. It would increase its size, but allow us to drop
the alloc/free. Generally profile is generally something that is relatively
rare, but when it does happen, its is quite common to have all the profiles
being updated, so a thunder herd type problem.

Unless someone else has a reason to carry this, I will pull it into
apparmor-next today, so we can get some testing in linux-next

Acked-by: John Johansen <[email protected]>

> ---
>   include/linux/task_work.h        |  1 +
>   kernel/task_work.c               | 14 ++++++++++++++
>   security/apparmor/include/cred.h |  6 +-----
>   security/apparmor/include/task.h |  1 +
>   security/apparmor/task.c         | 29 +++++++++++++++++++++++++++++
>   5 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/task_work.h b/include/linux/task_work.h
> index 0646804860ff..ce19fc14060c 100644
> --- a/include/linux/task_work.h
> +++ b/include/linux/task_work.h
> @@ -33,6 +33,7 @@ struct callback_head *task_work_cancel_match(struct task_struct *task,
>   	bool (*match)(struct callback_head *, void *data), void *data);
>   struct callback_head *task_work_cancel_func(struct task_struct *, task_work_func_t);
>   bool task_work_cancel(struct task_struct *task, struct callback_head *cb);
> +bool task_work_has_func(struct task_struct *task, task_work_func_t func);
>   void task_work_run(void);
>   
>   static inline void exit_task_work(struct task_struct *task)
> diff --git a/kernel/task_work.c b/kernel/task_work.c
> index 0f7519f8e7c9..f83d1528e0bc 100644
> --- a/kernel/task_work.c
> +++ b/kernel/task_work.c
> @@ -189,6 +189,20 @@ bool task_work_cancel(struct task_struct *task, struct callback_head *cb)
>   	return ret == cb;
>   }
>   
> +bool task_work_has_func(struct task_struct *task, task_work_func_t func)
> +{
> +	struct callback_head *work;
> +
> +	if (!task_work_pending(task))
> +		return false;
> +	guard(raw_spinlock_irqsave)(&task->pi_lock);
> +	for (work = READ_ONCE(task->task_works); work; work = READ_ONCE(work->next)) {
> +		if (work->func == func)
> +			return true;
> +	}
> +	return false;
> +}
> +
>   /**
>    * task_work_run - execute the works added by task_work_add()
>    *
> diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h
> index 2b6098149b15..0e8b67159f56 100644
> --- a/security/apparmor/include/cred.h
> +++ b/security/apparmor/include/cred.h
> @@ -222,13 +222,9 @@ static inline struct aa_label *begin_current_label_crit_section(void)
>   {
>   	struct aa_label *label = aa_current_raw_label();
>   
> -	might_sleep();
> -
>   	if (label_is_stale(label)) {
>   		label = aa_get_newest_label(label);
> -		if (aa_replace_current_label(label) == 0)
> -			/* task cred will keep the reference */
> -			aa_put_label(label);
> +		aa_schedule_stale_label_replacement();
>   	}
>   
>   	return label;
> diff --git a/security/apparmor/include/task.h b/security/apparmor/include/task.h
> index b1aaaf60fa8b..4e49a4142777 100644
> --- a/security/apparmor/include/task.h
> +++ b/security/apparmor/include/task.h
> @@ -30,6 +30,7 @@ struct aa_task_ctx {
>   };
>   
>   int aa_replace_current_label(struct aa_label *label);
> +void aa_schedule_stale_label_replacement(void);
>   void aa_set_current_onexec(struct aa_label *label, bool stack);
>   int aa_set_current_hat(struct aa_label *label, u64 token);
>   int aa_restore_previous_label(u64 cookie);
> diff --git a/security/apparmor/task.c b/security/apparmor/task.c
> index b9fb3738124e..8e368f6278f5 100644
> --- a/security/apparmor/task.c
> +++ b/security/apparmor/task.c
> @@ -14,6 +14,7 @@
>   
>   #include <linux/gfp.h>
>   #include <linux/ptrace.h>
> +#include <linux/task_work.h>
>   
>   #include "include/path.h"
>   #include "include/audit.h"
> @@ -89,6 +90,34 @@ int aa_replace_current_label(struct aa_label *label)
>   	return 0;
>   }
>   
> +static void aa_replace_stale_label_tw_func(struct callback_head *tw)
> +{
> +	struct aa_label *label;
> +
> +	kfree(tw);
> +	label = aa_current_raw_label();
> +	if (!label_is_stale(label))
> +		return;
> +	label = aa_get_newest_label(label);
> +	aa_replace_current_label(label);
> +	aa_put_label(label);
> +}
> +
> +/* replace the current task's stale label on syscall return */
> +void aa_schedule_stale_label_replacement(void)
> +{
> +	struct callback_head *tw;
> +
> +	if (task_work_has_func(current, aa_replace_stale_label_tw_func))
> +		return;
> +	tw = kmalloc_obj(struct callback_head);
> +	if (!tw)
> +		return;
> +	init_task_work(tw, aa_replace_stale_label_tw_func);
> +	if (task_work_add(current, tw, TWA_RESUME))
> +		kfree(tw);
> +}
> +
>   
>   /**
>    * aa_set_current_onexec - set the tasks change_profile to happen onexec
> 
> ---
> base-commit: 3b029c035b34bbc693405ddf759f0e9b920c27f1
> change-id: 20260714-fix-apparmor-cred-uaf-cc38ec2b38b7
> 
> Best regards,
> --
> Jann Horn <[email protected]>
>