Re: [PATCH 8/8] ntfs3: free replaced mount options after a grace period

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ntfs3: use-after-free of sbi->options during concurrent remount
- [Medium] vfs: ntfs3 silent mount option loss on remount
--

commit c093269bafa15fa705cf9dfc3164cc884b583d68
Author: Christian Brauner <brauner@kernel.org>
ntfs3: free replaced mount options after a grace period

This commit introduces an RCU grace period before freeing replaced mount
options in ntfs_fs_reconfigure() to resolve a use-after-free issue in
ntfs_nls_to_utf16() during pathwalks. It also introduces struct rcu_head
into struct ntfs_mount_options and updates readers like
ntfs_nls_to_utf16() to use RCU synchronization.

> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 841f9068d82f9..58ef6be3286b1 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -473,7 +479,15 @@ static int ntfs_fs_reconfigure(struct fs_context *fc)
>  	}
>  
>  	sync_filesystem(sb);
> -	swap(sbi->options, fc->fs_private);
> +	old_opts = sbi->options;
> +	/* pairs with the READ_ONCE() in ntfs_nls_to_utf16() */
> +	smp_store_release(&sbi->options, new_opts);
> +	fc->fs_private = NULL;
> +	/*
> +	 * ntfs_d_hash() and ntfs_d_compare() of a pathwalk in rcu mode may
> +	 * still read the old options through the sbi.
> +	 */
> +	call_rcu(&old_opts->rcu, put_mount_options_rcu);

[Severity: High]
Does this change leave other readers of sbi->options vulnerable to a
use-after-free?

While this patch protects ntfs_nls_to_utf16(), other VFS operations appear
to access sbi->options without holding rcu_read_lock() or s_umount. If a
thread is preempted immediately after loading the sbi->options pointer, and
a concurrent mount -o remount calls ntfs_fs_reconfigure() which replaces
sbi->options and schedules its destruction via call_rcu(), the grace period
can complete before the first thread resumes.

For example, during inode initialization:

fs/ntfs3/inode.c:ntfs_read_mft() {
    ...
    inode->i_uid = sbi->options->fs_uid;
    inode->i_gid = sbi->options->fs_gid;
    ...
}

Or during getattr operations:

fs/ntfs3/file.c:ntfs_fileattr_get() {
    ...
    if (sbi->options->nocase)
        flags |= FS_CASEFOLD_FL;
    ...
}

Or during directory traversal:

fs/ntfs3/dir.c:ntfs_dir_emit() {
    ...
    if (!sbi->options->showmeta && ntfs_is_meta_file(sbi, ino))
    ...
}

Will these unprotected accesses cause memory corruption when the options
are freed by the concurrent remount?

[Severity: Medium]
This problem wasn't introduced by this patch, but doesn't replacing
sbi->options with new_opts cause a silent loss of mount options during
remount?

When a user executes a remount without providing the full set of original
mount options, fc->fs_private is populated with a fresh, default-initialized
options struct in ntfs_init_fs_context():

fs/ntfs3/super.c:ntfs_init_fs_context() {
    ...
    if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE)
        goto ok;
    ...
}

Then in ntfs_fs_reconfigure(), the old options are entirely overwritten by
the new defaults, discarding any unspecified previous settings like uid, gid,
or umask:

    smp_store_release(&sbi->options, new_opts);

Can this lead to unintended permission changes or feature toggles by
resetting unspecified options to their default values?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-work-fixes-rcu-v1-0-7446d2d335e6@kernel.org?part=8