[PATCH] apparmor: resolve pivotroot paths before the failure audit

[PATCH] apparmor: resolve pivotroot paths before the failure audit

[Adriano Cordova]

build_pivotroot() stores the new and old path names in the audit data
while it mediates a transition, but it returns early for unconfined
profiles and profiles that do not mediate mounts.  Resolve the names in
the failure path in that case before the audit record is emitted.

Signed-off-by: Adriano Cordova <adrianox@gmail.com>
---
 security/apparmor/mount.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
index 4ed7b9136beb..cd869a07335b 100644
--- a/security/apparmor/mount.c
+++ b/security/apparmor/mount.c
@@ -698,9 +698,18 @@ int aa_pivotroot(const struct cred *subj_cred, struct aa_label *label,
 	return error;
 
 fail:
-	/* TODO: add back in auditing of new_name and old_name,
-	 * needs lifting of name lookup out of profile cb
-	 */
+	if (!ad.name) {
+		struct aa_profile *p = labels_profile(label);
+
+		aa_path_name(new_path, path_flags(p, new_path), new_buffer,
+			     &ad.name, &ad.info, p->disconnected);
+	}
+	if (!ad.mnt.src_name) {
+		struct aa_profile *p = labels_profile(label);
+
+		aa_path_name(old_path, path_flags(p, old_path), old_buffer,
+			     &ad.mnt.src_name, &ad.info, p->disconnected);
+	}
 	ad.mnt.trans = target->hname;
 	error = aa_audit_perm_error(label, AA_MAY_PIVOTROOT, error, &ad,
 				    audit_cb);
-- 
2.51.0

Re: [PATCH] apparmor: resolve pivotroot paths before the failure audit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] apparmor: uninitialized memory leak and OOB read in aa_pivotroot fail path
--

commit bfa86a29c5894cbd4b54b58280d9a34e8e058003
Author: Adriano Cordova <adrianox@gmail.com>

apparmor: resolve pivotroot paths before the failure audit

build_pivotroot() stores the new and old path names in the audit data
while it mediates a transition, but it returns early for unconfined
profiles and profiles that do not mediate mounts. Resolve the names in
the failure path in that case before the audit record is emitted.

> diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
> index 4ed7b9136bebf..cd869a07335bf 100644
> --- a/security/apparmor/mount.c
> +++ b/security/apparmor/mount.c
> @@ -698,9 +698,18 @@ int aa_pivotroot(const struct cred *subj_cred, struct aa_label *label,
>  	return error;
>  
>  fail:
> -	/* TODO: add back in auditing of new_name and old_name,
> -	 * needs lifting of name lookup out of profile cb
> -	 */
> +	if (!ad.name) {
> +		struct aa_profile *p = labels_profile(label);
> +
> +		aa_path_name(new_path, path_flags(p, new_path), new_buffer,
> +			     &ad.name, &ad.info, p->disconnected);

[Severity: High]
Does this ignore the return value and potentially leak uninitialized memory?

If aa_path_name() fails (e.g., due to an excessively long path), it sets
the output pointer to point to the unmodified buffer:

security/apparmor/path.c:d_namespace_path() {
    ...
		if (PTR_ERR(res) == -ENAMETOOLONG) {
			error = -ENAMETOOLONG;
			*name = buf;
			goto out;
		}
    ...
}

Since new_buffer was allocated using kmalloc() without zeroing:

security/apparmor/lsm.c:aa_get_buffer() {
    ...
	aa_buf = kmalloc(aa_g_path_max, flags);
    ...
}

When the error from aa_path_name() is ignored here, ad.name is left
pointing to uninitialized kernel heap memory. The subsequent call to
aa_audit_perm_error() will then read this uninitialized memory,
potentially reading out of bounds or leaking kernel heap data into the
audit logs.

> +	}
> +	if (!ad.mnt.src_name) {
> +		struct aa_profile *p = labels_profile(label);
> +
> +		aa_path_name(old_path, path_flags(p, old_path), old_buffer,
> +			     &ad.mnt.src_name, &ad.info, p->disconnected);
> +	}
>  	ad.mnt.trans = target->hname;
>  	error = aa_audit_perm_error(label, AA_MAY_PIVOTROOT, error, &ad,
>  				    audit_cb);

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