ACK: [Lucid][CVE-2014-4652] ALSA: control: Protect user controls against concurrent access

[Brad Figg]

On 07/17/2014 08:59 AM, Luis Henriques wrote:
> From: Lars-Peter Clausen <lars at metafoo.de>
> 
> The user-control put and get handlers as well as the tlv do not protect against
> concurrent access from multiple threads. Since the state of the control is not
> updated atomically it is possible that either two write operations or a write
> and a read operation race against each other. Both can lead to arbitrary memory
> disclosure. This patch introduces a new lock that protects user-controls from
> concurrent access. Since applications typically access controls sequentially
> than in parallel a single lock per card should be fine.
> 
> Signed-off-by: Lars-Peter Clausen <lars at metafoo.de>
> Acked-by: Jaroslav Kysela <perex at perex.cz>
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> (cherry picked from commit 07f4d9d74a04aa7c72c5dae0ef97565f28f17b92)
> CVE-2014-4652
> BugLink: http://bugs.launchpad.net/bugs/1339294
> Signed-off-by: Luis Henriques <luis.henriques at canonical.com>
> ---
>  include/sound/core.h |  2 ++
>  sound/core/control.c | 31 +++++++++++++++++++++++++------
>  sound/core/init.c    |  1 +
>  3 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/include/sound/core.h b/include/sound/core.h
> index a61499c22b0b..3ad641cdc735 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -120,6 +120,8 @@ struct snd_card {
>  	int user_ctl_count;		/* count of all user controls */
>  	struct list_head controls;	/* all controls for this card */
>  	struct list_head ctl_files;	/* active control files */
> +	struct mutex user_ctl_lock;	/* protects user controls against
> +					   concurrent access */
>  
>  	struct snd_info_entry *proc_root;	/* root for soundcard specific files */
>  	struct snd_info_entry *proc_id;	/* the card id */
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 7834a5438f75..b5266f7a1a0a 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -873,6 +873,7 @@ static int snd_ctl_elem_unlock(struct snd_ctl_file *file,
>  
>  struct user_element {
>  	struct snd_ctl_elem_info info;
> +	struct snd_card *card;
>  	void *elem_data;		/* element data */
>  	unsigned long elem_data_size;	/* size of element data in bytes */
>  	void *tlv_data;			/* TLV data */
> @@ -895,7 +896,9 @@ static int snd_ctl_elem_user_get(struct snd_kcontrol *kcontrol,
>  {
>  	struct user_element *ue = kcontrol->private_data;
>  
> +	mutex_lock(&ue->card->user_ctl_lock);
>  	memcpy(&ucontrol->value, ue->elem_data, ue->elem_data_size);
> +	mutex_unlock(&ue->card->user_ctl_lock);
>  	return 0;
>  }
>  
> @@ -904,10 +907,12 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
>  {
>  	int change;
>  	struct user_element *ue = kcontrol->private_data;
> -	
> +
> +	mutex_lock(&ue->card->user_ctl_lock);
>  	change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0;
>  	if (change)
>  		memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size);
> +	mutex_unlock(&ue->card->user_ctl_lock);
>  	return change;
>  }
>  
> @@ -927,19 +932,32 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol,
>  		new_data = memdup_user(tlv, size);
>  		if (IS_ERR(new_data))
>  			return PTR_ERR(new_data);
> +		mutex_lock(&ue->card->user_ctl_lock);
>  		change = ue->tlv_data_size != size;
>  		if (!change)
>  			change = memcmp(ue->tlv_data, new_data, size);
>  		kfree(ue->tlv_data);
>  		ue->tlv_data = new_data;
>  		ue->tlv_data_size = size;
> +		mutex_unlock(&ue->card->user_ctl_lock);
>  	} else {
> -		if (! ue->tlv_data_size || ! ue->tlv_data)
> -			return -ENXIO;
> -		if (size < ue->tlv_data_size)
> -			return -ENOSPC;
> +		int ret = 0;
> +
> +		mutex_lock(&ue->card->user_ctl_lock);
> +		if (!ue->tlv_data_size || !ue->tlv_data) {
> +			ret = -ENXIO;
> +			goto err_unlock;
> +		}
> +		if (size < ue->tlv_data_size) {
> +			ret = -ENOSPC;
> +			goto err_unlock;
> +		}
>  		if (copy_to_user(tlv, ue->tlv_data, ue->tlv_data_size))
> -			return -EFAULT;
> +			ret = -EFAULT;
> +err_unlock:
> +		mutex_unlock(&ue->card->user_ctl_lock);
> +		if (ret)
> +			return ret;
>  	}
>  	return change;
>  }
> @@ -1028,6 +1046,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL);
>  	if (ue == NULL)
>  		return -ENOMEM;
> +	ue->card = card;
>  	ue->info = *info;
>  	ue->info.access = 0;
>  	ue->elem_data = (char *)ue + sizeof(*ue);
> diff --git a/sound/core/init.c b/sound/core/init.c
> index 82f350e00cbe..fd8590fefbd9 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -206,6 +206,7 @@ int snd_card_create(int idx, const char *xid,
>  	INIT_LIST_HEAD(&card->devices);
>  	init_rwsem(&card->controls_rwsem);
>  	rwlock_init(&card->ctl_files_rwlock);
> +	mutex_init(&card->user_ctl_lock);
>  	INIT_LIST_HEAD(&card->controls);
>  	INIT_LIST_HEAD(&card->ctl_files);
>  	spin_lock_init(&card->files_lock);
> 


-- 
Brad Figg brad.figg at canonical.com http://www.canonical.com