[syzbot] KMSAN: uninit-value in pagecache_write

[syzbot] KMSAN: uninit-value in pagecache_write

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    968c2729e576 x86: kmsan: fix comment in kmsan_shadow.c
git tree:       https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=11d01ad6880000
kernel config:  https://syzkaller.appspot.com/x/.config?x=131312b26465c190
dashboard link: https://syzkaller.appspot.com/bug?extid=9767be679ef5016b6082
compiler:       clang version 15.0.0 (https://github.com/llvm/llvm-project.git 610139d2d9ce6746b3c617fb3e2f7886272d26ff), 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/c78ce21b953f/disk-968c2729.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/22868d826804/vmlinux-968c2729.xz

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

=====================================================
BUG: KMSAN: uninit-value in pagecache_write+0x655/0x720 fs/ext4/verity.c:91
 pagecache_write+0x655/0x720 fs/ext4/verity.c:91
 ext4_write_merkle_tree_block+0x84/0xa0 fs/ext4/verity.c:389
 build_merkle_tree_level+0x972/0x1250 fs/verity/enable.c:121
 build_merkle_tree fs/verity/enable.c:182 [inline]
 enable_verity+0xede/0x1920 fs/verity/enable.c:268
 fsverity_ioctl_enable+0x895/0xab0 fs/verity/enable.c:392
 __ext4_ioctl fs/ext4/ioctl.c:1572 [inline]
 ext4_ioctl+0x26dd/0x8c50 fs/ext4/ioctl.c:1606
 ext4_compat_ioctl+0x702/0x800 fs/ext4/ioctl.c:1682
 __do_compat_sys_ioctl fs/ioctl.c:968 [inline]
 __se_compat_sys_ioctl+0x781/0xfa0 fs/ioctl.c:910
 __ia32_compat_sys_ioctl+0x8f/0xd0 fs/ioctl.c:910
 do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
 __do_fast_syscall_32+0xa2/0x100 arch/x86/entry/common.c:178
 do_fast_syscall_32+0x33/0x70 arch/x86/entry/common.c:203
 do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:246
 entry_SYSENTER_compat_after_hwframe+0x70/0x82

Local variable fsdata created at:
 pagecache_write+0x21c/0x720 fs/ext4/verity.c:85
 ext4_write_merkle_tree_block+0x84/0xa0 fs/ext4/verity.c:389

CPU: 1 PID: 15121 Comm: syz-executor.3 Not tainted 6.0.0-rc5-syzkaller-48543-g968c2729e576 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/11/2022
=====================================================


---
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 syzkaller@googlegroups.com.

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

Re: [syzbot] KMSAN: uninit-value in pagecache_write

[Alexander Potapenko]

On Mon, Nov 7, 2022 at 10:10 AM syzbot
<syzbot+9767be679ef5016b6082@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:    968c2729e576 x86: kmsan: fix comment in kmsan_shadow.c
> git tree:       https://github.com/google/kmsan.git master
> console output: https://syzkaller.appspot.com/x/log.txt?x=11d01ad6880000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=131312b26465c190
> dashboard link: https://syzkaller.appspot.com/bug?extid=9767be679ef5016b6082
> compiler:       clang version 15.0.0 (https://github.com/llvm/llvm-project.git 610139d2d9ce6746b3c617fb3e2f7886272d26ff), 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/c78ce21b953f/disk-968c2729.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/22868d826804/vmlinux-968c2729.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+9767be679ef5016b6082@syzkaller.appspotmail.com
>
> =====================================================
> BUG: KMSAN: uninit-value in pagecache_write+0x655/0x720 fs/ext4/verity.c:91
>  pagecache_write+0x655/0x720 fs/ext4/verity.c:91
>  ext4_write_merkle_tree_block+0x84/0xa0 fs/ext4/verity.c:389
>  build_merkle_tree_level+0x972/0x1250 fs/verity/enable.c:121
>  build_merkle_tree fs/verity/enable.c:182 [inline]
>  enable_verity+0xede/0x1920 fs/verity/enable.c:268
>  fsverity_ioctl_enable+0x895/0xab0 fs/verity/enable.c:392
>  __ext4_ioctl fs/ext4/ioctl.c:1572 [inline]
>  ext4_ioctl+0x26dd/0x8c50 fs/ext4/ioctl.c:1606
>  ext4_compat_ioctl+0x702/0x800 fs/ext4/ioctl.c:1682
>  __do_compat_sys_ioctl fs/ioctl.c:968 [inline]
>  __se_compat_sys_ioctl+0x781/0xfa0 fs/ioctl.c:910
>  __ia32_compat_sys_ioctl+0x8f/0xd0 fs/ioctl.c:910
>  do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
>  __do_fast_syscall_32+0xa2/0x100 arch/x86/entry/common.c:178
>  do_fast_syscall_32+0x33/0x70 arch/x86/entry/common.c:203
>  do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:246
>  entry_SYSENTER_compat_after_hwframe+0x70/0x82
>
> Local variable fsdata created at:
>  pagecache_write+0x21c/0x720 fs/ext4/verity.c:85
>  ext4_write_merkle_tree_block+0x84/0xa0 fs/ext4/verity.c:389
>
> CPU: 1 PID: 15121 Comm: syz-executor.3 Not tainted 6.0.0-rc5-syzkaller-48543-g968c2729e576 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/11/2022
> =====================================================

This is identical to other reports fixed in
https://lore.kernel.org/lkml/20220915150417.722975-43-glider@google.com/
To fix the error, we need to initialize fsdata explicitly, because
aops->write_begin is not guaranteed to do so:

=============================================================================
   ext4: initialize fsdata in pagecache_write()

    When aops->write_begin() does not initialize fsdata, KMSAN reports
    an error passing the latter to aops->write_end().

    Fix this by unconditionally initializing fsdata.

    Fixes: c93d8f885809 ("ext4: add basic fs-verity support")
    Reported-by: syzbot+9767be679ef5016b6082@syzkaller.appspotmail.com
    Signed-off-by: Alexander Potapenko <glider@google.com>

diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
index 3c640bd7ecaeb..30e3b65798b50 100644
--- a/fs/ext4/verity.c
+++ b/fs/ext4/verity.c
@@ -79,7 +79,7 @@ static int pagecache_write(struct inode *inode,
const void *buf, size_t count,
                size_t n = min_t(size_t, count,
                                 PAGE_SIZE - offset_in_page(pos));
                struct page *page;
-               void *fsdata;
+               void *fsdata = NULL;
                int res;

                res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
=============================================================================

Re: [syzbot] KMSAN: uninit-value in pagecache_write

[Eric Biggers]

On Mon, Nov 07, 2022 at 10:46:13AM +0100, 'Alexander Potapenko' via syzkaller-bugs wrote:
>    ext4: initialize fsdata in pagecache_write()
> 
>     When aops->write_begin() does not initialize fsdata, KMSAN reports
>     an error passing the latter to aops->write_end().
> 
>     Fix this by unconditionally initializing fsdata.
> 
>     Fixes: c93d8f885809 ("ext4: add basic fs-verity support")
>     Reported-by: syzbot+9767be679ef5016b6082@syzkaller.appspotmail.com
>     Signed-off-by: Alexander Potapenko <glider@google.com>
> 
> diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> index 3c640bd7ecaeb..30e3b65798b50 100644
> --- a/fs/ext4/verity.c
> +++ b/fs/ext4/verity.c
> @@ -79,7 +79,7 @@ static int pagecache_write(struct inode *inode,
> const void *buf, size_t count,
>                 size_t n = min_t(size_t, count,
>                                  PAGE_SIZE - offset_in_page(pos));
>                 struct page *page;
> -               void *fsdata;
> +               void *fsdata = NULL;
>                 int res;
> 
>                 res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);

Are you sure that KMSAN should be reporting this?  The uninitialized value is
passed as a function parameter, but it's never actually used.

Anyway, this patch doesn't hurt, I suppose.  Can please you send it out as a
formal patch to linux-ext4?  It would be easy for people to miss this patch
buried in this thread.  Also, can you please send a patch to linux-f2fs-devel
for the same code in fs/f2fs/verity.c?

Thanks!

- Eric

Re: [syzbot] KMSAN: uninit-value in pagecache_write

[Alexander Potapenko]

On Mon, Nov 7, 2022 at 6:56 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Nov 07, 2022 at 10:46:13AM +0100, 'Alexander Potapenko' via syzkaller-bugs wrote:
> >    ext4: initialize fsdata in pagecache_write()
> >
> >     When aops->write_begin() does not initialize fsdata, KMSAN reports
> >     an error passing the latter to aops->write_end().
> >
> >     Fix this by unconditionally initializing fsdata.
> >
> >     Fixes: c93d8f885809 ("ext4: add basic fs-verity support")
> >     Reported-by: syzbot+9767be679ef5016b6082@syzkaller.appspotmail.com
> >     Signed-off-by: Alexander Potapenko <glider@google.com>
> >
> > diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> > index 3c640bd7ecaeb..30e3b65798b50 100644
> > --- a/fs/ext4/verity.c
> > +++ b/fs/ext4/verity.c
> > @@ -79,7 +79,7 @@ static int pagecache_write(struct inode *inode,
> > const void *buf, size_t count,
> >                 size_t n = min_t(size_t, count,
> >                                  PAGE_SIZE - offset_in_page(pos));
> >                 struct page *page;
> > -               void *fsdata;
> > +               void *fsdata = NULL;
> >                 int res;
> >
> >                 res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
>
> Are you sure that KMSAN should be reporting this?  The uninitialized value is
> passed as a function parameter, but it's never actually used.

To summarize what's written here:
https://lore.kernel.org/lkml/20220701142310.2188015-44-glider@google.com/
:
 - this is UB from the C11 standpoint;
 - Linus despises UB in general, but expecting function arguments to
be initialized is reasonable;
 - if a function ends up being a no-op, tools should not warn about
uninits passed to it;
 - KMSAN only warns about functions that survive inlining.

In addition, one cannot really rely on write_end() being consistent
with write_begin(), in particular if fsdata was ignored initially and
only appeared at a later point.

>
> Anyway, this patch doesn't hurt, I suppose.  Can please you send it out as a
> formal patch to linux-ext4?  It would be easy for people to miss this patch
> buried in this thread.  Also, can you please send a patch to linux-f2fs-devel
> for the same code in fs/f2fs/verity.c?

Will do!

> Thanks!
>
> - Eric



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [syzbot] KMSAN: uninit-value in pagecache_write

[Alexander Potapenko]

> >
> > Anyway, this patch doesn't hurt, I suppose.  Can please you send it out as a
> > formal patch to linux-ext4?  It would be easy for people to miss this patch
> > buried in this thread.  Also, can you please send a patch to linux-f2fs-devel
> > for the same code in fs/f2fs/verity.c?
>
> Will do!

Shall I also initialize fsdata here:

$ git grep 'void \*fsdata;'
fs/affs/file.c:         void *fsdata;
fs/ext4/verity.c:               void *fsdata;
fs/f2fs/verity.c:               void *fsdata;
fs/hfs/extent.c:                void *fsdata;
fs/hfsplus/extents.c:           void *fsdata;
fs/ocfs2/mmap.c:        void *fsdata;

?

Re: [syzbot] KMSAN: uninit-value in pagecache_write

[Eric Biggers]

On Tue, Nov 08, 2022 at 10:08:36AM +0100, 'Alexander Potapenko' via syzkaller-bugs wrote:
> > >
> > > Anyway, this patch doesn't hurt, I suppose.  Can please you send it out as a
> > > formal patch to linux-ext4?  It would be easy for people to miss this patch
> > > buried in this thread.  Also, can you please send a patch to linux-f2fs-devel
> > > for the same code in fs/f2fs/verity.c?
> >
> > Will do!
> 
> Shall I also initialize fsdata here:
> 
> $ git grep 'void \*fsdata;'
> fs/affs/file.c:         void *fsdata;
> fs/ext4/verity.c:               void *fsdata;
> fs/f2fs/verity.c:               void *fsdata;
> fs/hfs/extent.c:                void *fsdata;
> fs/hfsplus/extents.c:           void *fsdata;
> fs/ocfs2/mmap.c:        void *fsdata;

Yes, it looks like they all need this.  Except maybe ocfs2?  It's hard to tell.

- Eric

Re: [syzbot] KMSAN: uninit-value in pagecache_write

[Alexander Potapenko]

On Tue, Nov 8, 2022 at 6:41 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Nov 08, 2022 at 10:08:36AM +0100, 'Alexander Potapenko' via syzkaller-bugs wrote:
> > > >
> > > > Anyway, this patch doesn't hurt, I suppose.  Can please you send it out as a
> > > > formal patch to linux-ext4?  It would be easy for people to miss this patch
> > > > buried in this thread.  Also, can you please send a patch to linux-f2fs-devel
> > > > for the same code in fs/f2fs/verity.c?
> > >
> > > Will do!
> >
> > Shall I also initialize fsdata here:
> >
> > $ git grep 'void \*fsdata;'
> > fs/affs/file.c:         void *fsdata;
> > fs/ext4/verity.c:               void *fsdata;
> > fs/f2fs/verity.c:               void *fsdata;
> > fs/hfs/extent.c:                void *fsdata;
> > fs/hfsplus/extents.c:           void *fsdata;
> > fs/ocfs2/mmap.c:        void *fsdata;
>
> Yes, it looks like they all need this.  Except maybe ocfs2?  It's hard to tell.

For ocfs2 the begin/end functions are always the same, so it's harder
to mess fsdata up.
Guess we can say for now that __ocfs2_page_mkwrite() never passes an
uninitialized variable to another function.

> - Eric



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg