[syzbot] KMSAN: uninit-value in longest_match

[syzbot] KMSAN: uninit-value in longest_match

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    30d2727189c5 kmsan: fix memcpy tests
git tree:       https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=117d38f5880000
kernel config:  https://syzkaller.appspot.com/x/.config?x=a2144983ada8b4f3
dashboard link: https://syzkaller.appspot.com/bug?extid=14d9e7602ebdf7ec0a60
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/1e8c2d419c2e/disk-30d27271.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/9e8a728a72a9/vmlinux-30d27271.xz
kernel image: https://storage.googleapis.com/syzbot-assets/89f71c80c707/bzImage-30d27271.xz

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

=====================================================
BUG: KMSAN: uninit-value in longest_match+0xc88/0x1220 lib/zlib_deflate/deflate.c:668
 longest_match+0xc88/0x1220 lib/zlib_deflate/deflate.c:668
 deflate_fast+0x1838/0x2280 lib/zlib_deflate/deflate.c:954
 zlib_deflate+0x1783/0x22b0 lib/zlib_deflate/deflate.c:410
 zlib_compress_pages+0xd34/0x1f90 fs/btrfs/zlib.c:178
 compression_compress_pages fs/btrfs/compression.c:77 [inline]
 btrfs_compress_pages+0x325/0x440 fs/btrfs/compression.c:1208
 compress_file_range+0x11ac/0x3510 fs/btrfs/inode.c:730
 async_cow_start+0x33/0xd0 fs/btrfs/inode.c:1458
 btrfs_work_helper+0x55a/0x990 fs/btrfs/async-thread.c:280
 process_one_work+0xb27/0x13e0 kernel/workqueue.c:2289
 worker_thread+0x1076/0x1d60 kernel/workqueue.c:2436
 kthread+0x31b/0x430 kernel/kthread.c:376
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306

Uninit was created at:
 __kmalloc_large_node+0x2f7/0x3a0 mm/slab_common.c:1106
 __do_kmalloc_node mm/slab_common.c:943 [inline]
 __kmalloc_node+0x1d2/0x3c0 mm/slab_common.c:962
 kmalloc_node include/linux/slab.h:579 [inline]
 kvmalloc_node+0xbc/0x2d0 mm/util.c:581
 kvmalloc include/linux/slab.h:706 [inline]
 zlib_alloc_workspace+0x111/0x5e0 fs/btrfs/zlib.c:66
 alloc_workspace+0x329/0x5a0 fs/btrfs/compression.c:939
 btrfs_init_workspace_manager+0x11f/0x4d0 fs/btrfs/compression.c:982
 btrfs_init_compress+0x1f/0x30 fs/btrfs/compression.c:1249
 init_btrfs_fs+0x94/0x5f2 fs/btrfs/super.c:2736
 do_one_initcall+0x221/0x860 init/main.c:1303
 do_initcall_level+0x146/0x322 init/main.c:1376
 do_initcalls+0x11a/0x201 init/main.c:1392
 do_basic_setup+0x22/0x24 init/main.c:1411
 kernel_init_freeable+0x308/0x4d0 init/main.c:1631
 kernel_init+0x2b/0x7d0 init/main.c:1519
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306

CPU: 0 PID: 3530 Comm: kworker/u4:7 Not tainted 6.1.0-rc8-syzkaller-64144-g30d2727189c5 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
Workqueue: btrfs-delalloc btrfs_work_helper
=====================================================


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

[Eric Biggers]

On Fri, Dec 09, 2022 at 01:19:41AM -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    30d2727189c5 kmsan: fix memcpy tests
> git tree:       https://github.com/google/kmsan.git master
> console output: https://syzkaller.appspot.com/x/log.txt?x=117d38f5880000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=a2144983ada8b4f3
> dashboard link: https://syzkaller.appspot.com/bug?extid=14d9e7602ebdf7ec0a60
> 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/1e8c2d419c2e/disk-30d27271.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/9e8a728a72a9/vmlinux-30d27271.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/89f71c80c707/bzImage-30d27271.xz
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+14d9e7602ebdf7ec0a60@syzkaller.appspotmail.com
> 
> =====================================================
> BUG: KMSAN: uninit-value in longest_match+0xc88/0x1220 lib/zlib_deflate/deflate.c:668
>  longest_match+0xc88/0x1220 lib/zlib_deflate/deflate.c:668
>  deflate_fast+0x1838/0x2280 lib/zlib_deflate/deflate.c:954
>  zlib_deflate+0x1783/0x22b0 lib/zlib_deflate/deflate.c:410
>  zlib_compress_pages+0xd34/0x1f90 fs/btrfs/zlib.c:178
>  compression_compress_pages fs/btrfs/compression.c:77 [inline]
>  btrfs_compress_pages+0x325/0x440 fs/btrfs/compression.c:1208
>  compress_file_range+0x11ac/0x3510 fs/btrfs/inode.c:730
>  async_cow_start+0x33/0xd0 fs/btrfs/inode.c:1458
>  btrfs_work_helper+0x55a/0x990 fs/btrfs/async-thread.c:280
>  process_one_work+0xb27/0x13e0 kernel/workqueue.c:2289
>  worker_thread+0x1076/0x1d60 kernel/workqueue.c:2436
>  kthread+0x31b/0x430 kernel/kthread.c:376
>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306

zlib has long been known to use initialized values in longest_match().  This
issue is mentioned in the zlib FAQ.  I personally consider this to be a bug, as
the code could be written in a way such that it doesn't use uninitialized
memory.  However, zlib considers it to be "safe" and "working as intended".

Note that the copy of zlib in Linux is not really being maintained, and it is
based on a 25-year old version of zlib.  However, upstream zlib does not change
much anyway (it's very hard to get changes accepted into it), and as far as I
can tell even the latest version of upstream zlib has this same issue.

So I suppose the way to resolve this syzbot report is to just add
__no_kmsan_checks to longest_match().  The real issue, though, is that zlib
hasn't kept up with the times (nor has Linux kept up with zlib).

- Eric

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

[David Sterba]

On Wed, Dec 14, 2022 at 02:56:56PM +0100, Alexander Potapenko wrote:
> On Tue, Dec 13, 2022 at 7:40 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > On Fri, Dec 09, 2022 at 01:19:41AM -0800, syzbot wrote:
> > > Hello,
> > >
> > > syzbot found the following issue on:
> > >
> > > HEAD commit:    30d2727189c5 kmsan: fix memcpy tests
> > > git tree:       https://github.com/google/kmsan.git master
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=117d38f5880000
> > > kernel config:
> > https://syzkaller.appspot.com/x/.config?x=a2144983ada8b4f3
> > > dashboard link:
> > https://syzkaller.appspot.com/bug?extid=14d9e7602ebdf7ec0a60
> > > 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/1e8c2d419c2e/disk-30d27271.raw.xz
> > > vmlinux:
> > https://storage.googleapis.com/syzbot-assets/9e8a728a72a9/vmlinux-30d27271.xz
> > > kernel image:
> > https://storage.googleapis.com/syzbot-assets/89f71c80c707/bzImage-30d27271.xz
> > >
> > > IMPORTANT: if you fix the issue, please add the following tag to the
> > commit:
> > > Reported-by: syzbot+14d9e7602ebdf7ec0a60@syzkaller.appspotmail.com
> > >
> > > =====================================================
> > > BUG: KMSAN: uninit-value in longest_match+0xc88/0x1220
> > lib/zlib_deflate/deflate.c:668
> > >  longest_match+0xc88/0x1220 lib/zlib_deflate/deflate.c:668
> > >  deflate_fast+0x1838/0x2280 lib/zlib_deflate/deflate.c:954
> > >  zlib_deflate+0x1783/0x22b0 lib/zlib_deflate/deflate.c:410
> > >  zlib_compress_pages+0xd34/0x1f90 fs/btrfs/zlib.c:178
> > >  compression_compress_pages fs/btrfs/compression.c:77 [inline]
> > >  btrfs_compress_pages+0x325/0x440 fs/btrfs/compression.c:1208
> > >  compress_file_range+0x11ac/0x3510 fs/btrfs/inode.c:730
> > >  async_cow_start+0x33/0xd0 fs/btrfs/inode.c:1458
> > >  btrfs_work_helper+0x55a/0x990 fs/btrfs/async-thread.c:280
> > >  process_one_work+0xb27/0x13e0 kernel/workqueue.c:2289
> > >  worker_thread+0x1076/0x1d60 kernel/workqueue.c:2436
> > >  kthread+0x31b/0x430 kernel/kthread.c:376
> > >  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> >
> > zlib has long been known to use initialized values in longest_match().
> > This
> > issue is mentioned in the zlib FAQ.  I personally consider this to be a
> > bug, as
> > the code could be written in a way such that it doesn't use uninitialized
> > memory.  However, zlib considers it to be "safe" and "working as intended".
> >
> > Note that the copy of zlib in Linux is not really being maintained, and it
> > is
> > based on a 25-year old version of zlib.  However, upstream zlib does not
> > change
> > much anyway (it's very hard to get changes accepted into it), and as far
> > as I
> > can tell even the latest version of upstream zlib has this same issue.
> >
> > So I suppose the way to resolve this syzbot report is to just add
> > __no_kmsan_checks to longest_match().  The real issue, though, is that zlib
> > hasn't kept up with the times (nor has Linux kept up with zlib).
> >
> >
> Can't we just pass __GFP_ZERO when allocating the workspace here:
> 
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index b4f44662cda7c..23dc5628f8209 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -63,7 +63,8 @@ struct list_head *zlib_alloc_workspace(unsigned int level)
> 
>         workspacesize = max(zlib_deflate_workspacesize(MAX_WBITS,
> MAX_MEM_LEVEL),
>                         zlib_inflate_workspacesize());
> -       workspace->strm.workspace = kvmalloc(workspacesize, GFP_KERNEL);
> +       workspace->strm.workspace = kvmalloc(workspacesize,
> +                                            GFP_KERNEL | __GFP_ZERO);

Currently none of the compression workspaces does allocation with
zeroing. I'm not sure if we should actually zero the work memory right
before use, in the *get_workspace helpers so that each compression
starts from the same state. But this will be a performance hit and not
actually necessary if it's not required by the compression methods.

Which would leave only the allocation as the place to zero the memory.
If it's really just zlib that needs that then Ok, I'd suggest to use the
kvzalloc instead of __GFP_ZERO.

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

[Eric Biggers]

On Wed, Dec 14, 2022 at 02:56:56PM +0100, 'Alexander Potapenko' via syzkaller-bugs wrote:
> On Tue, Dec 13, 2022 at 7:40 AM Eric Biggers <ebiggers@kernel.org> wrote:
> 
> > On Fri, Dec 09, 2022 at 01:19:41AM -0800, syzbot wrote:
> > > Hello,
> > >
> > > syzbot found the following issue on:
> > >
> > > HEAD commit:    30d2727189c5 kmsan: fix memcpy tests
> > > git tree:       https://github.com/google/kmsan.git master
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=117d38f5880000
> > > kernel config:
> > https://syzkaller.appspot.com/x/.config?x=a2144983ada8b4f3
> > > dashboard link:
> > https://syzkaller.appspot.com/bug?extid=14d9e7602ebdf7ec0a60
> > > 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/1e8c2d419c2e/disk-30d27271.raw.xz
> > > vmlinux:
> > https://storage.googleapis.com/syzbot-assets/9e8a728a72a9/vmlinux-30d27271.xz
> > > kernel image:
> > https://storage.googleapis.com/syzbot-assets/89f71c80c707/bzImage-30d27271.xz
> > >
> > > IMPORTANT: if you fix the issue, please add the following tag to the
> > commit:
> > > Reported-by: syzbot+14d9e7602ebdf7ec0a60@syzkaller.appspotmail.com
> > >
> > > =====================================================
> > > BUG: KMSAN: uninit-value in longest_match+0xc88/0x1220
> > lib/zlib_deflate/deflate.c:668
> > >  longest_match+0xc88/0x1220 lib/zlib_deflate/deflate.c:668
> > >  deflate_fast+0x1838/0x2280 lib/zlib_deflate/deflate.c:954
> > >  zlib_deflate+0x1783/0x22b0 lib/zlib_deflate/deflate.c:410
> > >  zlib_compress_pages+0xd34/0x1f90 fs/btrfs/zlib.c:178
> > >  compression_compress_pages fs/btrfs/compression.c:77 [inline]
> > >  btrfs_compress_pages+0x325/0x440 fs/btrfs/compression.c:1208
> > >  compress_file_range+0x11ac/0x3510 fs/btrfs/inode.c:730
> > >  async_cow_start+0x33/0xd0 fs/btrfs/inode.c:1458
> > >  btrfs_work_helper+0x55a/0x990 fs/btrfs/async-thread.c:280
> > >  process_one_work+0xb27/0x13e0 kernel/workqueue.c:2289
> > >  worker_thread+0x1076/0x1d60 kernel/workqueue.c:2436
> > >  kthread+0x31b/0x430 kernel/kthread.c:376
> > >  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> >
> > zlib has long been known to use initialized values in longest_match().
> > This
> > issue is mentioned in the zlib FAQ.  I personally consider this to be a
> > bug, as
> > the code could be written in a way such that it doesn't use uninitialized
> > memory.  However, zlib considers it to be "safe" and "working as intended".
> >
> > Note that the copy of zlib in Linux is not really being maintained, and it
> > is
> > based on a 25-year old version of zlib.  However, upstream zlib does not
> > change
> > much anyway (it's very hard to get changes accepted into it), and as far
> > as I
> > can tell even the latest version of upstream zlib has this same issue.
> >
> > So I suppose the way to resolve this syzbot report is to just add
> > __no_kmsan_checks to longest_match().  The real issue, though, is that zlib
> > hasn't kept up with the times (nor has Linux kept up with zlib).
> >
> >
> Can't we just pass __GFP_ZERO when allocating the workspace here:
> 
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index b4f44662cda7c..23dc5628f8209 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -63,7 +63,8 @@ struct list_head *zlib_alloc_workspace(unsigned int level)
> 
>         workspacesize = max(zlib_deflate_workspacesize(MAX_WBITS,
> MAX_MEM_LEVEL),
>                         zlib_inflate_workspacesize());
> -       workspace->strm.workspace = kvmalloc(workspacesize, GFP_KERNEL);
> +       workspace->strm.workspace = kvmalloc(workspacesize,
> +                                            GFP_KERNEL | __GFP_ZERO);
>         workspace->level = level;
>         workspace->buf = NULL;
>         /*
> 
> ?
> 
> This is what Chrome folks did in the same situation (
> https://chromium.googlesource.com/chromium/src.git/+/962cbbe81708214ff8e14e2bc8a07271cb15f1b9
> )

Sure, the btrfs folks can do that if they want to avoid this warning for the
btrfs zlib compression specifically.  This would not solve the issue for the
other users of zlib compression in the kernel.

- Eric

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

[Alexander Potapenko]

> > Can't we just pass __GFP_ZERO when allocating the workspace here:
> >
> > ...
> > This is what Chrome folks did in the same situation (
> > https://chromium.googlesource.com/chromium/src.git/+/962cbbe81708214ff8e14e2bc8a07271cb15f1b9
> > )
>
> Sure, the btrfs folks can do that if they want to avoid this warning for the
> btrfs zlib compression specifically.  This would not solve the issue for the
> other users of zlib compression in the kernel.

We probably can't fix zlib without sacrificing performance, so I think
the right thing to do is to actually modify the users of zlib
compression to pass only initialized data to zlib.
Doing so will make the calls to zlib_deflate() well-defined, whereas
marking longest_match() as buggy will just hide future error reports,
but won't fix the code in question.