Re: [PATCH v6 3/8] apparmor: Convert from sb_mount to granular mount hooks

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The patch completely disables AppArmor mount mediation by replacing active LSM hooks (`sb_mount` and `move_mount`) with new granular hooks (`mount_bind`, `mount_new`, `mount_remount`, `mount_move`, etc.) that are never invoked by the VFS.
- [High] The new granular mount hooks fail to strip `AA_MS_IGNORE_MASK` internal kernel flags before policy matching, causing legitimate mounts to be denied if the hooks were active.
--

--- Patch [3]: [PATCH v6 3/8] apparmor: Convert from sb_mount to granular mount hooks ---
commit 6ef64d51cf68e28f54ba97e3ee56be6a9183f162
Author: Song Liu <[email protected]>

apparmor: Convert from sb_mount to granular mount hooks

Replace AppArmor's monolithic apparmor_sb_mount() with granular mount hooks.
Key changes include updating bind, new, remount, reconfigure, move, and
change_type mount handling.

> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 4415bca5889cf..cfd7f0f757734 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -698,41 +699,91 @@ static int apparmor_uring_sqpoll(void)
>  }
>  #endif /* CONFIG_IO_URING */
>  
> -static int apparmor_sb_mount(const char *dev_name, const struct path *path,
> -			     const char *type, unsigned long flags, void *data)
> -{
> -	struct aa_label *label;
> -	int error = 0;
> -	bool needput;
> -
> -	flags &= ~AA_MS_IGNORE_MASK;

[ ... ]

> +static int apparmor_mount_new(struct fs_context *fc, const struct path *mp,
> +			      int mnt_flags, unsigned long flags, void *data)
> +{
> +	struct aa_label *label;
> +	int error = 0;
> +	bool needput;
> +
> +	/* flags and data are from the original mount(2) call */
> +	label = __begin_current_label_crit_section(&needput);
> +	if (!unconfined(label))
> +		error = aa_new_mount(current_cred(), label, fc->source,
> +				     mp, fc->fs_type->name, flags, data);

[Severity: High]
Does this pass raw flags to aa_new_mount() without stripping internal kernel
flags? 

The old apparmor_sb_mount() explicitly performed:

    flags &= ~AA_MS_IGNORE_MASK;

Could passing raw flags cause legitimate mounts to be denied if internal bits
fail policy evaluation?

> +	__end_current_label_crit_section(label, needput);
> +
> +	return error;
> +}
> +
> +static int apparmor_mount_remount(struct fs_context *fc, const struct path *mp,
> +				  int mnt_flags, unsigned long flags,
> +				  void *data)
> +{
> +	struct aa_label *label;
> +	int error = 0;
> +	bool needput;
> +
> +	/* flags and data are from the original mount(2) call */
> +	label = __begin_current_label_crit_section(&needput);
> +	if (!unconfined(label))
> +		error = aa_remount(current_cred(), label, mp, flags, data);

[Severity: High]
Similarly, should AA_MS_IGNORE_MASK be stripped from flags here before
passing them to aa_remount()?

> +	__end_current_label_crit_section(label, needput);
> +
> +	return error;
> +}

[ ... ]

> @@ -1655,8 +1706,12 @@ static struct security_hook_list apparmor_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(capget, apparmor_capget),
>  	LSM_HOOK_INIT(capable, apparmor_capable),
>  
> -	LSM_HOOK_INIT(move_mount, apparmor_move_mount),
> -	LSM_HOOK_INIT(sb_mount, apparmor_sb_mount),
> +	LSM_HOOK_INIT(mount_bind, apparmor_mount_bind),
> +	LSM_HOOK_INIT(mount_new, apparmor_mount_new),
> +	LSM_HOOK_INIT(mount_remount, apparmor_mount_remount),
> +	LSM_HOOK_INIT(mount_reconfigure, apparmor_mount_reconfigure),
> +	LSM_HOOK_INIT(mount_move, apparmor_move_mount),
> +	LSM_HOOK_INIT(mount_change_type, apparmor_mount_change_type),
>  	LSM_HOOK_INIT(sb_umount, apparmor_sb_umount),
>  	LSM_HOOK_INIT(sb_pivotroot, apparmor_sb_pivotroot),

[Severity: Critical]
Are these new hooks being invoked by the VFS?

This patch removes the active sb_mount and move_mount hooks, replacing them
with granular hooks like mount_new and mount_bind. However, the VFS doesn't
appear to have been updated to call these new security hooks (e.g.,
security_mount_new() is never called in fs/namespace.c).

Could this completely disable AppArmor mount mediation, allowing userspace
to perform arbitrary mount operations regardless of the AppArmor profile?

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