[syzbot] [nilfs?] KMSAN: uninit-value in nilfs_add_checksums_on_logs

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 54b4a7d3d6c6 kmsan: add memsetXX tests
git tree: https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=143f81e4c80000
kernel config: https://syzkaller.appspot.com/x/.config?x=46c642641b9ef616
dashboard link: https://syzkaller.appspot.com/bug?extid=048585f3f4227bb2b49b
compiler: Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: i386

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/b2a457fb4580/disk-54b4a7d3.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/1acddc397ddb/vmlinux-54b4a7d3.xz
kernel image: https://storage.googleapis.com/syzbot-assets/eceee3367163/bzImage-54b4a7d3.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+048585...@syzkaller.appspotmail.com

=====================================================
BUG: KMSAN: uninit-value in crc32_body lib/crc32.c:110 [inline]
BUG: KMSAN: uninit-value in crc32_le_generic lib/crc32.c:179 [inline]
BUG: KMSAN: uninit-value in crc32_le_base+0x446/0xd80 lib/crc32.c:197
crc32_body lib/crc32.c:110 [inline]
crc32_le_generic lib/crc32.c:179 [inline]
crc32_le_base+0x446/0xd80 lib/crc32.c:197
nilfs_segbuf_fill_in_segsum_crc fs/nilfs2/segbuf.c:182 [inline]
nilfs_add_checksums_on_logs+0x2ce/0xe30 fs/nilfs2/segbuf.c:320
nilfs_segctor_do_construct+0xa553/0xe900 fs/nilfs2/segment.c:2076
nilfs_segctor_construct+0x1eb/0xe30 fs/nilfs2/segment.c:2379
nilfs_segctor_thread_construct fs/nilfs2/segment.c:2487 [inline]
nilfs_segctor_thread+0xc76/0x1240 fs/nilfs2/segment.c:2570
kthread+0x31f/0x430 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308

Uninit was stored to memory at:
nilfs_write_dat_node_binfo+0x12c/0x280 fs/nilfs2/segment.c:658
nilfs_segctor_assign fs/nilfs2/segment.c:1629 [inline]
nilfs_segctor_do_construct+0x753c/0xe900 fs/nilfs2/segment.c:2056
nilfs_segctor_construct+0x1eb/0xe30 fs/nilfs2/segment.c:2379
nilfs_segctor_thread_construct fs/nilfs2/segment.c:2487 [inline]
nilfs_segctor_thread+0xc76/0x1240 fs/nilfs2/segment.c:2570
kthread+0x31f/0x430 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308

Local variable binfo.i.i created at:
nilfs_segctor_update_payload_blocknr fs/nilfs2/segment.c:1562 [inline]
nilfs_segctor_assign fs/nilfs2/segment.c:1629 [inline]
nilfs_segctor_do_construct+0x6ac9/0xe900 fs/nilfs2/segment.c:2056
nilfs_segctor_construct+0x1eb/0xe30 fs/nilfs2/segment.c:2379

CPU: 1 PID: 28393 Comm: segctord Not tainted 6.2.0-syzkaller-81157-g54b4a7d3d6c6 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/02/2023
=====================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

[Tetsuo Handa]

syzbot is reporting uninit value at nilfs_add_checksums_on_logs() [1], for
nilfs_direct_assign_p() from nilfs_direct_assign() from nilfs_bmap_assign()
does not initialize "struct nilfs_binfo_dat"->bi_pad field.

We need to initialize sizeof("union nilfs_binfo"->bi_dat) bytes if
nilfs_write_dat_node_binfo() from nilfs_segctor_assign() copies it
and nilfs_add_checksums_on_logs() passes it to CRC function.

Reported-by: syzbot <syzbot+048585...@syzkaller.appspotmail.com>
Link: https://syzkaller.appspot.com/bug?extid=048585f3f4227bb2b49b [1]
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
---
I'm not sure whether this can fix the bug, for a reproducer is not yet
available...

fs/nilfs2/direct.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nilfs2/direct.c b/fs/nilfs2/direct.c
index a35f2795b242..4358b4581ec4 100644
--- a/fs/nilfs2/direct.c
+++ b/fs/nilfs2/direct.c
@@ -313,7 +313,8 @@ static int nilfs_direct_assign_p(struct nilfs_bmap *direct,
nilfs_direct_set_ptr(direct, key, blocknr);

binfo->bi_dat.bi_blkoff = cpu_to_le64(key);
- binfo->bi_dat.bi_level = 0;
+ /* initialize bi_pad field together while assigning bi_level field */
+ *(u64 *) &binfo->bi_dat.bi_level = (u64) 0;

return 0;
}
--
2.34.1

[Ryusuke Konishi]

Could you change this just to the initialization using bi_pad below?

memset(binfo->bi_dat.bi_pad, 0, sizeof(binfo->bi_dat.bi_pad));

This is not efficient depending on the compiler, but I'd rather avoid
the non-intuitive initialization using the above cast and use a
straightforward initialization.

This does not eliminate the problem, but it does fix one, so I'll send
it upstream.

Thanks,
Ryusuke Konishi

[Tetsuo Handa]

nilfs_direct_assign_p() from nilfs_direct_assign() from nilfs_bmap_assign()

is not initializing "struct nilfs_binfo_dat"->bi_pad field.

We need to initialize sizeof("union nilfs_binfo"->bi_dat) bytes if
nilfs_write_dat_node_binfo() from nilfs_segctor_assign() copies it
and nilfs_add_checksums_on_logs() passes it to CRC function.

Link: https://syzkaller.appspot.com/bug?extid=048585f3f4227bb2b49b

Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
---

Changes in v2:
Use memset() for initialization, suggested by Ryusuke Konishi.

fs/nilfs2/direct.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/fs/nilfs2/direct.c b/fs/nilfs2/direct.c
index a35f2795b242..4c85914f2abc 100644
--- a/fs/nilfs2/direct.c
+++ b/fs/nilfs2/direct.c
@@ -314,6 +314,7 @@ static int nilfs_direct_assign_p(struct nilfs_bmap *direct,

binfo->bi_dat.bi_blkoff = cpu_to_le64(key);
binfo->bi_dat.bi_level = 0;
+ memset(binfo->bi_dat.bi_pad, 0, sizeof(binfo->bi_dat.bi_pad));

return 0;
}
--
2.34.1

[Tetsuo Handa]

nilfs_btree_assign_p() and nilfs_direct_assign_p() are not initializing
"struct nilfs_binfo_dat"->bi_pad field, causing uninit-value reports
when being passed to CRC function.

Reported-by: syzbot <syzbot+048585...@syzkaller.appspotmail.com>

Link: https://syzkaller.appspot.com/bug?extid=048585f3f4227bb2b49b
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
---

Changes in v3:
Also fix nilfs_btree_assign_p().

Changes in v2:
Use memset() for initialization, suggested by Ryusuke Konishi.

fs/nilfs2/btree.c | 1 +
fs/nilfs2/direct.c | 1 +
2 files changed, 2 insertions(+)

diff --git a/fs/nilfs2/btree.c b/fs/nilfs2/btree.c
index 2681a449edc1..13592e82eaf6 100644
--- a/fs/nilfs2/btree.c
+++ b/fs/nilfs2/btree.c
@@ -2219,6 +2219,7 @@ static int nilfs_btree_assign_p(struct nilfs_bmap *btree,
/* on-disk format */
binfo->bi_dat.bi_blkoff = cpu_to_le64(key);
binfo->bi_dat.bi_level = level;

[Ryusuke Konishi]

Thank you for your cooperation, Handa-san.

I'll send this upstream, adding a Reported-by tag of the preceding report.

Thanks,
Ryusuke Konishi

[Ryusuke Konishi]

From: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>

nilfs_btree_assign_p() and nilfs_direct_assign_p() are not initializing
"struct nilfs_binfo_dat"->bi_pad field, causing uninit-value reports
when being passed to CRC function.

Reported-by: syzbot <syzbot+048585...@syzkaller.appspotmail.com>
Link: https://syzkaller.appspot.com/bug?extid=048585f3f4227bb2b49b

Reported-by: Dipanjan Das <mail.dip...@gmail.com>
Link: https://lkml.kernel.org/r/CANX2M5bVbzRi6zH3PTcNE_31...@mail.gmail.com
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
Signed-off-by: Ryusuke Konishi <konishi...@gmail.com>
---
Andrew, please pick this up, a fix for the KMSAN report.

Ryusuke Konishi

[Ryusuke Konishi]

Syzbot still reports uninit-value in nilfs_add_checksums_on_logs() for
KMSAN enabled kernels after applying commit 7397031622e0 ("nilfs2:
initialize "struct nilfs_binfo_dat"->bi_pad field").

This is because the unused bytes at the end of each block in segment
summaries are not initialized. So this fixes the issue by padding the
unused bytes with null bytes.

Signed-off-by: Ryusuke Konishi <konishi...@gmail.com>
Reported-by: syzbot+048585...@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=048585f3f4227bb2b49b
Tested-by: Ryusuke Konishi <konishi...@gmail.com>
Cc: sta...@vger.kernel.org
---
fs/nilfs2/segment.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index 6ad41390fa74..228659612c0d 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -430,6 +430,23 @@ static int nilfs_segctor_reset_segment_buffer(struct nilfs_sc_info *sci)
return 0;
}

+/**
+ * nilfs_segctor_zeropad_segsum - zero pad the rest of the segment summary area
+ * @sci: segment constructor object
+ *
+ * nilfs_segctor_zeropad_segsum() zero-fills unallocated space at the end of
+ * the current segment summary block.
+ */
+static void nilfs_segctor_zeropad_segsum(struct nilfs_sc_info *sci)
+{
+ struct nilfs_segsum_pointer *ssp;
+
+ ssp = sci->sc_blk_cnt > 0 ? &sci->sc_binfo_ptr : &sci->sc_finfo_ptr;
+ if (ssp->offset < ssp->bh->b_size)
+ memset(ssp->bh->b_data + ssp->offset, 0,
+ ssp->bh->b_size - ssp->offset);
+}
+
static int nilfs_segctor_feed_segment(struct nilfs_sc_info *sci)
{
sci->sc_nblk_this_inc += sci->sc_curseg->sb_sum.nblocks;
@@ -438,6 +455,7 @@ static int nilfs_segctor_feed_segment(struct nilfs_sc_info *sci)
* The current segment is filled up
* (internal code)
*/
+ nilfs_segctor_zeropad_segsum(sci);
sci->sc_curseg = NILFS_NEXT_SEGBUF(sci->sc_curseg);
return nilfs_segctor_reset_segment_buffer(sci);
}
@@ -542,6 +560,7 @@ static int nilfs_segctor_add_file_block(struct nilfs_sc_info *sci,
goto retry;
}
if (unlikely(required)) {
+ nilfs_segctor_zeropad_segsum(sci);
err = nilfs_segbuf_extend_segsum(segbuf);
if (unlikely(err))
goto failed;
@@ -1533,6 +1552,7 @@ static int nilfs_segctor_collect(struct nilfs_sc_info *sci,
nadd = min_t(int, nadd << 1, SC_MAX_SEGDELTA);
sci->sc_stage = prev_stage;
}
+ nilfs_segctor_zeropad_segsum(sci);
nilfs_segctor_truncate_segments(sci, sci->sc_curseg, nilfs->ns_sufile);
return 0;

--
2.34.1