Skip to content

Commit ca0214e

Browse files
committed
ALSA: pcm: Fix possible OOB access in PCM oss plugins
The PCM OSS emulation converts and transfers the data on the fly via "plugins". The data is converted over the dynamically allocated buffer for each plugin, and recently syzkaller caught OOB in this flow. Although the bisection by syzbot pointed out to the commit 65766ee ("ALSA: oss: Use kvzalloc() for local buffer allocations"), this is merely a commit to replace vmalloc() with kvmalloc(), hence it can't be the cause. The further debug action revealed that this happens in the case where a slave PCM doesn't support only the stereo channels while the OSS stream is set up for a mono channel. Below is a brief explanation: At each OSS parameter change, the driver sets up the PCM hw_params again in snd_pcm_oss_change_params_lock(). This is also the place where plugins are created and local buffers are allocated. The problem is that the plugins are created before the final hw_params is determined. Namely, two snd_pcm_hw_param_near() calls for setting the period size and periods may influence on the final result of channels, rates, etc, too, while the current code has already created plugins beforehand with the premature values. So, the plugin believes that channels=1, while the actual I/O is with channels=2, which makes the driver reading/writing over the allocated buffer size. The fix is simply to move the plugin allocation code after the final hw_params call. Reported-by: syzbot+d4503ae45b65c5bc1194@syzkaller.appspotmail.com Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de>
1 parent 6ac371a commit ca0214e

File tree

1 file changed

+22
-21
lines changed

1 file changed

+22
-21
lines changed

sound/core/oss/pcm_oss.c

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -940,6 +940,28 @@ static int snd_pcm_oss_change_params_locked(struct snd_pcm_substream *substream)
940940
oss_frame_size = snd_pcm_format_physical_width(params_format(params)) *
941941
params_channels(params) / 8;
942942

943+
err = snd_pcm_oss_period_size(substream, params, sparams);
944+
if (err < 0)
945+
goto failure;
946+
947+
n = snd_pcm_plug_slave_size(substream, runtime->oss.period_bytes / oss_frame_size);
948+
err = snd_pcm_hw_param_near(substream, sparams, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, n, NULL);
949+
if (err < 0)
950+
goto failure;
951+
952+
err = snd_pcm_hw_param_near(substream, sparams, SNDRV_PCM_HW_PARAM_PERIODS,
953+
runtime->oss.periods, NULL);
954+
if (err < 0)
955+
goto failure;
956+
957+
snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, NULL);
958+
959+
err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_HW_PARAMS, sparams);
960+
if (err < 0) {
961+
pcm_dbg(substream->pcm, "HW_PARAMS failed: %i\n", err);
962+
goto failure;
963+
}
964+
943965
#ifdef CONFIG_SND_PCM_OSS_PLUGINS
944966
snd_pcm_oss_plugin_clear(substream);
945967
if (!direct) {
@@ -974,27 +996,6 @@ static int snd_pcm_oss_change_params_locked(struct snd_pcm_substream *substream)
974996
}
975997
#endif
976998

977-
err = snd_pcm_oss_period_size(substream, params, sparams);
978-
if (err < 0)
979-
goto failure;
980-
981-
n = snd_pcm_plug_slave_size(substream, runtime->oss.period_bytes / oss_frame_size);
982-
err = snd_pcm_hw_param_near(substream, sparams, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, n, NULL);
983-
if (err < 0)
984-
goto failure;
985-
986-
err = snd_pcm_hw_param_near(substream, sparams, SNDRV_PCM_HW_PARAM_PERIODS,
987-
runtime->oss.periods, NULL);
988-
if (err < 0)
989-
goto failure;
990-
991-
snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, NULL);
992-
993-
if ((err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_HW_PARAMS, sparams)) < 0) {
994-
pcm_dbg(substream->pcm, "HW_PARAMS failed: %i\n", err);
995-
goto failure;
996-
}
997-
998999
if (runtime->oss.trigger) {
9991000
sw_params->start_threshold = 1;
10001001
} else {

0 commit comments

Comments
 (0)