* [PATCH -next] ALSA: Fix oversized kvmalloc() calls
@ 2021-11-30 11:16 Bixuan Cui
2021-11-30 11:39 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Bixuan Cui @ 2021-11-30 11:16 UTC (permalink / raw)
To: alsa-devel, inux-kernel; +Cc: cuibixuan, tiwai
The commit 7661809d493b ("mm: don't allow oversized kvmalloc()
calls") limits the max allocatable memory via kvzalloc() to MAX_INT.
Reported-by: syzbot+bb348e9f9a954d42746f@syzkaller.appspotmail.com
Signed-off-by: Bixuan Cui <cuibixuan@linux.alibaba.com>
---
sound/core/oss/pcm_plugin.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sound/core/oss/pcm_plugin.c b/sound/core/oss/pcm_plugin.c
index 061ba06..61fccb5 100644
--- a/sound/core/oss/pcm_plugin.c
+++ b/sound/core/oss/pcm_plugin.c
@@ -68,6 +68,10 @@ static int snd_pcm_plugin_alloc(struct snd_pcm_plugin *plugin, snd_pcm_uframes_t
size /= 8;
if (plugin->buf_frames < frames) {
kvfree(plugin->buf);
+
+ if (size > INT_MAX)
+ return -ENOMEM;
+
plugin->buf = kvzalloc(size, GFP_KERNEL);
plugin->buf_frames = frames;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH -next] ALSA: Fix oversized kvmalloc() calls 2021-11-30 11:16 [PATCH -next] ALSA: Fix oversized kvmalloc() calls Bixuan Cui @ 2021-11-30 11:39 ` Takashi Iwai 2021-11-30 14:05 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2021-11-30 11:39 UTC (permalink / raw) To: Bixuan Cui; +Cc: alsa-devel, tiwai, inux-kernel On Tue, 30 Nov 2021 12:16:18 +0100, Bixuan Cui wrote: > > The commit 7661809d493b ("mm: don't allow oversized kvmalloc() > calls") limits the max allocatable memory via kvzalloc() to MAX_INT. > > Reported-by: syzbot+bb348e9f9a954d42746f@syzkaller.appspotmail.com > Signed-off-by: Bixuan Cui <cuibixuan@linux.alibaba.com> We should check the allocation size a lot earlier than here. IOW, such a big size shouldn't have been passed to this function but it should have been handled as an error in the caller side (snd_pcm_oss_change_params*()). Could you give the reproducer? thanks, Takashi > --- > sound/core/oss/pcm_plugin.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/sound/core/oss/pcm_plugin.c b/sound/core/oss/pcm_plugin.c > index 061ba06..61fccb5 100644 > --- a/sound/core/oss/pcm_plugin.c > +++ b/sound/core/oss/pcm_plugin.c > @@ -68,6 +68,10 @@ static int snd_pcm_plugin_alloc(struct snd_pcm_plugin *plugin, snd_pcm_uframes_t > size /= 8; > if (plugin->buf_frames < frames) { > kvfree(plugin->buf); > + > + if (size > INT_MAX) > + return -ENOMEM; > + > plugin->buf = kvzalloc(size, GFP_KERNEL); > plugin->buf_frames = frames; > } > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] ALSA: Fix oversized kvmalloc() calls 2021-11-30 11:39 ` Takashi Iwai @ 2021-11-30 14:05 ` Takashi Iwai 2021-12-01 6:16 ` Bixuan Cui 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2021-11-30 14:05 UTC (permalink / raw) To: Bixuan Cui; +Cc: alsa-devel, tiwai, inux-kernel On Tue, 30 Nov 2021 12:39:27 +0100, Takashi Iwai wrote: > > On Tue, 30 Nov 2021 12:16:18 +0100, > Bixuan Cui wrote: > > > > The commit 7661809d493b ("mm: don't allow oversized kvmalloc() > > calls") limits the max allocatable memory via kvzalloc() to MAX_INT. > > > > Reported-by: syzbot+bb348e9f9a954d42746f@syzkaller.appspotmail.com > > Signed-off-by: Bixuan Cui <cuibixuan@linux.alibaba.com> > > We should check the allocation size a lot earlier than here. > IOW, such a big size shouldn't have been passed to this function but > it should have been handled as an error in the caller side > (snd_pcm_oss_change_params*()). > > Could you give the reproducer? I'm asking it because the patch like below might cover the case. Takashi -- 8< -- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] ALSA: pcm: oss: Fix negative period/buffer sizes The period size calculation in OSS layer may receive a negative value as an error, but the code there assumes only the positive values and handle them with size_t. Due to that, a too big value may be passed to the lower layers. This patch changes the code to handle with ssize_t and adds the proper error checks appropriately. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/oss/pcm_oss.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c index 82a818734a5f..bec7590bc84b 100644 --- a/sound/core/oss/pcm_oss.c +++ b/sound/core/oss/pcm_oss.c @@ -147,7 +147,7 @@ snd_pcm_hw_param_value_min(const struct snd_pcm_hw_params *params, * * Return the maximum value for field PAR. */ -static unsigned int +static int snd_pcm_hw_param_value_max(const struct snd_pcm_hw_params *params, snd_pcm_hw_param_t var, int *dir) { @@ -682,18 +682,24 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *oss_params, struct snd_pcm_hw_params *slave_params) { - size_t s; - size_t oss_buffer_size, oss_period_size, oss_periods; - size_t min_period_size, max_period_size; + ssize_t s; + ssize_t oss_buffer_size; + ssize_t oss_period_size, oss_periods; + ssize_t min_period_size, max_period_size; struct snd_pcm_runtime *runtime = substream->runtime; size_t oss_frame_size; oss_frame_size = snd_pcm_format_physical_width(params_format(oss_params)) * params_channels(oss_params) / 8; + oss_buffer_size = snd_pcm_hw_param_value_max(slave_params, + SNDRV_PCM_HW_PARAM_BUFFER_SIZE, + NULL); + if (oss_buffer_size <= 0) + return -EINVAL; oss_buffer_size = snd_pcm_plug_client_size(substream, - snd_pcm_hw_param_value_max(slave_params, SNDRV_PCM_HW_PARAM_BUFFER_SIZE, NULL)) * oss_frame_size; - if (!oss_buffer_size) + oss_buffer_size * oss_frame_size); + if (oss_buffer_size <= 0) return -EINVAL; oss_buffer_size = rounddown_pow_of_two(oss_buffer_size); if (atomic_read(&substream->mmap_count)) { @@ -730,7 +736,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, min_period_size = snd_pcm_plug_client_size(substream, snd_pcm_hw_param_value_min(slave_params, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, NULL)); - if (min_period_size) { + if (min_period_size > 0) { min_period_size *= oss_frame_size; min_period_size = roundup_pow_of_two(min_period_size); if (oss_period_size < min_period_size) @@ -739,7 +745,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, max_period_size = snd_pcm_plug_client_size(substream, snd_pcm_hw_param_value_max(slave_params, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, NULL)); - if (max_period_size) { + if (max_period_size > 0) { max_period_size *= oss_frame_size; max_period_size = rounddown_pow_of_two(max_period_size); if (oss_period_size > max_period_size) @@ -752,7 +758,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, oss_periods = substream->oss.setup.periods; s = snd_pcm_hw_param_value_max(slave_params, SNDRV_PCM_HW_PARAM_PERIODS, NULL); - if (runtime->oss.maxfrags && s > runtime->oss.maxfrags) + if (s > 0 && runtime->oss.maxfrags && s > runtime->oss.maxfrags) s = runtime->oss.maxfrags; if (oss_periods > s) oss_periods = s; -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -next] ALSA: Fix oversized kvmalloc() calls 2021-11-30 14:05 ` Takashi Iwai @ 2021-12-01 6:16 ` Bixuan Cui 2021-12-01 7:23 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Bixuan Cui @ 2021-12-01 6:16 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, tiwai, inux-kernel 在 2021/11/30 下午10:05, Takashi Iwai 写道: > On Tue, 30 Nov 2021 12:39:27 +0100, > Takashi Iwai wrote: >> On Tue, 30 Nov 2021 12:16:18 +0100, >> Bixuan Cui wrote: >>> The commit 7661809d493b ("mm: don't allow oversized kvmalloc() >>> calls") limits the max allocatable memory via kvzalloc() to MAX_INT. >>> >>> Reported-by:syzbot+bb348e9f9a954d42746f@syzkaller.appspotmail.com >>> Signed-off-by: Bixuan Cui<cuibixuan@linux.alibaba.com> >> We should check the allocation size a lot earlier than here. >> IOW, such a big size shouldn't have been passed to this function but >> it should have been handled as an error in the caller side >> (snd_pcm_oss_change_params*()). >> >> Could you give the reproducer? > I'm asking it because the patch like below might cover the case. > > > Takashi > > -- 8< -- > From: Takashi Iwai<tiwai@suse.de> > Subject: [PATCH] ALSA: pcm: oss: Fix negative period/buffer sizes > > The period size calculation in OSS layer may receive a negative value > as an error, but the code there assumes only the positive values and > handle them with size_t. Due to that, a too big value may be passed > to the lower layers. > > This patch changes the code to handle with ssize_t and adds the proper > error checks appropriately. > > Signed-off-by: Takashi Iwai<tiwai@suse.de> > --- > sound/core/oss/pcm_oss.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c > index 82a818734a5f..bec7590bc84b 100644 > --- a/sound/core/oss/pcm_oss.c > +++ b/sound/core/oss/pcm_oss.c > @@ -147,7 +147,7 @@ snd_pcm_hw_param_value_min(const struct snd_pcm_hw_params *params, > * > * Return the maximum value for field PAR. > */ > -static unsigned int > +static int > snd_pcm_hw_param_value_max(const struct snd_pcm_hw_params *params, > snd_pcm_hw_param_t var, int *dir) > { > @@ -682,18 +682,24 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *oss_params, > struct snd_pcm_hw_params *slave_params) > { > - size_t s; > - size_t oss_buffer_size, oss_period_size, oss_periods; > - size_t min_period_size, max_period_size; > + ssize_t s; > + ssize_t oss_buffer_size; > + ssize_t oss_period_size, oss_periods; > + ssize_t min_period_size, max_period_size; > struct snd_pcm_runtime *runtime = substream->runtime; > size_t oss_frame_size; > > oss_frame_size = snd_pcm_format_physical_width(params_format(oss_params)) * > params_channels(oss_params) / 8; > > + oss_buffer_size = snd_pcm_hw_param_value_max(slave_params, > + SNDRV_PCM_HW_PARAM_BUFFER_SIZE, > + NULL); > + if (oss_buffer_size <= 0) > + return -EINVAL; > oss_buffer_size = snd_pcm_plug_client_size(substream, > - snd_pcm_hw_param_value_max(slave_params, SNDRV_PCM_HW_PARAM_BUFFER_SIZE, NULL)) * oss_frame_size; > - if (!oss_buffer_size) > + oss_buffer_size * oss_frame_size); > + if (oss_buffer_size <= 0) > return -EINVAL; > oss_buffer_size = rounddown_pow_of_two(oss_buffer_size); > if (atomic_read(&substream->mmap_count)) { > @@ -730,7 +736,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, > > min_period_size = snd_pcm_plug_client_size(substream, > snd_pcm_hw_param_value_min(slave_params, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, NULL)); > - if (min_period_size) { > + if (min_period_size > 0) { > min_period_size *= oss_frame_size; > min_period_size = roundup_pow_of_two(min_period_size); > if (oss_period_size < min_period_size) > @@ -739,7 +745,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, > > max_period_size = snd_pcm_plug_client_size(substream, > snd_pcm_hw_param_value_max(slave_params, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, NULL)); > - if (max_period_size) { > + if (max_period_size > 0) { > max_period_size *= oss_frame_size; > max_period_size = rounddown_pow_of_two(max_period_size); > if (oss_period_size > max_period_size) > @@ -752,7 +758,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, > oss_periods = substream->oss.setup.periods; > > s = snd_pcm_hw_param_value_max(slave_params, SNDRV_PCM_HW_PARAM_PERIODS, NULL); > - if (runtime->oss.maxfrags && s > runtime->oss.maxfrags) > + if (s > 0 && runtime->oss.maxfrags && s > runtime->oss.maxfrags) > s = runtime->oss.maxfrags; > if (oss_periods > s) > oss_periods = s; Hi, I got the bug report from syzbot: https://syzkaller.appspot.com/bug?id=c224c2af9ed367315fc048b50f008385bd5c4c3f . I checked the call stack that reported the error, and then tried to construct a case, but it relied on some hardware devices. My machine did not have it, so I couldn't construct it. :-( I reviewed the code again and found that 'format->channels' in 'size = frames * format->channels * width' should come from file->private_data in snd_pcm_oss_ioctl(). And file->private_data is initialized in snd_pcm_oss_open_file(). Maybe this patch cannot cover this problem. But I think we can wait for this patch to be applied whether the problem occurs. Thanks Bixuan Cui ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] ALSA: Fix oversized kvmalloc() calls 2021-12-01 6:16 ` Bixuan Cui @ 2021-12-01 7:23 ` Takashi Iwai 2021-12-01 8:19 ` Bixuan Cui 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2021-12-01 7:23 UTC (permalink / raw) To: Bixuan Cui; +Cc: alsa-devel, tiwai, inux-kernel On Wed, 01 Dec 2021 07:16:12 +0100, Bixuan Cui wrote: > > 在 2021/11/30 下午10:05, Takashi Iwai 写道: > > On Tue, 30 Nov 2021 12:39:27 +0100, > Takashi Iwai wrote: > > On Tue, 30 Nov 2021 12:16:18 +0100, > Bixuan Cui wrote: > > The commit 7661809d493b ("mm: don't allow oversized kvmalloc() > calls") limits the max allocatable memory via kvzalloc() to MAX_INT. > > Reported-by: syzbot+bb348e9f9a954d42746f@syzkaller.appspotmail.com > Signed-off-by: Bixuan Cui <cuibixuan@linux.alibaba.com> > > We should check the allocation size a lot earlier than here. > IOW, such a big size shouldn't have been passed to this function but > it should have been handled as an error in the caller side > (snd_pcm_oss_change_params*()). > > Could you give the reproducer? > > I'm asking it because the patch like below might cover the case. > > Takashi > > -- 8< -- > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH] ALSA: pcm: oss: Fix negative period/buffer sizes > > The period size calculation in OSS layer may receive a negative value > as an error, but the code there assumes only the positive values and > handle them with size_t. Due to that, a too big value may be passed > to the lower layers. > > This patch changes the code to handle with ssize_t and adds the proper > error checks appropriately. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/core/oss/pcm_oss.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c > index 82a818734a5f..bec7590bc84b 100644 > --- a/sound/core/oss/pcm_oss.c > +++ b/sound/core/oss/pcm_oss.c > @@ -147,7 +147,7 @@ snd_pcm_hw_param_value_min(const struct snd_pcm_hw_params *params, > * > * Return the maximum value for field PAR. > */ > -static unsigned int > +static int > snd_pcm_hw_param_value_max(const struct snd_pcm_hw_params *params, > snd_pcm_hw_param_t var, int *dir) > { > @@ -682,18 +682,24 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *oss_params, > struct snd_pcm_hw_params *slave_params) > { > - size_t s; > - size_t oss_buffer_size, oss_period_size, oss_periods; > - size_t min_period_size, max_period_size; > + ssize_t s; > + ssize_t oss_buffer_size; > + ssize_t oss_period_size, oss_periods; > + ssize_t min_period_size, max_period_size; > struct snd_pcm_runtime *runtime = substream->runtime; > size_t oss_frame_size; > > oss_frame_size = snd_pcm_format_physical_width(params_format(oss_params)) * > params_channels(oss_params) / 8; > > + oss_buffer_size = snd_pcm_hw_param_value_max(slave_params, > + SNDRV_PCM_HW_PARAM_BUFFER_SIZE, > + NULL); > + if (oss_buffer_size <= 0) > + return -EINVAL; > oss_buffer_size = snd_pcm_plug_client_size(substream, > - snd_pcm_hw_param_value_max(slave_params, SNDRV_PCM_HW_PARAM_BUFFER_SIZE, NULL)) * oss_frame_size; > - if (!oss_buffer_size) > + oss_buffer_size * oss_frame_size); > + if (oss_buffer_size <= 0) > return -EINVAL; > oss_buffer_size = rounddown_pow_of_two(oss_buffer_size); > if (atomic_read(&substream->mmap_count)) { > @@ -730,7 +736,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, > > min_period_size = snd_pcm_plug_client_size(substream, > snd_pcm_hw_param_value_min(slave_params, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, NULL)); > - if (min_period_size) { > + if (min_period_size > 0) { > min_period_size *= oss_frame_size; > min_period_size = roundup_pow_of_two(min_period_size); > if (oss_period_size < min_period_size) > @@ -739,7 +745,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, > > max_period_size = snd_pcm_plug_client_size(substream, > snd_pcm_hw_param_value_max(slave_params, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, NULL)); > - if (max_period_size) { > + if (max_period_size > 0) { > max_period_size *= oss_frame_size; > max_period_size = rounddown_pow_of_two(max_period_size); > if (oss_period_size > max_period_size) > @@ -752,7 +758,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, > oss_periods = substream->oss.setup.periods; > > s = snd_pcm_hw_param_value_max(slave_params, SNDRV_PCM_HW_PARAM_PERIODS, NULL); > - if (runtime->oss.maxfrags && s > runtime->oss.maxfrags) > + if (s > 0 && runtime->oss.maxfrags && s > runtime->oss.maxfrags) > s = runtime->oss.maxfrags; > if (oss_periods > s) > oss_periods = s; > > Hi, > > I got the bug report from syzbot: > https://syzkaller.appspot.com/bug?id=c224c2af9ed367315fc048b50f008385bd5c4c3f > . Thanks! > I checked the call stack that reported the error, and then tried to construct > a case, but it relied on some hardware > > devices. My machine did not have it, so I couldn't construct it. :-( > > I reviewed the code again and found that 'format->channels' in 'size = frames > * format->channels * width' > > should come from file->private_data in snd_pcm_oss_ioctl(). And file-> > private_data is initialized in snd_pcm_oss_open_file(). > > Maybe this patch cannot cover this problem. The code path is about the ioctl changing the PCM channels, and it's calling the common parameter update function. So it goes through the point the patch touches, at least. It's not clear, though, whether the overflow is due to the lack of the error checks or the driver still allowed a larger value. Unfortunately, there seems no reproducer. > But I think we can wait for this patch to be applied whether the problem > occurs. OK, I wrote a couple of more fixes for covering such cases in OSS layer. Will submit a series later. thanks, Takashi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] ALSA: Fix oversized kvmalloc() calls 2021-12-01 7:23 ` Takashi Iwai @ 2021-12-01 8:19 ` Bixuan Cui 0 siblings, 0 replies; 6+ messages in thread From: Bixuan Cui @ 2021-12-01 8:19 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, tiwai, inux-kernel 在 2021/12/1 下午3:23, Takashi Iwai 写道: >> But I think we can wait for this patch to be applied whether the problem >> occurs. > OK, I wrote a couple of more fixes for covering such cases in OSS > layer. Will submit a series later. Ok. Thanks, Bixuan Cui ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-01 8:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-30 11:16 [PATCH -next] ALSA: Fix oversized kvmalloc() calls Bixuan Cui 2021-11-30 11:39 ` Takashi Iwai 2021-11-30 14:05 ` Takashi Iwai 2021-12-01 6:16 ` Bixuan Cui 2021-12-01 7:23 ` Takashi Iwai 2021-12-01 8:19 ` Bixuan Cui
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.