[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+14d9e7...@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 syzk...@googlegroups.com.

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

[Eric Biggers]

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

[Alexander Potapenko]

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) 

- 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

[David Sterba]

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.

[Alexander Potapenko]

Yeah, that's what I meant. Tried looking for kzvalloc and failed :) 

[Eric Biggers]

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

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