Re: [PATCH 4/4] ctl: fix to handle several elements added by one operation for userspace element

From: Takashi Iwai <tiwai@suse.de>
To: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Cc: alsa-devel@alsa-project.org, clemens@ladisch.de
Subject: Re: [PATCH 4/4] ctl: fix to handle several elements added by one operation for userspace element
Date: Sat, 11 Apr 2015 17:42:58 +0200	[thread overview]
Message-ID: <s5ha8ye4qnx.wl-tiwai@suse.de> (raw)
In-Reply-To: <1428741665-22262-5-git-send-email-o-takashi@sakamocchi.jp>

At Sat, 11 Apr 2015 17:41:05 +0900,
Takashi Sakamoto wrote:
> 
> An element instance can have several elements with the same feature.
> Some userspace applications can add such an element instance by add
> operation with the number of elements. Then, an userspace element
> instance keeps a memory object to keep state of these elements.
> 
> But the element instance has just one memory object for the elements.
> This causes the same result to each read/write operations to the
> different elements.
> 
> This commit fixes this bug by allocating enough memory objects to element
> instance for each of elements.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/control.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index ccb1ca2..23ea738 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1078,9 +1078,12 @@ static int snd_ctl_elem_user_get(struct snd_kcontrol *kcontrol,
>  				 struct snd_ctl_elem_value *ucontrol)
>  {
>  	struct user_element *ue = kcontrol->private_data;
> +	unsigned int size = ue->elem_data_size;
> +	/* The caller filled whole identical information. */
> +	char *target = ue->elem_data + ucontrol->id.index * size;
>  
>  	mutex_lock(&ue->card->user_ctl_lock);
> -	memcpy(&ucontrol->value, ue->elem_data, ue->elem_data_size);
> +	memcpy(&ucontrol->value, target, size);

You must check the validity of the address, i.e. index offset.
This is a security hole.

thanks,

Takashi

>  	mutex_unlock(&ue->card->user_ctl_lock);
>  	return 0;
>  }
> @@ -1090,11 +1093,14 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
>  {
>  	int change;
>  	struct user_element *ue = kcontrol->private_data;
> +	unsigned int size = ue->elem_data_size;
> +	/* The caller filled whole identical information. */
> +	char *target = ue->elem_data + ucontrol->id.index * size;
>  
>  	mutex_lock(&ue->card->user_ctl_lock);
> -	change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0;
> +	change = memcmp(&ucontrol->value, target, size) != 0;
>  	if (change)
> -		memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size);
> +		memcpy(target, &ucontrol->value, size);
>  	mutex_unlock(&ue->card->user_ctl_lock);
>  	return change;
>  }
> @@ -1278,7 +1284,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	if (err < 0)
>  		return err;
>  	memcpy(&kctl->id, &info->id, sizeof(kctl->id));
> -	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size,
> +	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count,
>  				     GFP_KERNEL);
>  	if (kctl->private_data == NULL) {
>  		kfree(kctl);
> -- 
> 2.1.0
> 