Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <00000000000085b1b305da5a66f3@google.com>
In-Reply-To: <00000000000085b1b305da5a66f3@google.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 16 Mar 2022 12:28:46 -0700
Message-ID: <CAHk-=wjLNbf7viXP74K59jK=sRkg6mUbj0i3qpQvy9_2S4Lbtg@mail.gmail.com>
Subject: Re: [syzbot] WARNING: kmalloc bug in snd_pcm_plugin_alloc (2)
To:     syzbot <syzbot+72732c532ac1454eeee9@syzkaller.appspotmail.com>,
        Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        alsa-devel@alsa-project.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        Willy Tarreau <w@1wt.eu>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Mar 16, 2022 at 11:51 AM syzbot
<syzbot+72732c532ac1454eeee9@syzkaller.appspotmail.com> wrote:
>
> WARNING: CPU: 1 PID: 3761 at mm/util.c:591 kvmalloc_node+0x121/0x130 mm/util.c:591
>  snd_pcm_plugin_alloc+0x570/0x770 sound/core/oss/pcm_plugin.c:71
>  snd_pcm_plug_alloc+0x20d/0x310 sound/core/oss/pcm_plugin.c:118
>  snd_pcm_oss_change_params_locked+0x19db/0x3bf0 sound/core/oss/pcm_oss.c:1041
>  snd_pcm_oss_change_params sound/core/oss/pcm_oss.c:1104 [inline]
>  snd_pcm_oss_get_active_substream+0x164/0x1c0 sound/core/oss/pcm_oss.c:1121
>  snd_pcm_oss_get_rate sound/core/oss/pcm_oss.c:1778 [inline]
>  snd_pcm_oss_set_rate sound/core/oss/pcm_oss.c:1770 [inline]
>  snd_pcm_oss_ioctl+0x144f/0x3430 sound/core/oss/pcm_oss.c:2632

Well, that looks like a real bug in the sound subsystem, and the
warning is appropriate.

It looks like

        size = frames * format->channels * width;

can overflow 32 bits, and this is presumably user-triggerable with
snd_pcm_oss_ioctl().

Maybe there's some range check at an upper layer that is supposed to
catch this, but I'm not seeing it.

I think the simple fix is to do

        size = array3_size(frames, format->channels, width);

instead, which clamps the values at the maximum size_t.

Then you can trivially check for that overflow value (SIZE_MAX), but
you can - and probably should - just check for some sane value.
INT_MAX comes to mind, since that's what the allocation routine will
warn about.

But you can also say "Ok, I have now used the 'array_size()' function
to make sure any overflow will clamp to a very high value, so I know
I'll get an allocation failure, and I'd rather just make the allocator
do the size checking, so I'll add __GFP_NOWARN at allocation time and
just return -ENOMEM when that fails".

But that __GFP_NOWARN is *ONLY* acceptable if you have actually made
sure that "yes, all my size calculations have checked for overflow
and/or done that SIZE_MAX clamping".

Alternatively, you can just do each multiplication carefully, and use
"check_mul_overflow()" by hand, but it's a lot more inconvenient and
the end result tends to look horrible. There's a reason we have those
"array_size()" and "array3_size()" helpers.

There is also some very odd and suspicious-looking code in
snd_pcm_oss_change_params_locked():

        oss_period_size *= oss_frame_size;

        oss_buffer_size = oss_period_size * runtime->oss.periods;
        if (oss_buffer_size < 0) {
                err = -EINVAL;
                goto failure;
        }

which seems to think that checking the end result for being negative
is how you check for overflow. But that's actually after the
snd_pcm_plug_alloc() call.

It looks like all of this should use "check_mul_overflow()", but it
presumably also wants fixing (and also would like to use the
'array_size()' helpers, but note that those take a 'size_t', so you do
want to check for negative values *before* if you allow zeroes
anywhere else)

If you don't mind "multiplying by zero will hide a negative
intermediate value", you can pass in 'ssize_t' arguments, do the
multiplication as unsigned, put the result in a 'ssize_t' value, and
just check for a negative result.

That would seem to be acceptable here, and that
snd_pcm_oss_change_params_locked() code could also just be

        oss_period_size = array_size(oss_period_size, oss_frame_size);
        oss_buffer_size = array_size(oss_period_size, runtime->oss.periods);
        if (oss_buffer_size < 0) {
                ...

but I would suggest checking for a zero result too, because that can
hide the sub-parts having been some invalid crazy values that can also
cause problems later.

Takashi?

                 Linus