[PATCH] apparmor: safely acquire the cached label in aa_file_perm()

[PATCH] apparmor: safely acquire the cached label in aa_file_perm()

[Weiming Shi]

aa_file_perm() passes an unreferenced RCU snapshot of fctx->label to
aa_get_newest_label(). Its non-stale branch uses a plain kref_get(). Policy
replacement can drop the proxy reference after the stale test while
update_file_ctx() drops the cached reference, so pread64() can increment
the label's refcount from zero.

Acquire the RCU-published label with kref_get_unless_zero before testing it
for staleness, keeping the label alive while resolving its replacement.

refcount_t: addition on 0; use-after-free.
WARNING: lib/refcount.c:25 at refcount_warn_saturate+0x111/0x130, CPU#1: AAVICTIM/5091
RIP: refcount_warn_saturate (lib/refcount.c:25)
Call Trace:
 aa_file_perm (security/apparmor/file.c:657)
 common_file_perm (security/apparmor/lsm.c:529)
 security_file_permission (security/security.c:2392)
 rw_verify_area (fs/read_write.c:476)
 vfs_read (fs/read_write.c:566)
 __x64_sys_pread64 (fs/read_write.c:770)
 do_syscall_64 (arch/x86/entry/syscall_64.c:94)
 entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)

Fixes: bce4e7e9c45e ("apparmor: reduce rcu_read_lock scope for aa_file_perm mediation")
Cc: stable@vger.kernel.org
Reported-by: <co+c85e76ce40a43b9b@bugs.sh>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 security/apparmor/file.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/security/apparmor/file.c b/security/apparmor/file.c
index 694e157149e85e..59c27b9c5a8d15 100644
--- a/security/apparmor/file.c
+++ b/security/apparmor/file.c
@@ -621,7 +621,7 @@ int aa_file_perm(const char *op, const struct cred *subj_cred,
 		 u32 request, bool in_atomic)
 {
 	struct aa_file_ctx *fctx;
-	struct aa_label *flabel;
+	struct aa_label *flabel, *tmp;
 	u32 denied;
 	int error = 0;
 
@@ -654,7 +654,12 @@ int aa_file_perm(const char *op, const struct cred *subj_cred,
 	}
 
 	/* slow path - revalidate access */
-	flabel  = aa_get_newest_label(flabel);
+	flabel = aa_get_label_rcu(&fctx->label);
+	if (label_is_stale(flabel)) {
+		tmp = flabel;
+		flabel = aa_get_newest_label(tmp);
+		aa_put_label(tmp);
+	}
 	rcu_read_unlock();
 
 	if (path_mediated_fs(file->f_path.dentry))
-- 
2.56.0

Re: [PATCH] apparmor: safely acquire the cached label in aa_file_perm()

[sashiko-bot]

> aa_file_perm() passes an unreferenced RCU snapshot of fctx->label to
> aa_get_newest_label(). Its non-stale branch uses a plain kref_get(). Policy
> replacement can drop the proxy reference after the stale test while
> update_file_ctx() drops the cached reference, so pread64() can increment
> the label's refcount from zero.
> 
> Acquire the RCU-published label with kref_get_unless_zero before testing it
> for staleness, keeping the label alive while resolving its replacement.
> 
> refcount_t: addition on 0; use-after-free.
> WARNING: lib/refcount.c:25 at refcount_warn_saturate+0x111/0x130, CPU#1: AAVICTIM/5091
> RIP: refcount_warn_saturate (lib/refcount.c:25)
> Call Trace:
>  aa_file_perm (security/apparmor/file.c:657)
>  common_file_perm (security/apparmor/lsm.c:529)
>  security_file_permission (security/security.c:2392)
>  rw_verify_area (fs/read_write.c:476)
>  vfs_read (fs/read_write.c:566)
>  __x64_sys_pread64 (fs/read_write.c:770)
>  do_syscall_64 (arch/x86/entry/syscall_64.c:94)
>  entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
> 
> Fixes: bce4e7e9c45e ("apparmor: reduce rcu_read_lock scope for aa_file_perm mediation")
> Cc: stable@vger.kernel.org
> Reported-by: <co+c85e76ce40a43b9b@bugs.sh>
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107102534.816756-2-bestswngs@gmail.com?part=1