* [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()
@ 2026-07-14 15:38 Jann Horn
2026-07-14 15:49 ` Jann Horn
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Jann Horn @ 2026-07-14 15:38 UTC (permalink / raw)
To: John Johansen, John Johansen, Georgia Garcia, apparmor,
Paul Moore, Serge E. Hallyn
Cc: James Morris, Christian Brauner, Al Viro, Peter Zijlstra (Intel),
linux-security-module, kernel list, stable, Jann Horn
AppArmor's begin_current_label_crit_section() is a scary function called
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.)
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().
Cc: stable@vger.kernel.org
Fixes: c75afcd153f6 ("AppArmor: contexts used in attaching policy to system objects")
Signed-off-by: Jann Horn <jannh@google.com>
---
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 <jannh@google.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()
2026-07-14 15:38 [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section() Jann Horn
@ 2026-07-14 15:49 ` Jann Horn
2026-08-05 15:10 ` Jann Horn
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Jann Horn @ 2026-07-14 15:49 UTC (permalink / raw)
To: John Johansen, John Johansen, Georgia Garcia, apparmor,
Paul Moore, Serge E. Hallyn
Cc: James Morris, Christian Brauner, Al Viro, Peter Zijlstra (Intel),
linux-security-module, kernel list, stable
On Tue, Jul 14, 2026 at 5:39 PM Jann Horn <jannh@google.com> wrote:
> 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 test this, you should run a kernel with KASAN.
CONFIG_RCU_STRICT_GRACE_PERIOD=y might also be necessary to trigger a
KASAN warning.
Open two terminals A and B. In terminal A, write the following policy
into /etc/apparmor.d/credbug-test:
```
abi <abi/4.0>,
include <tunables/global>
profile credbug /tmp/credbug {
/** rwm,
mount,
umount,
capability sys_admin setuid,
}
```
and enable it:
```
user@vm:~$ sudo aa-enforce /tmp/credbug
Setting /tmp/credbug to enforce mode.
Warning: profile credbug represents multiple programs
user@vm:~$
```
In terminal B, build the reproducer and launch it with root privileges:
```
user@vm:~/apparmor-replace-label$ cat > credbug.c
#define _GNU_SOURCE
#include <pthread.h>
#include <assert.h>
#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/fsuid.h>
#include <sys/mount.h>
#include <sys/prctl.h>
#include <linux/fuse.h>
#define SYSCHK(x) ({ \
typeof(x) __res = (x); \
if (__res == (typeof(x))-1) \
err(1, "SYSCHK(" #x ")"); \
__res; \
})
static int p[2];
static int fuse_fd;
static volatile int fuse_ready = 0;
static volatile int backing_id = -1;
static int last_credref_fd = -1;
static int drop_last_credref = 0;
static void *splice_thread_fn(void *dummy) {
int backing_fd = SYSCHK(open("/tmp/credbug", O_RDONLY));
// unshare creds
setfsuid(1);
setfsuid(0);
last_credref_fd = SYSCHK(open("/", O_PATH));
while (!fuse_ready) /*spin*/;
struct fuse_backing_map backing_arg = { .fd = backing_fd };
backing_id = SYSCHK(ioctl(fuse_fd, FUSE_DEV_IOC_BACKING_OPEN, &backing_arg));
int passthrough_fd = SYSCHK(open("/tmp/mntfile", O_RDONLY));
off_t off0 = 0;
// creds are changed in the middle of this
splice(passthrough_fd, &off0, p[1], NULL, 1, 0);
close(passthrough_fd);
SYSCHK(ioctl(fuse_fd, FUSE_DEV_IOC_BACKING_CLOSE, &backing_id));
drop_last_credref = 1;
while (drop_last_credref) /*spin*/;
sleep(1);
getuid();
return NULL;
}
#define READ(_obj) if (read(fuse_fd, &(_obj), sizeof(_obj)) !=
sizeof(_obj)) err(1, "failed to read " #_obj)
#define WRITE(_obj) if (write(fuse_fd, &(_obj), (_obj).h.len) !=
(_obj).h.len) err(1, "failed to write " #_obj)
static void *fuse_thread_fn(void *dummy) {
while (1) {
struct {
struct fuse_in_header inh;
union {
struct fuse_init_in init_in;
struct fuse_open_in open_in;
struct fuse_read_in read_in;
char pad[10000];
};
} buf;
ssize_t read_res = read(fuse_fd, &buf, sizeof(buf));
if (read_res == -1) {
if (errno == ENODEV)
return NULL;
}
assert(read_res >= sizeof(buf.inh));
if (buf.inh.opcode == FUSE_INIT) {
printf("fuse: init\n");
struct {
struct fuse_out_header h;
struct fuse_init_out b;
} reply = {
.h = { .len = sizeof(reply), .error = 0, .unique = buf.inh.unique },
.b = {
.major = buf.init_in.major, .minor = buf.init_in.minor,
.max_stack_depth=1, .flags=FUSE_INIT_EXT, .flags2=FUSE_PASSTHROUGH>>32
}
};
WRITE(reply);
fuse_ready = 1;
} else if (buf.inh.opcode == FUSE_GETATTR) {
printf("fuse: getattr\n");
struct {
struct fuse_out_header h;
struct fuse_attr_out b;
} reply = {
.h = { .len = sizeof(reply), .error = 0, .unique = buf.inh.unique },
.b = {
.attr_valid = FATTR_SIZE | FATTR_MODE,
.attr = {
.size = 0x1,
.mode = 0100777
}
}
};
WRITE(reply);
} else if (buf.inh.opcode == FUSE_OPEN) {
printf("fuse: open node 0x%lu\n", (unsigned long)buf.inh.nodeid);
while (backing_id == -1) /*spin*/;
struct {
struct fuse_out_header h;
struct fuse_open_out b;
} reply = {
.h = { .len = sizeof(reply), .error = 0, .unique = buf.inh.unique },
.b = { .open_flags = FOPEN_PASSTHROUGH, .backing_id = backing_id }
};
WRITE(reply);
} else {
printf("FUSE_<%d> unhandled\n", buf.inh.opcode);
struct {
struct fuse_out_header h;
} reply = {
.h = { .len = sizeof(reply), .error = -ENOSYS, .unique =
buf.inh.unique },
};
WRITE(reply);
}
}
}
int main(void) {
// create a FUSE mount, and handle requests in a thread
SYSCHK(close(SYSCHK(open("/tmp/mntfile", O_RDONLY|O_CREAT, 0666))));
fuse_fd = SYSCHK(open("/dev/fuse", O_RDWR));
char mount_data[4096];
sprintf(mount_data, "fd=%d,rootmode=0100777,user_id=%d,group_id=%d",
fuse_fd, getuid(), getgid());
SYSCHK(mount("blah", "/tmp/mntfile", "fuse", MS_NODEV|MS_NOSUID, mount_data));
pthread_t fuse_thread;
pthread_create(&fuse_thread, NULL, fuse_thread_fn, NULL);
SYSCHK(pipe(p));
SYSCHK(fcntl(p[1], F_SETPIPE_SZ, 0x1000));
char buf[0x1000] = {};
SYSCHK(write(p[1], buf, 0x1000));
pthread_t splice_thread;
pthread_create(&splice_thread, NULL, splice_thread_fn, NULL);
getchar();
read(p[0], buf, 0x1000);
while (!drop_last_credref) /*spin*/;
close(last_credref_fd);
drop_last_credref = 0;
pthread_join(splice_thread, NULL);
SYSCHK(umount2("/tmp/mntfile", MNT_DETACH));
pthread_join(fuse_thread, NULL);
}
user@vm:~/apparmor-replace-label$ gcc -o /tmp/credbug credbug.c
user@vm:~/apparmor-replace-label$ sudo /tmp/credbug
fuse: init
fuse: open node 0x1
```
While the reproducer is blocked, use terminal A to disable its profile:
```
user@vm:~$ sudo aa-disable /tmp/credbug
Disabling /tmp/credbug.
user@vm:~$
```
Then hit enter in terminal B to let the reproducer continue:
```
FUSE_<25> unhandled
FUSE_<18> unhandled
user@vm:~/apparmor-replace-label$
```
Now you should see a KASAN UAF splat in dmesg, when getuid() tries
accessing the creds:
```
==================================================================
BUG: KASAN: slab-use-after-free in __ia32_sys_getuid+0x3d/0x80
Read of size 8 at addr ffff8881269a8950 by task credbug/696
CPU: 2 UID: 0 PID: 696 Comm: credbug Not tainted
7.2.0-rc3-00038-g3b029c035b34 #55 PREEMPT
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.17.0-debian-1.17.0-1 04/01/2014
Call Trace:
<TASK>
__dump_stack+0x21/0x30
dump_stack_lvl+0x76/0xa0
print_address_description+0x7b/0x1f0
print_report+0x5b/0x70
kasan_report+0x16d/0x1a0
[...]
__asan_load8+0x98/0xa0
__ia32_sys_getuid+0x3d/0x80
x64_sys_call+0x1cc1/0x3030
do_syscall_64+0xd8/0x380
[...]
entry_SYSCALL_64_after_hwframe+0x76/0x7e
[...]
</TASK>
Allocated by task 696:
kasan_save_track+0x3a/0x70
kasan_save_alloc_info+0x3c/0x50
__kasan_slab_alloc+0x4e/0x60
kmem_cache_alloc_noprof+0x25f/0x570
prepare_creds+0x2b/0x4d0
__sys_setfsuid+0x8b/0x190
__x64_sys_setfsuid+0x1e/0x30
x64_sys_call+0x955/0x3030
do_syscall_64+0xd8/0x380
entry_SYSCALL_64_after_hwframe+0x76/0x7e
Freed by task 70:
kasan_save_track+0x3a/0x70
kasan_save_free_info+0x46/0x60
__kasan_slab_free+0x43/0x70
kmem_cache_free+0x182/0x500
put_cred_rcu+0x19e/0x210
rcu_core+0x877/0xfd0
rcu_core_si+0x9/0x10
handle_softirqs+0x19f/0x550
__irq_exit_rcu+0xab/0x180
irq_exit_rcu+0x9/0x20
sysvec_call_function+0x73/0x80
asm_sysvec_call_function+0x1b/0x20
Last potentially related work creation:
kasan_save_stack+0x3a/0x60
kasan_record_aux_stack+0x99/0xb0
call_rcu+0x51/0x5c0
__put_cred+0x9a/0xc0
__fput+0x425/0x540
fput_close_sync+0x8a/0x140
__x64_sys_close+0x55/0xe0
x64_sys_call+0x26ce/0x3030
do_syscall_64+0xd8/0x380
entry_SYSCALL_64_after_hwframe+0x76/0x7e
The buggy address belongs to the object at ffff8881269a88c0
which belongs to the cache cred of size 184
The buggy address is located 144 bytes inside of
freed 184-byte region [ffff8881269a88c0, ffff8881269a8978)
[...]
==================================================================
```
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()
2026-07-14 15:38 [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section() Jann Horn
2026-07-14 15:49 ` Jann Horn
@ 2026-08-05 15:10 ` Jann Horn
2026-08-05 23:13 ` John Johansen
2026-08-06 0:02 ` John Johansen
2026-08-06 7:32 ` Peter Zijlstra
3 siblings, 1 reply; 9+ messages in thread
From: Jann Horn @ 2026-08-05 15:10 UTC (permalink / raw)
To: John Johansen, John Johansen, Georgia Garcia, apparmor,
Paul Moore, Serge E. Hallyn
Cc: James Morris, Christian Brauner, Al Viro, Peter Zijlstra (Intel),
linux-security-module, kernel list, stable
On Tue, Jul 14, 2026 at 5:39 PM Jann Horn <jannh@google.com> wrote:
> AppArmor's begin_current_label_crit_section() is a scary function called
> 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.
ping, I've seen no activity on this fix since posting it
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()
2026-08-05 15:10 ` Jann Horn
@ 2026-08-05 23:13 ` John Johansen
0 siblings, 0 replies; 9+ messages in thread
From: John Johansen @ 2026-08-05 23:13 UTC (permalink / raw)
To: Jann Horn, John Johansen, Georgia Garcia, apparmor, Paul Moore,
Serge E. Hallyn
Cc: James Morris, Christian Brauner, Al Viro, Peter Zijlstra (Intel),
linux-security-module, kernel list, stable
On 8/5/26 08:10, Jann Horn wrote:
> On Tue, Jul 14, 2026 at 5:39 PM Jann Horn <jannh@google.com> wrote:
>> AppArmor's begin_current_label_crit_section() is a scary function called
>> 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.
>
> ping, I've seen no activity on this fix since posting it
sorry, just swamped and not catching up so far.
I am looking at it now
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()
2026-07-14 15:38 [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section() Jann Horn
2026-07-14 15:49 ` Jann Horn
2026-08-05 15:10 ` Jann Horn
@ 2026-08-06 0:02 ` John Johansen
2026-08-06 13:14 ` Jann Horn
2026-08-06 7:32 ` Peter Zijlstra
3 siblings, 1 reply; 9+ messages in thread
From: John Johansen @ 2026-08-06 0:02 UTC (permalink / raw)
To: Jann Horn, John Johansen, Georgia Garcia, apparmor, Paul Moore,
Serge E. Hallyn
Cc: James Morris, Christian Brauner, Al Viro, Peter Zijlstra (Intel),
linux-security-module, kernel list, stable
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: stable@vger.kernel.org
> Fixes: c75afcd153f6 ("AppArmor: contexts used in attaching policy to system objects")
> Signed-off-by: Jann Horn <jannh@google.com>
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 <john.johansen@canonical.com>
> ---
> 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 <jannh@google.com>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()
2026-07-14 15:38 [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section() Jann Horn
` (2 preceding siblings ...)
2026-08-06 0:02 ` John Johansen
@ 2026-08-06 7:32 ` Peter Zijlstra
2026-08-06 8:06 ` John Johansen
3 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2026-08-06 7:32 UTC (permalink / raw)
To: Jann Horn
Cc: John Johansen, John Johansen, Georgia Garcia, apparmor,
Paul Moore, Serge E. Hallyn, James Morris, Christian Brauner,
Al Viro, linux-security-module, kernel list, stable
On Tue, Jul 14, 2026 at 05:38:07PM +0200, Jann Horn wrote:
> 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()
> *
This thing is quite terrible. And AFAICT the only purpose is to
determine if said task already has said function enqueued. Why not add a
single bit to struct task_struct for this? I'm sure we have a spare bit
somewhere.
> +/* 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);
> +}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()
2026-08-06 7:32 ` Peter Zijlstra
@ 2026-08-06 8:06 ` John Johansen
2026-08-06 13:11 ` Jann Horn
0 siblings, 1 reply; 9+ messages in thread
From: John Johansen @ 2026-08-06 8:06 UTC (permalink / raw)
To: Peter Zijlstra, Jann Horn
Cc: John Johansen, Georgia Garcia, apparmor, Paul Moore,
Serge E. Hallyn, James Morris, Christian Brauner, Al Viro,
linux-security-module, kernel list, stable
On 8/6/26 00:32, Peter Zijlstra wrote:
> On Tue, Jul 14, 2026 at 05:38:07PM +0200, Jann Horn wrote:
>
>> 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()
>> *
>
> This thing is quite terrible. And AFAICT the only purpose is to
> determine if said task already has said function enqueued. Why not add a
> single bit to struct task_struct for this? I'm sure we have a spare bit
> somewhere.
>
single bit wouldn't work generically to represent the different functions
that could be enqueued but we could stick a flag in the apparmor task
security blob, so we could just check if apparmor has enqueued its
function.
The trade-off is you don't get an admittedly ugly generic fn that someone
else could use.
>> +/* 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);
>> +}
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()
2026-08-06 8:06 ` John Johansen
@ 2026-08-06 13:11 ` Jann Horn
0 siblings, 0 replies; 9+ messages in thread
From: Jann Horn @ 2026-08-06 13:11 UTC (permalink / raw)
To: John Johansen, Peter Zijlstra
Cc: John Johansen, Georgia Garcia, apparmor, Paul Moore,
Serge E. Hallyn, James Morris, Christian Brauner, Al Viro,
linux-security-module, kernel list, stable
On Thu, Aug 6, 2026 at 10:06 AM John Johansen
<john.johansen@canonical.com> wrote:
> On 8/6/26 00:32, Peter Zijlstra wrote:
> > On Tue, Jul 14, 2026 at 05:38:07PM +0200, Jann Horn wrote:
> >
> >> 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()
> >> *
> >
> > This thing is quite terrible. And AFAICT the only purpose is to
> > determine if said task already has said function enqueued. Why not add a
> > single bit to struct task_struct for this? I'm sure we have a spare bit
> > somewhere.
> >
>
> single bit wouldn't work generically to represent the different functions
> that could be enqueued but we could stick a flag in the apparmor task
> security blob, so we could just check if apparmor has enqueued its
> function.
>
> The trade-off is you don't get an admittedly ugly generic fn that someone
> else could use.
Yeah, makes sense, I'll rework this to not touch task_work.c and
instead do something with the apparmor task blob.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section()
2026-08-06 0:02 ` John Johansen
@ 2026-08-06 13:14 ` Jann Horn
0 siblings, 0 replies; 9+ messages in thread
From: Jann Horn @ 2026-08-06 13:14 UTC (permalink / raw)
To: John Johansen
Cc: John Johansen, Georgia Garcia, apparmor, Paul Moore,
Serge E. Hallyn, James Morris, Christian Brauner, Al Viro,
Peter Zijlstra (Intel),
linux-security-module, kernel list, stable
On Thu, Aug 6, 2026 at 2:02 AM John Johansen
<john.johansen@canonical.com> wrote:
> 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.
It would waste a little bit of memory, and I'm not convinced that the
thundering herd would be an issue since allocation would only happen
when a task is running, and the allocation would go away at the end of
the syscall...
But yeah, I think that would make things simpler and avoid touching
task_work infrastructure, so I'll rework this.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-08-06 13:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-07-14 15:38 [PATCH] apparmor: fix cred UAF caused by begin_current_label_crit_section() Jann Horn
2026-07-14 15:49 ` Jann Horn
2026-08-05 15:10 ` Jann Horn
2026-08-05 23:13 ` John Johansen
2026-08-06 0:02 ` John Johansen
2026-08-06 13:14 ` Jann Horn
2026-08-06 7:32 ` Peter Zijlstra
2026-08-06 8:06 ` John Johansen
2026-08-06 13:11 ` Jann Horn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®