| Message ID | 1638270978-42412-1-git-send-email-cuibixuan@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next] ALSA: Fix oversized kvmalloc() calls | expand |
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;
在 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
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
在 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
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; }
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(+)