[alsa-devel] [PATCH 2/3] ALSA: control: add dimension validator for userspace element

[Takashi Sakamoto]

Sorry to be late. I had a short vacation.

On Jul 2 2016 16:56, Takashi Iwai wrote:
> On Fri, 01 Jul 2016 14:29:37 +0200,
> Takashi Sakamoto wrote:
>>
>> On Jul 1 2016 20:10, Takashi Sakamoto wrote:
>>> The 'dimen' field in struct snd_ctl_elem_info is used to compose all of
>>> members in the element as multi-dimensional matrix. The field has four
>>> members. Each member represents the width in each dimension level by
>>> element member unit. For example, if the members 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 members in the matrix should be within the number of members in
>>> the 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 dimension information for userspace
>>> applications which add new element sets. When they add the element sets
>>> with wrong dimension information, they receive -EINVAL.
>>>
>>> Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
>>> ---
>>>  sound/core/control.c | 30 ++++++++++++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>
>>> diff --git a/sound/core/control.c b/sound/core/control.c
>>> index a85d455..54da910 100644
>>> --- a/sound/core/control.c
>>> +++ b/sound/core/control.c
>>> @@ -805,6 +805,34 @@ static int snd_ctl_elem_list(struct snd_card *card,
>>>  	return 0;
>>>  }
>>>  
>>> +static bool validate_element_member_dimension(struct snd_ctl_elem_info *info)
>>> +{
>>> +	unsigned int members;
>>> +	unsigned int i = 0;
> 
> Unnecessary initialization.
> 
>>> +
>>> +	if (info->dimen.d[0] == 0)
>>> +		return true;
>>> +
>>> +	members = 1;
>>> +	for (i = 0; i < ARRAY_SIZE(info->dimen.d); ++i) {
>>> +		if (info->dimen.d[i] < 0)
>>> +			return false;
>>
>> Mmm... info->dimen.d is declared with 'unsigned short' type. Thus,
>> negative value check is needless...
>>
>> struct snd_ctl_elem_info {
>>     ...
>>     union {
>>         unsigned short d[4];
>>         ..
>>     };
>>     ...
>> };
>>
>> Please abandon this patchset. I'll post new one tomorrow.
> 
> Indeed, somehow I overlooked it, too.
> 
> While we're at it: maybe it's even safer to check more strictly the
> result whether it matches with info->count.  I don't think there is
> any reason to have an unaligned info->count value when the dimen array
> is given.  It must be a bug.
> 
> That is:
>>> +		if (info->dimen.d[i] == 0)
>>> +			break;
>>> +
>>> +		members *= info->dimen.d[i];
>>> +		if (members > info->count)
>>> +			return false;
>>> +	}
>>> +
>>> +	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
>>> +		if (info->dimen.d[i] != 0)
>>> +			return false;
>>> +	}
>>> +
>>> +	return true;
> 
> Here will be
> 	return members == info->count;

I don't mind to be strict here. It's the same as my initial idea (not
posted).


But here, I have a question related to next patch (3rd patch). The
validation logic depends on reliability of info->count. In a case of
user-defined control element set, info->count is validated in advance,
therefore it's reasonable. But in a case of each kernel driver,
info->count is not validated yet, here. Thus, reliability of the
calculation is lost, I think. The result depends on implementation of
each driver and it can bring disadvantages to userspace.

When I think back, this is the main reason that I'm unwilling to use
info->count for prevention of arithmetic overflow. If we apply the same
logic to each kernel drivers, we need more validators for info->count.
For example:
'''
static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
                             struct snd_ctl_elem_info *info)
{
    ...
    if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
        info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64) {
        dev_err(card->dev,
            "This module has a bug of invalid element type.\n");
        result = -ENODATA;
        goto end;
    }

    if (info->count < 1 ||
        info->count > max_element_members[info->type]) {
        dev_err(card->dev,
            "This module has a bug of invalid member count.\n");
        result = -ENODATA;
        goto end;
    }

    /* This is a driver bug. */
    if (!validate_element_member_dimension(info)) {
        dev_err(card->dev,
            "This module has a bug of invalid dimention info.\n");
        result = -ENODATA;
        goto end;
    }
    ...
'''

Assume 'max_element_members' is precomputed table about the maximum
number of members in an element, like:
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n1209

What do you think about it?


Regards

Takashi Sakamoto