On Fri, May 13, 2022 at 11:00:14AM +0300, Ovidiu Panait wrote: > From: Takashi Iwai <tiwai@xxxxxxx> > > commit 92ee3c60ec9fe64404dc035e7c41277d74aa26cb upstream. > > Currently we have neither proper check nor protection against the > concurrent calls of PCM hw_params and hw_free ioctls, which may result > in a UAF. Since the existing PCM stream lock can't be used for > protecting the whole ioctl operations, we need a new mutex to protect > those racy calls. > > This patch introduced a new mutex, runtime->buffer_mutex, and applies > it to both hw_params and hw_free ioctl code paths. Along with it, the > both functions are slightly modified (the mmap_count check is moved > into the state-check block) for code simplicity. > > Reported-by: Hu Jiahui <kirin.say@xxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> > Reviewed-by: Jaroslav Kysela <perex@xxxxxxxx> > Link: https://lore.kernel.org/r/20220322170720.3529-2-tiwai@xxxxxxx > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > [OP: backport to 4.19: adjusted context] > Signed-off-by: Ovidiu Panait <ovidiu.panait@xxxxxxxxxxxxx> > --- > include/sound/pcm.h | 1 + > sound/core/pcm.c | 2 ++ > sound/core/pcm_native.c | 55 +++++++++++++++++++++++++++-------------- > 3 files changed, 39 insertions(+), 19 deletions(-) > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h > index d6bd3caf6878..0871e16cd04b 100644 > --- a/include/sound/pcm.h > +++ b/include/sound/pcm.h > @@ -404,6 +404,7 @@ struct snd_pcm_runtime { > wait_queue_head_t sleep; /* poll sleep */ > wait_queue_head_t tsleep; /* transfer sleep */ > struct fasync_struct *fasync; > + struct mutex buffer_mutex; /* protect for buffer changes */ > > /* -- private section -- */ > void *private_data; > diff --git a/sound/core/pcm.c b/sound/core/pcm.c > index b6ed38dec435..a11365dc5349 100644 > --- a/sound/core/pcm.c > +++ b/sound/core/pcm.c > @@ -1031,6 +1031,7 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, > init_waitqueue_head(&runtime->tsleep); > > runtime->status->state = SNDRV_PCM_STATE_OPEN; > + mutex_init(&runtime->buffer_mutex); > > substream->runtime = runtime; > substream->private_data = pcm->private_data; > @@ -1062,6 +1063,7 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream) > substream->runtime = NULL; > if (substream->timer) > spin_unlock_irq(&substream->timer->lock); > + mutex_destroy(&runtime->buffer_mutex); > kfree(runtime); > put_pid(substream->pid); > substream->pid = NULL; > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index c92eca627840..2b46a2ebefe0 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -666,33 +666,40 @@ static int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm, > return 0; > } > > +#if IS_ENABLED(CONFIG_SND_PCM_OSS) > +#define is_oss_stream(substream) ((substream)->oss.oss) > +#else > +#define is_oss_stream(substream) false > +#endif > + > static int snd_pcm_hw_params(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *params) > { > struct snd_pcm_runtime *runtime; > - int err, usecs; > + int err = 0, usecs; > unsigned int bits; > snd_pcm_uframes_t frames; > > if (PCM_RUNTIME_CHECK(substream)) > return -ENXIO; > runtime = substream->runtime; > + mutex_lock(&runtime->buffer_mutex); > snd_pcm_stream_lock_irq(substream); > switch (runtime->status->state) { > case SNDRV_PCM_STATE_OPEN: > case SNDRV_PCM_STATE_SETUP: > case SNDRV_PCM_STATE_PREPARED: > + if (!is_oss_stream(substream) && > + atomic_read(&substream->mmap_count)) > + err = -EBADFD; > break; > default: > - snd_pcm_stream_unlock_irq(substream); > - return -EBADFD; > + err = -EBADFD; > + break; > } > snd_pcm_stream_unlock_irq(substream); > -#if IS_ENABLED(CONFIG_SND_PCM_OSS) > - if (!substream->oss.oss) > -#endif > - if (atomic_read(&substream->mmap_count)) > - return -EBADFD; > + if (err) > + goto unlock; > > params->rmask = ~0U; > err = snd_pcm_hw_refine(substream, params); > @@ -769,14 +776,19 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, > if ((usecs = period_to_usecs(runtime)) >= 0) > pm_qos_add_request(&substream->latency_pm_qos_req, > PM_QOS_CPU_DMA_LATENCY, usecs); > - return 0; > + err = 0; > _error: > - /* hardware might be unusable from this time, > - so we force application to retry to set > - the correct hardware parameter settings */ > - snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); > - if (substream->ops->hw_free != NULL) > - substream->ops->hw_free(substream); > + if (err) { > + /* hardware might be unusable from this time, > + * so we force application to retry to set > + * the correct hardware parameter settings > + */ > + snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); > + if (substream->ops->hw_free != NULL) > + substream->ops->hw_free(substream); > + } > + unlock: > + mutex_unlock(&runtime->buffer_mutex); > return err; > } > > @@ -809,22 +821,27 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) > if (PCM_RUNTIME_CHECK(substream)) > return -ENXIO; > runtime = substream->runtime; > + mutex_lock(&runtime->buffer_mutex); > snd_pcm_stream_lock_irq(substream); > switch (runtime->status->state) { > case SNDRV_PCM_STATE_SETUP: > case SNDRV_PCM_STATE_PREPARED: > + if (atomic_read(&substream->mmap_count)) > + result = -EBADFD; > break; > default: > - snd_pcm_stream_unlock_irq(substream); > - return -EBADFD; > + result = -EBADFD; > + break; > } > snd_pcm_stream_unlock_irq(substream); > - if (atomic_read(&substream->mmap_count)) > - return -EBADFD; > + if (result) > + goto unlock; > if (substream->ops->hw_free) > result = substream->ops->hw_free(substream); > snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); > pm_qos_remove_request(&substream->latency_pm_qos_req); > + unlock: > + mutex_unlock(&runtime->buffer_mutex); > return result; > } > > -- > 2.36.0 > All now queued up, thanks! greg k-h
