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

[Takashi Sakamoto]

On 2015年04月12日 00:42, Takashi Iwai wrote:
> 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 at 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.

OK. I forgot the case that the value of kctl->index starts 1 or larger...


Thanks

Takashi Sakamoto

> 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
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>