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

[Takashi Iwai]

On Wed, 16 Mar 2022 20:28:46 +0100,
Linus Torvalds wrote:
> 
> 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.

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@xxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxxxxxx
Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
Link: https://lore.kernel.org/r/00000000000085b1b305da5a66f3@xxxxxxxxxx
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
 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;
-- 
2.34.1