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 <[email protected]> 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/[email protected]?part=8
