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

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Igor Putko <igorpetindev@gmail.com> writes:

> 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.

No. Commit 07bfa4415ab6 prevents the race about the non uptodate
buffer. However, because the your patch is adding the lock to the
uptodate buffer, it will not prevent any *read* side like that commit.

And looks like the syzkaller report would be about the stable page issue
that FAT is not supporting, or just temporary on disk corruption that
fixing later writes.

Thanks.

> Reported-by: syzbot+f72da8b30ddc89cc2371@syzkaller.appspotmail.com
> 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 <igorpetindev@gmail.com>
> ---
>  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))

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>