ALSA: pcm: Check for integer overflow during multiplication

Commit Message

Banajit Goswami May 28, 2019, 5:27 a.m. UTC

From: Phani Kumar Uppalapati <[e-mail]>

Channel info data structure is parsed from userspace and if
the number of channels is not set correctly, it could lead
to integer overflow when the number of channels is multiplied
with pcm bit width. Add a condition to check for integer
overflow during the multiplication operationi, and return error
if overflow detected.

Signed-off-by: Phani Kumar Uppalapati <[e-mail]>
Signed-off-by: Banajit Goswami <[e-mail]>
---
 sound/core/pcm_lib.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Takashi Iwai May 28, 2019, 5:47 a.m. UTC | #1

On Tue, 28 May 2019 07:27:03 +0200,
<[e-mail]> wrote:
> 
> From: Phani Kumar Uppalapati <[e-mail]>
> 
> Channel info data structure is parsed from userspace and if
> the number of channels is not set correctly, it could lead
> to integer overflow when the number of channels is multiplied
> with pcm bit width. Add a condition to check for integer
> overflow during the multiplication operationi, and return error
> if overflow detected.
> 
> Signed-off-by: Phani Kumar Uppalapati <[e-mail]>
> Signed-off-by: Banajit Goswami <[e-mail]>

Did you really hit this?

The info->channel value is already checked in snd_pcm_channel_info()
before calling the ioctl ops, to the upper bound runtime->channels.
So it shouldn't overflow at the point you suggested.


thanks,

Takashi

Banajit Goswami June 3, 2019, 4:43 a.m. UTC | #2

Thanks Takashi for the review!

On 5/27/2019 10:47 PM, Takashi Iwai wrote:
> On Tue, 28 May 2019 07:27:03 +0200,
> <[e-mail]> wrote:
>> From: Phani Kumar Uppalapati <[e-mail]>
>>
>> Channel info data structure is parsed from userspace and if
>> the number of channels is not set correctly, it could lead
>> to integer overflow when the number of channels is multiplied
>> with pcm bit width. Add a condition to check for integer
>> overflow during the multiplication operationi, and return error
>> if overflow detected.
>>
>> Signed-off-by: Phani Kumar Uppalapati <[e-mail]>
>> Signed-off-by: Banajit Goswami <[e-mail]>
> Did you really hit this?

This was reported by static analysis tool.

I will take your feedback, and re-look at the issue, to see if this 
issue can happen.

> The info->channel value is already checked in snd_pcm_channel_info()
> before calling the ioctl ops, to the upper bound runtime->channels.
> So it shouldn't overflow at the point you suggested.
>
>
> thanks,
>
> Takashi

Patch

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 345ab1a..f45ae3a 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1718,6 +1718,11 @@  static int snd_pcm_lib_ioctl_channel_info(struct snd_pcm_substream *substream,
 	switch (runtime->access) {
 	case SNDRV_PCM_ACCESS_MMAP_INTERLEAVED:
 	case SNDRV_PCM_ACCESS_RW_INTERLEAVED:
+		if ((UINT_MAX/width) < info->channel) {
+			snd_printd("%s: integer overflow in multiplication\n",
+				   __func__);
+			return -EINVAL;
+		}
 		info->first = info->channel * width;
 		info->step = runtime->channels * width;
 		break;
@@ -1725,6 +1730,12 @@  static int snd_pcm_lib_ioctl_channel_info(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_ACCESS_RW_NONINTERLEAVED:
 	{
 		size_t size = runtime->dma_bytes / runtime->channels;
+
+		if ((size > 0) && ((UINT_MAX/(size * 8)) < info->channel)) {
+			snd_printd("%s: integer overflow in multiplication\n",
+				   __func__);
+			return -EINVAL;
+		}
 		info->first = info->channel * size * 8;
 		info->step = width;
 		break;