Re: [PATCH 1/2] ALSA: control: add dimension validator for userspace element

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

On May 29 2015 18:35, Takashi Iwai wrote:
> At Fri, 29 May 2015 17:58:56 +0900,
> Takashi Sakamoto wrote:
>>
>> The 'dimen' field in struct snd_ctl_elem_info is used to compose
>> all of channels in the control element as multi-dimensional matrix.
>> The field has four members. Each member represents the number in
>> each dimension level. For example, if the channels consist of typical
>> two dimensional matrix, the dimen[0] represents the number of rows
>> and dimen[1] represents the number of columns, or vise-versa.
>>
>> The total elements in the matrix should be within the number of
>> channels in the control element, while current implementation has
>> no validator of this information. In a view of userspace applications,
>> the information must be valid so that it cannot cause any bugs such as
>> buffer-over-run.
>>
>> This commit adds a validator of dimen information for userspace
>> applications which add new control elements. When they add the elements
>> with wrong dimen information, they receive EINVAL.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>  sound/core/control.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index 196a6fe..9b77afd 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -805,6 +805,25 @@ static int snd_ctl_elem_list(struct snd_card *card,
>>  	return 0;
>>  }
>>  
>> +static bool validate_dimen(struct snd_ctl_elem_info *info)
>> +{
>> +	unsigned int elements;
>> +	unsigned int i;
>> +
>> +	/*
>> +	 * When drivers don't use dimen field, this value is zero and pass the
>> +	 * validation. Else, calculated number of elements is validated.
>> +	 */
>> +	elements = info->dimen.d[0];
>> +	for (i = 1; i < ARRAY_SIZE(info->dimen.d); ++i) {
>> +		if (info->dimen.d[i] == 0)
>> +			break;
> 
> It'd be better to have a value sanity check, also for avoiding an
> overflow.  A simple one like below should suffice:
> 
> 	if (info->dimens.d[i] > info->count)
> 		return false;

Indeed, thanks.

>> +		elements *= info->dimen.d[i];
>> +	}
>> +
>> +	return elements <= info->count;
>> +}
>> +
>>  static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
>>  			     struct snd_ctl_elem_info *info)
>>  {
>> @@ -1272,6 +1291,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>>  	if (info->count < 1 ||
>>  	    info->count > max_value_counts[info->type])
>>  		return -EINVAL;
>> +	if (!validate_dimen(info))
>> +		return -EINVAL;
>>  	private_size = value_sizes[info->type] * info->count;
>>  
>>  	/*
>> -- 
>> 2.1.4

Takashi Sakamoto