| Message ID | 20230921135837.3590897-1-make_ruc2021@163.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] ALSA: pcm: oss: Fix race at SNDCTL_DSP_SETTRIGGER | expand |
On Thu, 21 Sep 2023 15:58:37 +0200, Ma Ke wrote: > > There is a small race window at snd_pcm_oss_set_trigger() that is > called from OSS PCM SNDCTL_DSP_SETTRIGGER ioctl; namely the function > calls snd_pcm_oss_make_ready() at first, then takes the params_lock > mutex for the rest. When the stream is set up again by another thread > between them, it leads to inconsistency, and may result in unexpected > results such as NULL dereference of OSS buffer as a fuzzer spotted > recently. > The fix is simply to cover snd_pcm_oss_make_ready() call into the same > params_lock mutex with snd_pcm_oss_make_ready_locked() variant. Sorry for the late response, as I've been (still) off since the last week. The code change itself looks OK, but unlike the change (with almost same changelog) in commit 8423f0b6d513, this won't hit a serious problem like NULL dereference. The code path merely sets runtime->oss.trigger and start_threshold flags, then issues the ioctl outside the lock. Unless you really hit a problem with a fuzzer, the changelog is misleading and better to be rewritten. thanks, Takashi > > Signed-off-by: Ma Ke <make_ruc2021@163.com> > --- > sound/core/oss/pcm_oss.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c > index 728c211142d1..fd9d23c3684b 100644 > --- a/sound/core/oss/pcm_oss.c > +++ b/sound/core/oss/pcm_oss.c > @@ -2083,21 +2083,16 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr > psubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_PLAYBACK]; > csubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_CAPTURE]; > > - if (psubstream) { > - err = snd_pcm_oss_make_ready(psubstream); > - if (err < 0) > - return err; > - } > - if (csubstream) { > - err = snd_pcm_oss_make_ready(csubstream); > - if (err < 0) > - return err; > - } > if (psubstream) { > runtime = psubstream->runtime; > cmd = 0; > if (mutex_lock_interruptible(&runtime->oss.params_lock)) > return -ERESTARTSYS; > + err = snd_pcm_oss_make_ready_locked(psubstream); > + if (err < 0) { > + mutex_unlock(&runtime->oss.params_lock); > + return err; > + } > if (trigger & PCM_ENABLE_OUTPUT) { > if (runtime->oss.trigger) > goto _skip1; > @@ -2128,6 +2123,11 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr > cmd = 0; > if (mutex_lock_interruptible(&runtime->oss.params_lock)) > return -ERESTARTSYS; > + err = snd_pcm_oss_make_ready_locked(csubstream); > + if (err < 0) { > + mutex_unlock(&runtime->oss.params_lock); > + return err; > + } > if (trigger & PCM_ENABLE_INPUT) { > if (runtime->oss.trigger) > goto _skip2; > -- > 2.37.2 >
diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c index 728c211142d1..fd9d23c3684b 100644 --- a/sound/core/oss/pcm_oss.c +++ b/sound/core/oss/pcm_oss.c @@ -2083,21 +2083,16 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr psubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_PLAYBACK]; csubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_CAPTURE]; - if (psubstream) { - err = snd_pcm_oss_make_ready(psubstream); - if (err < 0) - return err; - } - if (csubstream) { - err = snd_pcm_oss_make_ready(csubstream); - if (err < 0) - return err; - } if (psubstream) { runtime = psubstream->runtime; cmd = 0; if (mutex_lock_interruptible(&runtime->oss.params_lock)) return -ERESTARTSYS; + err = snd_pcm_oss_make_ready_locked(psubstream); + if (err < 0) { + mutex_unlock(&runtime->oss.params_lock); + return err; + } if (trigger & PCM_ENABLE_OUTPUT) { if (runtime->oss.trigger) goto _skip1; @@ -2128,6 +2123,11 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr cmd = 0; if (mutex_lock_interruptible(&runtime->oss.params_lock)) return -ERESTARTSYS; + err = snd_pcm_oss_make_ready_locked(csubstream); + if (err < 0) { + mutex_unlock(&runtime->oss.params_lock); + return err; + } if (trigger & PCM_ENABLE_INPUT) { if (runtime->oss.trigger) goto _skip2;
There is a small race window at snd_pcm_oss_set_trigger() that is called from OSS PCM SNDCTL_DSP_SETTRIGGER ioctl; namely the function calls snd_pcm_oss_make_ready() at first, then takes the params_lock mutex for the rest. When the stream is set up again by another thread between them, it leads to inconsistency, and may result in unexpected results such as NULL dereference of OSS buffer as a fuzzer spotted recently. The fix is simply to cover snd_pcm_oss_make_ready() call into the same params_lock mutex with snd_pcm_oss_make_ready_locked() variant. Signed-off-by: Ma Ke <make_ruc2021@163.com> --- sound/core/oss/pcm_oss.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)