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

[sashiko-bot@kernel.org]

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