nilfs2: fix buffer corruption due to concurrent device reads

As a result of analysis of a syzbot report, it turned out that in three
cases where nilfs2 allocates block device buffers directly via sb_getblk,
concurrent reads to the device can corrupt the allocated buffers.

Nilfs2 uses sb_getblk for segment summary blocks, that make up a log
header, and the super root block, that is the trailer, and when moving and
writing the second super block after fs resize.

In any of these, since the uptodate flag is not set when storing metadata
to be written in the allocated buffers, the stored metadata will be
overwritten if a device read of the same block occurs concurrently before
the write.  This causes metadata corruption and misbehavior in the log
write itself, causing warnings in nilfs_btree_assign() as reported.

Fix these issues by setting an uptodate flag on the buffer head on the
first or before modifying each buffer obtained with sb_getblk, and
clearing the flag on failure.

When setting the uptodate flag, the lock_buffer/unlock_buffer pair is used
to perform necessary exclusive control, and the buffer is filled to ensure
that uninitialized bytes are not mixed into the data read from others.  As
for buffers for segment summary blocks, they are filled incrementally, so
if the uptodate flag was unset on their allocation, set the flag and zero
fill the buffer once at that point.

Also, regarding the superblock move routine, the starting point of the
memset call to zerofill the block is incorrectly specified, which can
cause a buffer overflow on file systems with block sizes greater than
4KiB.  In addition, if the superblock is moved within a large block, it is
necessary to assume the possibility that the data in the superblock will
be destroyed by zero-filling before copying.  So fix these potential
issues as well.

Link: https://lkml.kernel.org/r/20230609035732.20426-1-konishi.ryusuke@gmail.com
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Reported-by: syzbot+31837fe952932efc8fb9@syzkaller.appspotmail.com
Closes: https://lkml.kernel.org/r/00000000000030000a05e981f475@google.com
Tested-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

diff --git a/fs/nilfs2/segbuf.c b/fs/nilfs2/segbuf.c
index 1362ccb64ec7d0e2086f8efeb3a8dac277b8b5a8..6e59dc19a732498a64ebdbdea43fd14c88e2f0aa 100644 (file)
--- a/fs/nilfs2/segbuf.c
+++ b/fs/nilfs2/segbuf.c
@@ -101,6 +101,12 @@ int nilfs_segbuf_extend_segsum(struct nilfs_segment_buffer *segbuf)
        if (unlikely(!bh))
                return -ENOMEM;
 
+       lock_buffer(bh);
+       if (!buffer_uptodate(bh)) {
+               memset(bh->b_data, 0, bh->b_size);
+               set_buffer_uptodate(bh);
+       }
+       unlock_buffer(bh);
        nilfs_segbuf_add_segsum_buffer(segbuf, bh);
        return 0;
 }

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index ac949fd7603ff22f4c5654c3ae2ab9c06ccd92bd..c2553024bd25e651c38f99c414816a4bc57b51a1 100644 (file)
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -981,10 +981,13 @@ static void nilfs_segctor_fill_in_super_root(struct nilfs_sc_info *sci,
        unsigned int isz, srsz;
 
        bh_sr = NILFS_LAST_SEGBUF(&sci->sc_segbufs)->sb_super_root;
+
+       lock_buffer(bh_sr);
        raw_sr = (struct nilfs_super_root *)bh_sr->b_data;
        isz = nilfs->ns_inode_size;
        srsz = NILFS_SR_BYTES(isz);
 
+       raw_sr->sr_sum = 0;  /* Ensure initialization within this update */
        raw_sr->sr_bytes = cpu_to_le16(srsz);
        raw_sr->sr_nongc_ctime
                = cpu_to_le64(nilfs_doing_gc() ?
@@ -998,6 +1001,8 @@ static void nilfs_segctor_fill_in_super_root(struct nilfs_sc_info *sci,
        nilfs_write_inode_common(nilfs->ns_sufile, (void *)raw_sr +
                                 NILFS_SR_SUFILE_OFFSET(isz), 1);
        memset((void *)raw_sr + srsz, 0, nilfs->ns_blocksize - srsz);
+       set_buffer_uptodate(bh_sr);
+       unlock_buffer(bh_sr);
 }
 
 static void nilfs_redirty_inodes(struct list_head *head)
@@ -1780,6 +1785,7 @@ static void nilfs_abort_logs(struct list_head *logs, int err)
        list_for_each_entry(segbuf, logs, sb_list) {
                list_for_each_entry(bh, &segbuf->sb_segsum_buffers,
                                    b_assoc_buffers) {
+                       clear_buffer_uptodate(bh);
                        if (bh->b_page != bd_page) {
                                if (bd_page)
                                        end_page_writeback(bd_page);
@@ -1791,6 +1797,7 @@ static void nilfs_abort_logs(struct list_head *logs, int err)
                                    b_assoc_buffers) {
                        clear_buffer_async_write(bh);
                        if (bh == segbuf->sb_super_root) {
+                               clear_buffer_uptodate(bh);
                                if (bh->b_page != bd_page) {
                                        end_page_writeback(bd_page);
                                        bd_page = bh->b_page;

diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 77f1e5778d1c84504b14289e72ff89c52d6df9a8..9ba4933087af07f2f26dff0725d67b181c3a360a 100644 (file)
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -372,10 +372,31 @@ static int nilfs_move_2nd_super(struct super_block *sb, loff_t sb2off)
                goto out;
        }
        nsbp = (void *)nsbh->b_data + offset;
-       memset(nsbp, 0, nilfs->ns_blocksize);
 
+       lock_buffer(nsbh);
        if (sb2i >= 0) {
+               /*
+                * The position of the second superblock only changes by 4KiB,
+                * which is larger than the maximum superblock data size
+                * (= 1KiB), so there is no need to use memmove() to allow
+                * overlap between source and destination.
+                */
                memcpy(nsbp, nilfs->ns_sbp[sb2i], nilfs->ns_sbsize);
+
+               /*
+                * Zero fill after copy to avoid overwriting in case of move
+                * within the same block.
+                */
+               memset(nsbh->b_data, 0, offset);
+               memset((void *)nsbp + nilfs->ns_sbsize, 0,
+                      nsbh->b_size - offset - nilfs->ns_sbsize);
+       } else {
+               memset(nsbh->b_data, 0, nsbh->b_size);
+       }
+       set_buffer_uptodate(nsbh);
+       unlock_buffer(nsbh);
+
+       if (sb2i >= 0) {
                brelse(nilfs->ns_sbh[sb2i]);
                nilfs->ns_sbh[sb2i] = nsbh;
                nilfs->ns_sbp[sb2i] = nsbp;