[PATCH] fat: fix data race in fat_add_entries() over unlocked buffer write

Igor Putko <[email protected]>
Newsgroups gmane.linux.kernel
Message-ID <[email protected]>
fat_add_entries() writes new directory entries directly into
bh->b_data via memcpy() without holding the buffer lock. This is
inconsistent with every other buffer-filling call site in this file
(fat_zeroed_cluster(), fat_alloc_new_dir(), fat_add_new_entries())
and in fs/fat/fatent.c (fat_mirror_bhs()), all of which wrap their
memcpy()/memset() calls in lock_buffer()/unlock_buffer() specifically
"to avoid race with userspace read via bdev".

Commit 07bfa4415ab6 ("fat: work around race with userspace's read
via blockdev while mounting") added that locking to those four call
sites in 2019, but missed the structurally identical pattern in
fat_add_entries()'s "Second stage: filling the free entries with new
entries" block, which writes both the long-name and short-name slots
into pre-existing (already uptodate) buffer heads the same way.

Since a FAT directory's buffer_head aliases a page in the backing
block device's page cache, a concurrent write into that page via the
bdev (e.g. through a loop device backed by shmem, as in the syzbot
reproducer) races with this unlocked memcpy(), as reported by KCSAN.

Wrap both the long-name and short-name slot writes in
fat_add_entries() with lock_buffer()/unlock_buffer(), matching the
pattern used everywhere else in this file.

Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=f72da8b30ddc89cc2371
Fixes: 07bfa4415ab6 ("fat: work around race with userspace's read via blockdev while mounting")
Signed-off-by: Igor Putko <[email protected]>
---
 fs/fat/dir.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 35bdb6294..c9e2bf472 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -1394,7 +1394,10 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
 		/* Fill the long name slots. */
 		for (i = 0; i < long_bhs; i++) {
 			int copy = umin(sb->s_blocksize - offset, size);
+			/* Avoid race with userspace read via bdev */
+			lock_buffer(bhs[i]);
 			memcpy(bhs[i]->b_data + offset, slots, copy);
+			unlock_buffer(bhs[i]);
 			mmb_mark_buffer_dirty(bhs[i],
 					      &MSDOS_I(dir)->i_metadata_bhs);
 			offset = 0;
@@ -1406,7 +1409,10 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
 		if (!err && i < nr_bhs) {
 			/* Fill the short name slot. */
 			int copy = umin(sb->s_blocksize - offset, size);
+			/* Avoid race with userspace read via bdev */
+			lock_buffer(bhs[i]);
 			memcpy(bhs[i]->b_data + offset, slots, copy);
+			unlock_buffer(bhs[i]);
 			mmb_mark_buffer_dirty(bhs[i],
 					      &MSDOS_I(dir)->i_metadata_bhs);
 			if (IS_DIRSYNC(dir))
-- 
2.47.3
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.