Re: [syzbot] WARNING: kmalloc bug in snd_pcm_plugin_alloc (2)

[Linus Torvalds]

On Wed, Mar 16, 2022 at 11:51 AM syzbot
<syzbot+72732c532ac1454eeee9@xxxxxxxxxxxxxxxxxxxxxxxxx> 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