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