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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Bixuan Cui <cuibixuan@xxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxx>
> 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@xxxxxxx>
> ---
> 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
