Re: [syzbot] [btrfs?] WARNING: kmalloc bug in btrfs_ioctl_send

[David Sterba <dsterba@suse.cz>]

On Sun, Jan 22, 2023 at 11:53:08AM -0800, Linus Torvalds wrote:
> On Sun, Jan 22, 2023 at 3:14 AM syzbot
> <syzbot+4376a9a073770c173269@syzkaller.appspotmail.com> wrote:
> >
> > syzbot has bisected this issue to:
> >
> > commit 7661809d493b426e979f39ab512e3adf41fbcc69
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date:   Wed Jul 14 16:45:49 2021 +0000
> >
> >     mm: don't allow oversized kvmalloc() calls
> 
> Heh. I assume this is the
> 
>         sctx->clone_roots = kvcalloc(sizeof(*sctx->clone_roots),
>                                      arg->clone_sources_count + 1,
>                                      GFP_KERNEL);
> 
> in btrfs_ioctl_send(), where the 'clone_sources_count' thing is
> basically just an argument to the btrfs ioctl, and user space can set
> it to anything it damn well likes.
> 
> So that warning is very much correct, and the problem is that the code
> doesn't do any  realsanity checking at all on the ioctl arguments, and
> basically allows the code to exhaust all memory.
> 
> Ok, there's a sanity check in the form of an overflow check:
> 
>         /*
>          * Check that we don't overflow at later allocations, we request
>          * clone_sources_count + 1 items, and compare to unsigned long inside
>          * access_ok.
>          */
>         if (arg->clone_sources_count >
>             ULONG_MAX / sizeof(struct clone_root) - 1) {
>                 ret = -EINVAL;
>                 goto out;
>         }
> 
> but ULONG_MAX is a *lot* of memory that the btrfs code is happy to try
> to allocate.
> 
> This ioctl does seem to be protected by a
> 
>         if (!capable(CAP_SYS_ADMIN))
>                 return -EPERM;
> 
> so at least it wasn't some kind of "random user can use up all memory".
> 
> I suspect the simplest way to make syzbot happy is to change the
> 
>         if (arg->clone_sources_count >
>             ULONG_MAX / sizeof(struct clone_root) - 1) {
> 
> test to use INT_MAX instead of ULONG_MAX, which will then match the
> vmalloc sanity check and avoid the warning.
> 
> But maybe an even smaller value might be more domain-appropriate here?

Real world number for clone_sources_count would be low, in tens or
hundreds at most.  Size of struct clone_root is 40 bytes, and 1M array
can hold about 26K items, which is enough for some realistic stress
testing. I'll set the limit for allocated memory to 8M which should be
generous (about 200K array items) and future proof in case new members
need to be added to clone_root.