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

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 68453767131a ARM: Spectre-BHB: provide empty stub for non-..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11ddd329700000
kernel config: https://syzkaller.appspot.com/x/.config?x=442f8ac61e60a75e
dashboard link: https://syzkaller.appspot.com/bug?extid=72732c532ac1454eeee9
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13636d79700000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=128fcd03700000

The issue was bisected to:

commit 7661809d493b426e979f39ab512e3adf41fbcc69
Author: Linus Torvalds <torv...@linux-foundation.org>
Date: Wed Jul 14 16:45:49 2021 +0000

mm: don't allow oversized kvmalloc() calls

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=134eb7d1700000
final oops: https://syzkaller.appspot.com/x/report.txt?x=10ceb7d1700000
console output: https://syzkaller.appspot.com/x/log.txt?x=174eb7d1700000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+72732c...@syzkaller.appspotmail.com
Fixes: 7661809d493b ("mm: don't allow oversized kvmalloc() calls")

------------[ cut here ]------------
WARNING: CPU: 1 PID: 3761 at mm/util.c:591 kvmalloc_node+0x121/0x130 mm/util.c:591
Modules linked in:
CPU: 0 PID: 3761 Comm: syz-executor165 Not tainted 5.17.0-rc7-syzkaller-00227-g68453767131a #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:kvmalloc_node+0x121/0x130 mm/util.c:591
Code: eb 8e 45 31 e4 e9 49 ff ff ff e8 fa 91 d0 ff 41 81 e5 00 20 00 00 31 ff 44 89 ee e8 69 95 d0 ff 45 85 ed 75 dd e8 df 91 d0 ff <0f> 0b e9 22 ff ff ff 0f 1f 84 00 00 00 00 00 55 48 89 fd 53 e8 c6
RSP: 0018:ffffc9000282fb38 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffff88801c2d4d00 RCX: 0000000000000000
RDX: ffff88806c235700 RSI: ffffffff81a82e51 RDI: 0000000000000003
RBP: 00000000c0c0c100 R08: 0000000000000000 R09: 00000000ffffffff
R10: ffffffff81a82e47 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 00000000ffffffff R15: ffff88801c2d4d14
FS: 00007f4196580700(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffee56e9938 CR3: 000000006d5c7000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
kvmalloc include/linux/slab.h:731 [inline]
kvzalloc include/linux/slab.h:739 [inline]
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
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:874 [inline]
__se_sys_ioctl fs/ioctl.c:860 [inline]
__x64_sys_ioctl+0x193/0x200 fs/ioctl.c:860
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f41965cf1f9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 41 15 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f41965802f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f41966584a8 RCX: 00007f41965cf1f9
RDX: 0000000020000140 RSI: 00000000c0045002 RDI: 0000000000000003
RBP: 00007f41966584a0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f41966584ac
R13: 00007f4196625088 R14: 7364612f7665642f R15: 0000000000022000
</TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Linus Torvalds]

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

[Takashi Iwai]

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 <ti...@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+72732c...@syzkaller.appspotmail.com
Suggested-by: Linus Torvalds <torv...@linux-foundation.org>
Cc: <sta...@vger.kernel.org>
Link: https://lore.kernel.org/r/00000000000085...@google.com
Signed-off-by: Takashi Iwai <ti...@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;
--
2.34.1

[Linus Torvalds]

On Thu, Mar 17, 2022 at 7:13 AM Takashi Iwai <ti...@suse.de> wrote:
>
> 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.

Looks sane to me, although I obviously can't judge how well that 1M
limit works since I don't know the uses.

Thanks,
Linus