Re: [PATCH v6 2/8] apparmor: Remove redundant MS_MGC_MSK stripping in apparmor_sb_mount

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] AppArmor's mount mediation contains a Time-of-Check to Time-of-Use (TOCTOU) vulnerability during mount operations (such as MS_BIND and MS_MOVE). It resolves user-supplied mount source paths via `kern_path()` for authorization. However, after authorization succeeds, VFS independently resolves the exact same string. An attacker can race this window to swap a symlink and mount unauthorized paths, entirely bypassing the MAC policy.
--

--- Patch [2]: [PATCH v6 2/8] apparmor: Remove redundant MS_MGC_MSK stripping in apparmor_sb_mount ---
commit eaca63dc76aaf43bda7315bdb224bbd33d80cf0e
Author: Song Liu <[email protected]>

apparmor: Remove redundant MS_MGC_MSK stripping in apparmor_sb_mount

This commit removes a redundant MS_MGC_MSK flag stripping check from
apparmor_sb_mount(), as path_mount() already handles it.

> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 3491e9f601943..4415bca5889cf 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -705,10 +705,6 @@ static int apparmor_sb_mount(const char *dev_name, const struct path *path,
>  	int error = 0;
>  	bool needput;

[Severity: High]
This is a pre-existing issue, but does relying on the user-supplied dev_name
string here create a time-of-check to time-of-use vulnerability?

AppArmor's mount mediation resolves the untrusted user string dev_name via
kern_path() for authorization. For example:

security/apparmor/mount.c:aa_bind_mount() {
    ...
    error = kern_path(dev_name, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &old_path);
    ...
}

After authorization succeeds, the core mount code independently resolves the
exact same string again:

fs/namespace.c:do_loopback() {
    ...
    err = kern_path(old_name, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &old_path);
    ...
}

Could an attacker race this window to swap a symlink and mount unauthorized
paths, bypassing the access policy?

> -	/* Discard magic */
> -	if ((flags & MS_MGC_MSK) == MS_MGC_VAL)
> -		flags &= ~MS_MGC_MSK;
> -
>  	flags &= ~AA_MS_IGNORE_MASK;
>  
>  	label = __begin_current_label_crit_section(&needput);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2