Re: [PATCH v6 7/8] vfs: Replace security_sb_mount/security_move_mount with granular hooks

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

New issues:
- [Critical] The `data` buffer containing mount options is truncated before being evaluated by LSM hooks due to in-place modification.

Pre-existing issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition in `security_mount_new()` allows mount restriction bypass via concurrent overmounting.
--

--- Patch [7]: [PATCH v6 7/8] vfs: Replace security_sb_mount/security_move_mount with granular hooks ---
commit db44b5a9ddea0d89351b2678afb54e9fc292fef1
Author: Song Liu <[email protected]>

vfs: Replace security_sb_mount/security_move_mount with granular hooks

This commit replaces the monolithic security_sb_mount() and
security_move_mount() calls with new granular mount hooks at the individual
operation level. The new hooks receive appropriate context like resolved paths
and fs_context rather than raw string arguments. It also checks permissions
against pinned mountpoints for bind and move operations to avoid overmount
races.

> diff --git a/fs/namespace.c b/fs/namespace.c
> index 341ddd353b3a8..fef435c93e50e 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c

[ ... ]

> @@ -3396,6 +3417,9 @@ static int do_remount(const struct path *path, int sb_flags,
>  	fc->oldapi = true;
>  
>  	err = parse_monolithic_mount_data(fc, data);
> +	if (!err)
> +		err = security_mount_remount(fc, path, mnt_flags, flags,
> +					    data);

[Severity: Critical]
Does passing the data buffer to security_mount_remount() after calling
parse_monolithic_mount_data() result in the LSM seeing a truncated options
string?

The generic_parse_monolithic() parser uses strsep() and strchr() to mutate
the data string in-place, replacing commas and equal signs with null bytes.

When the buffer is subsequently passed to security_mount_remount(), will an
LSM (like AppArmor) only evaluate the first mount option, while the kernel
processes all options?

Could this allow an attacker to bypass mount option policies by placing
allowed options first, followed by restricted options?

>  	if (!err) {
>  		down_write(&sb->s_umount);
>  		err = -EPERM;

[ ... ]

> @@ -3833,6 +3885,9 @@ static int do_new_mount(const struct path *path, const char *fstype,
>  		err = parse_monolithic_mount_data(fc, data);
>  	if (!err && !mount_capable(fc))
>  		err = -EPERM;
> +
> +	if (!err)
> +		err = security_mount_new(fc, path, mnt_flags, flags, data);

[Severity: Critical]
Does passing the data buffer to security_mount_new() after calling
parse_monolithic_mount_data() also result in the LSM seeing a truncated
options string?

Similar to do_remount(), the string is modified in-place before the LSM hook,
potentially allowing mount restriction policies to be bypassed.

>  	if (!err)
>  		err = do_new_mount_fc(fc, path, mnt_flags);

[Severity: High]
This is a pre-existing issue, but does evaluating the unlocked path here open
a Time-of-Check to Time-of-Use (TOCTOU) race condition?

The security_mount_new() hook evaluates the caller-supplied path before
do_new_mount_fc() calls LOCK_MOUNT().

Could an attacker concurrently bind-mount a shared mount over the path
immediately after the LSM check?

When do_new_mount_fc() executes LOCK_MOUNT(), it would follow the new
overmount. The filesystem would then be grafted onto the shared mount and
propagate to restricted peers, bypassing the LSM's denial of mounting onto
those peers.

Can security_mount_new() be moved to run after the mountpoint is locked,
similar to how the TOCTOU protection was applied for bind and move mounts
in this commit?

>  
>  	put_fs_context(fc);
>  	return err;

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