On Wed, 16 Mar 2022 20:28:46 +0100, Linus Torvalds wrote:
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.
Indeed there seem missing value limit checks. Currently we rely on the fact that the parameters of the underlying PCM device have been already configured properly, and it assures that the original values are fine. OTOH, this PCM OSS layer does also conversions and it allocates temporary buffers for that. The problem happens with those converted parameters; depending on the sample rate, channels, and format, it may increases significantly, and this was the reason of the 31bit overflow.
And, we want not only avoiding the overflow but also limiting the actual size, too. Practically seen, more than 1MB temporary buffer is unrealistic, and better to bail if more than that is requested.
Blow is the fix patch. It works fine for local testing.
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Date: Thu, 17 Mar 2022 11:29:39 +0100 Subject: [PATCH] ALSA: oss: Fix PCM OSS buffer allocation overflow
We've got syzbot reports hitting INT_MAX overflow at vmalloc() allocation that is called from snd_pcm_plug_alloc(). Although we apply the restrictions to input parameters, it's based only on the hw_params of the underlying PCM device. Since the PCM OSS layer allocates a temporary buffer for the data conversion, the size may become unexpectedly large when more channels or higher rates is given; in the reported case, it went over INT_MAX, hence it hits WARN_ON().
This patch is an attempt to avoid such an overflow and an allocation for too large buffers. First off, it adds the limit of 1MB as the upper bound for period bytes. This must be large enough for all use cases, and we really don't want to handle a larger temporary buffer than this size. The size check is performed at two places, where the original period bytes is calculated and where the plugin buffer size is calculated.
In addition, the driver uses array_size() and array3_size() for multiplications to catch overflows for the converted period size and buffer bytes.
Reported-by: syzbot+72732c532ac1454eeee9@syzkaller.appspotmail.com Suggested-by: Linus Torvalds torvalds@linux-foundation.org Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/00000000000085b1b305da5a66f3@google.com Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/oss/pcm_oss.c | 12 ++++++++---- sound/core/oss/pcm_plugin.c | 5 ++++- 2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c index 3ee9edf85815..f158f0abd25d 100644 --- a/sound/core/oss/pcm_oss.c +++ b/sound/core/oss/pcm_oss.c @@ -774,6 +774,11 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream,
if (oss_period_size < 16) return -EINVAL; + + /* don't allocate too large period; 1MB period must be enough */ + if (oss_period_size > 1024 * 1024) + return -ENOMEM; + runtime->oss.period_bytes = oss_period_size; runtime->oss.period_frames = 1; runtime->oss.periods = oss_periods; @@ -1043,10 +1048,9 @@ static int snd_pcm_oss_change_params_locked(struct snd_pcm_substream *substream) goto failure; } #endif - oss_period_size *= oss_frame_size; - - oss_buffer_size = oss_period_size * runtime->oss.periods; - if (oss_buffer_size < 0) { + 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) { err = -EINVAL; goto failure; } diff --git a/sound/core/oss/pcm_plugin.c b/sound/core/oss/pcm_plugin.c index 061ba06bc926..82e180c776ae 100644 --- a/sound/core/oss/pcm_plugin.c +++ b/sound/core/oss/pcm_plugin.c @@ -62,7 +62,10 @@ static int snd_pcm_plugin_alloc(struct snd_pcm_plugin *plugin, snd_pcm_uframes_t width = snd_pcm_format_physical_width(format->format); if (width < 0) return width; - size = frames * format->channels * width; + size = array3_size(frames, format->channels, width); + /* check for too large period size once again */ + if (size > 1024 * 1024) + return -ENOMEM; if (snd_BUG_ON(size % 8)) return -ENXIO; size /= 8;