Commit 92ee3c60 authored by Takashi Iwai's avatar Takashi Iwai
Browse files

ALSA: pcm: Fix races among concurrent hw_params and hw_free calls 



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: default avatarHu Jiahui <kirin.say@gmail.com>
Cc: <stable@vger.kernel.org>
Reviewed-by: default avatarJaroslav Kysela <perex@perex.cz>
Link: https://lore.kernel.org/r/20220322170720.3529-2-tiwai@suse.de


Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent 646b907e

include/sound/pcm.h

+1 −0
Original line number Original line Diff line number Diff line
@@ -401,6 +401,7 @@ struct snd_pcm_runtime { 
	wait_queue_head_t tsleep;	/* transfer sleep */

	wait_queue_head_t tsleep;	/* transfer sleep */

	struct fasync_struct *fasync;

	struct fasync_struct *fasync;

	bool stop_operating;		/* sync_stop will be called */

	bool stop_operating;		/* sync_stop will be called */

	struct mutex buffer_mutex;	/* protect for buffer changes */





	/* -- private section -- */

	/* -- private section -- */

	void *private_data;

	void *private_data;

sound/core/pcm.c

+2 −0
Original line number Original line Diff line number Diff line
@@ -969,6 +969,7 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, 
	init_waitqueue_head(&runtime->tsleep);

	init_waitqueue_head(&runtime->tsleep);





	runtime->status->state = SNDRV_PCM_STATE_OPEN;

	runtime->status->state = SNDRV_PCM_STATE_OPEN;

	mutex_init(&runtime->buffer_mutex);





	substream->runtime = runtime;

	substream->runtime = runtime;

	substream->private_data = pcm->private_data;

	substream->private_data = pcm->private_data;
@@ -1002,6 +1003,7 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream) 
	} else {

	} else {

		substream->runtime = NULL;

		substream->runtime = NULL;

	}

	}

	mutex_destroy(&runtime->buffer_mutex);

	kfree(runtime);

	kfree(runtime);

	put_pid(substream->pid);

	put_pid(substream->pid);

	substream->pid = NULL;

	substream->pid = NULL;

sound/core/pcm_native.c

+39 −22
Original line number Original line Diff line number Diff line
@@ -685,33 +685,40 @@ static int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm, 
	return 0;

	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,

static int snd_pcm_hw_params(struct snd_pcm_substream *substream,

			     struct snd_pcm_hw_params *params)

			     struct snd_pcm_hw_params *params)

{

{

	struct snd_pcm_runtime *runtime;

	struct snd_pcm_runtime *runtime;

	int err, usecs;

	int err = 0, usecs;

	unsigned int bits;

	unsigned int bits;

	snd_pcm_uframes_t frames;

	snd_pcm_uframes_t frames;





	if (PCM_RUNTIME_CHECK(substream))

	if (PCM_RUNTIME_CHECK(substream))

		return -ENXIO;

		return -ENXIO;

	runtime = substream->runtime;

	runtime = substream->runtime;

	mutex_lock(&runtime->buffer_mutex);

	snd_pcm_stream_lock_irq(substream);

	snd_pcm_stream_lock_irq(substream);

	switch (runtime->status->state) {

	switch (runtime->status->state) {

	case SNDRV_PCM_STATE_OPEN:

	case SNDRV_PCM_STATE_OPEN:

	case SNDRV_PCM_STATE_SETUP:

	case SNDRV_PCM_STATE_SETUP:

	case SNDRV_PCM_STATE_PREPARED:

	case SNDRV_PCM_STATE_PREPARED:

		if (!is_oss_stream(substream) &&

		    atomic_read(&substream->mmap_count))

			err = -EBADFD;

		break;

		break;

	default:

	default:

		snd_pcm_stream_unlock_irq(substream);

		err = -EBADFD;

		return -EBADFD;

		break;

	}

	}

	snd_pcm_stream_unlock_irq(substream);

	snd_pcm_stream_unlock_irq(substream);

#if IS_ENABLED(CONFIG_SND_PCM_OSS)

	if (err)

	if (!substream->oss.oss)

		goto unlock;

#endif

		if (atomic_read(&substream->mmap_count))

			return -EBADFD;





	snd_pcm_sync_stop(substream, true);

	snd_pcm_sync_stop(substream, true);
@@ -799,16 +806,21 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, 
	if (usecs >= 0)

	if (usecs >= 0)

		cpu_latency_qos_add_request(&substream->latency_pm_qos_req,

		cpu_latency_qos_add_request(&substream->latency_pm_qos_req,

					    usecs);

					    usecs);

	return 0;

	err = 0;

 _error:

 _error:

	if (err) {

		/* hardware might be unusable from this time,

		/* hardware might be unusable from this time,

	   so we force application to retry to set

		 * so we force application to retry to set

	   the correct hardware parameter settings */

		 * the correct hardware parameter settings

		 */

		snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN);

		snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN);

		if (substream->ops->hw_free != NULL)

		if (substream->ops->hw_free != NULL)

			substream->ops->hw_free(substream);

			substream->ops->hw_free(substream);

		if (substream->managed_buffer_alloc)

		if (substream->managed_buffer_alloc)

			snd_pcm_lib_free_pages(substream);

			snd_pcm_lib_free_pages(substream);

	}

 unlock:

	mutex_unlock(&runtime->buffer_mutex);

	return err;

	return err;

}

}
@@ -848,26 +860,31 @@ static int do_hw_free(struct snd_pcm_substream *substream) 
static int snd_pcm_hw_free(struct snd_pcm_substream *substream)

static int snd_pcm_hw_free(struct snd_pcm_substream *substream)

{

{

	struct snd_pcm_runtime *runtime;

	struct snd_pcm_runtime *runtime;

	int result;

	int result = 0;





	if (PCM_RUNTIME_CHECK(substream))

	if (PCM_RUNTIME_CHECK(substream))

		return -ENXIO;

		return -ENXIO;

	runtime = substream->runtime;

	runtime = substream->runtime;

	mutex_lock(&runtime->buffer_mutex);

	snd_pcm_stream_lock_irq(substream);

	snd_pcm_stream_lock_irq(substream);

	switch (runtime->status->state) {

	switch (runtime->status->state) {

	case SNDRV_PCM_STATE_SETUP:

	case SNDRV_PCM_STATE_SETUP:

	case SNDRV_PCM_STATE_PREPARED:

	case SNDRV_PCM_STATE_PREPARED:

		if (atomic_read(&substream->mmap_count))

			result = -EBADFD;

		break;

		break;

	default:

	default:

		snd_pcm_stream_unlock_irq(substream);

		result = -EBADFD;

		return -EBADFD;

		break;

	}

	}

	snd_pcm_stream_unlock_irq(substream);

	snd_pcm_stream_unlock_irq(substream);

	if (atomic_read(&substream->mmap_count))

	if (result)

		return -EBADFD;

		goto unlock;

	result = do_hw_free(substream);

	result = do_hw_free(substream);

	snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN);

	snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN);

	cpu_latency_qos_remove_request(&substream->latency_pm_qos_req);

	cpu_latency_qos_remove_request(&substream->latency_pm_qos_req);

 unlock:

	mutex_unlock(&runtime->buffer_mutex);

	return result;

	return result;

}

}