lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 22 Jan 2023 11:53:08 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     syzbot <syzbot+4376a9a073770c173269@...kaller.appspotmail.com>
Cc:     akpm@...ux-foundation.org, clm@...com, dsterba@...e.com,
        dsterba@...e.cz, josef@...icpanda.com, linux-btrfs@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        syzkaller-bugs@...glegroups.com, w@....eu
Subject: Re: [syzbot] [btrfs?] WARNING: kmalloc bug in btrfs_ioctl_send

On Sun, Jan 22, 2023 at 3:14 AM syzbot
<syzbot+4376a9a073770c173269@...kaller.appspotmail.com> wrote:
>
> syzbot has bisected this issue to:
>
> commit 7661809d493b426e979f39ab512e3adf41fbcc69
> Author: Linus Torvalds <torvalds@...ux-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?

             Linus

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ