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
