[PATCH 0/3 v2] ALSA: ctl: add dimension information validator

[PATCH 0/3 v2] ALSA: ctl: add dimension information validator

[Takashi Sakamoto]

Hi,

This patchset is revised version of my previous one.

[alsa-devel] [PATCH 0/3] ALSA: add dimension information validator
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-June/109811.html

The main purpose of this patchset is the same, just including some changes
below.
 * Add arithmetic overflow check
 * Add negative value check of first dimension level
 * Change function name of the validator and name of local variable so that
   the aim is clear.

Takashi Sakamoto (3):
  ALSA: echoaudio: purge contradictions between dimension matrix members
    and total number of members
  ALSA: control: add dimension validator for userspace element
  ALSA: control: add dimension validator for kernel driver

 sound/core/control.c            | 87 +++++++++++++++++++++++++++++++++--------
 sound/pci/echoaudio/echoaudio.c |  6 +--
 2 files changed, 74 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH 1/3] ALSA: echoaudio: purge contradictions between dimension matrix members and total number of members

[Takashi Sakamoto]

Currently, sound device drivers for PCI cards produced by Echo Audio
support dimension parameter of element information. But the information
has contradictions to the number of members of each element. I guess that
this comes from the assumption that these sound cards are used only by
'echomixer' in userspace. But ideally, they should be used with usual ALSA
control applications.

This commit removes the contradiction. As a result, 'Monitor Mixer Volume'
and 'VMixer Volume' elements are shown in usual ALSA control applications
such as 'amixer' and 'alsamixer' in series.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/pci/echoaudio/echoaudio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 1cb85ae..3a8e8d5 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -1272,11 +1272,11 @@ static int snd_echo_mixer_info(struct snd_kcontrol *kcontrol,
 
 	chip = snd_kcontrol_chip(kcontrol);
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
-	uinfo->count = 1;
 	uinfo->value.integer.min = ECHOGAIN_MINOUT;
 	uinfo->value.integer.max = ECHOGAIN_MAXOUT;
 	uinfo->dimen.d[0] = num_busses_out(chip);
 	uinfo->dimen.d[1] = num_busses_in(chip);
+	uinfo->count = uinfo->dimen.d[0] * uinfo->dimen.d[1];
 	return 0;
 }
 
@@ -1344,11 +1344,11 @@ static int snd_echo_vmixer_info(struct snd_kcontrol *kcontrol,
 
 	chip = snd_kcontrol_chip(kcontrol);
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
-	uinfo->count = 1;
 	uinfo->value.integer.min = ECHOGAIN_MINOUT;
 	uinfo->value.integer.max = ECHOGAIN_MAXOUT;
 	uinfo->dimen.d[0] = num_busses_out(chip);
 	uinfo->dimen.d[1] = num_pipes_out(chip);
+	uinfo->count = uinfo->dimen.d[0] * uinfo->dimen.d[1];
 	return 0;
 }
 
@@ -1728,7 +1728,6 @@ static int snd_echo_vumeters_info(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_info *uinfo)
 {
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
-	uinfo->count = 96;
 	uinfo->value.integer.min = ECHOGAIN_MINOUT;
 	uinfo->value.integer.max = 0;
 #ifdef ECHOCARD_HAS_VMIXER
@@ -1738,6 +1737,7 @@ static int snd_echo_vumeters_info(struct snd_kcontrol *kcontrol,
 #endif
 	uinfo->dimen.d[1] = 16;	/* 16 channels */
 	uinfo->dimen.d[2] = 2;	/* 0=level, 1=peak */
+	uinfo->count = uinfo->dimen.d[0] * uinfo->dimen.d[1] * uinfo->dimen.d[2];
 	return 0;
 }
 
-- 
2.7.4

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

[Takashi Sakamoto]

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@sakamocchi.jp>
---
 sound/core/control.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index a85d455..eeb080f 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -805,6 +805,43 @@ 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;
+
+	/* The value for each level should be zero or positive. */
+	if (info->dimen.d[0] < 0)
+		return false;
+	members = info->dimen.d[0];
+
+	if (members > 0) {
+		for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
+			if (info->dimen.d[i] < 0)
+				return false;
+			if (info->dimen.d[i] == 0)
+				break;
+
+			/* Prevention of division by zero, for safe. */
+			if (members == 0)
+				return false;
+			/* Prevent arithmetic overflow. */
+			if (info->dimen.d[i] > UINT_MAX / members)
+				return false;
+
+			members *= info->dimen.d[i];
+		}
+	}
+
+	/* The rest of level should be zero. */
+	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
+		if (info->dimen.d[i] != 0)
+			return false;
+	}
+
+	return members <= info->count;
+}
+
 static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
 			     struct snd_ctl_elem_info *info)
 {
@@ -1272,6 +1309,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_element_member_dimension(info))
+		return -EINVAL;
 	private_size = value_sizes[info->type] * info->count;
 
 	/*
-- 
2.7.4

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

[Takashi Iwai]

On Fri, 01 Jul 2016 06:15:10 +0200,
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@sakamocchi.jp>
> ---
>  sound/core/control.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index a85d455..eeb080f 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -805,6 +805,43 @@ 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;
> +
> +	/* The value for each level should be zero or positive. */
> +	if (info->dimen.d[0] < 0)
> +		return false;
> +	members = info->dimen.d[0];
> +
> +	if (members > 0) {
> +		for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
> +			if (info->dimen.d[i] < 0)
> +				return false;
> +			if (info->dimen.d[i] == 0)
> +				break;
> +
> +			/* Prevention of division by zero, for safe. */
> +			if (members == 0)
> +				return false;
> +			/* Prevent arithmetic overflow. */
> +			if (info->dimen.d[i] > UINT_MAX / members)
> +				return false;
> +
> +			members *= info->dimen.d[i];
> +		}
> +	}
> +
> +	/* The rest of level should be zero. */
> +	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
> +		if (info->dimen.d[i] != 0)
> +			return false;
> +	}
> +
> +	return members <= info->count;

Well, since the dimen is 16bit short, it's easier to check the
overflow just by comparing the value with info->count at each time
(supposing info->count being the right value).

That is, something like this:

	/* If no dimension is given, it's OK */
	if (!info->dimen.d[0])
		return true;

	elements = 1;
	for (i = 0; i < ARRAY_SIZE(info->dimen.d); i++) {
		if (info->dimen.d[i] < 0)
			return false;
		if (!info->dimen.d[i])
			break;
		elements *= info->dimen.d[i];
		/* overflow? */
		if (elements > info->count)
			return false;
	}

	/* The rest should be all zero */
	for (i++; i < ARRAY_SIZE(info->dimen.d); i++)
		if (info->dimen.d[i])
			return false;

	return true;


thanks,

Takashi

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

[Takashi Sakamoto]

On Jul 1 2016 16:19, Takashi Iwai wrote:
> On Fri, 01 Jul 2016 06:15:10 +0200,
> 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@sakamocchi.jp>
>> ---
>>  sound/core/control.c | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index a85d455..eeb080f 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -805,6 +805,43 @@ 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;
>> +
>> +	/* The value for each level should be zero or positive. */
>> +	if (info->dimen.d[0] < 0)
>> +		return false;
>> +	members = info->dimen.d[0];
>> +
>> +	if (members > 0) {
>> +		for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
>> +			if (info->dimen.d[i] < 0)
>> +				return false;
>> +			if (info->dimen.d[i] == 0)
>> +				break;
>> +
>> +			/* Prevention of division by zero, for safe. */
>> +			if (members == 0)
>> +				return false;
>> +			/* Prevent arithmetic overflow. */
>> +			if (info->dimen.d[i] > UINT_MAX / members)
>> +				return false;
>> +
>> +			members *= info->dimen.d[i];
>> +		}
>> +	}
>> +
>> +	/* The rest of level should be zero. */
>> +	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
>> +		if (info->dimen.d[i] != 0)
>> +			return false;
>> +	}
>> +
>> +	return members <= info->count;
> 
> Well, since the dimen is 16bit short, it's easier to check the
> overflow just by comparing the value with info->count at each time
> (supposing info->count being the right value).

Your logic still causes arithmetic overflow. Let's assume a case that
the first element of dimension info has 3 or more and the second has
SHRT_MAX. It's better to avoid evaluation after calculation.

> That is, something like this:
> 
> 	/* If no dimension is given, it's OK */
> 	if (!info->dimen.d[0])
> 		return true;
> 
> 	elements = 1;
> 	for (i = 0; i < ARRAY_SIZE(info->dimen.d); i++) {
> 		if (info->dimen.d[i] < 0)
> 			return false;
> 		if (!info->dimen.d[i])
> 			break;

Were I you, I would insert codes to evaluate the element of dimension
info; i.e.

                if (info->dimen.d[i] > 512)
                        break;

Here, 512 is the maximum number of members which an element can have. In
this case, it's certainly an element of byte type.

> 		elements *= info->dimen.d[i];
> 		/* overflow? */
> 		if (elements > info->count)
> 			return false;
> 	}
> 
> 	/* The rest should be all zero */
> 	for (i++; i < ARRAY_SIZE(info->dimen.d); i++)
> 		if (info->dimen.d[i])
> 			return false;
> 
> 	return true;


Regards

Takashi Sakamoto

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

[Takashi Iwai]

On Fri, 01 Jul 2016 10:30:20 +0200,
Takashi Sakamoto wrote:
> 
> On Jul 1 2016 16:19, Takashi Iwai wrote:
> > On Fri, 01 Jul 2016 06:15:10 +0200,
> > 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@sakamocchi.jp>
> >> ---
> >>  sound/core/control.c | 39 +++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 39 insertions(+)
> >>
> >> diff --git a/sound/core/control.c b/sound/core/control.c
> >> index a85d455..eeb080f 100644
> >> --- a/sound/core/control.c
> >> +++ b/sound/core/control.c
> >> @@ -805,6 +805,43 @@ 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;
> >> +
> >> +	/* The value for each level should be zero or positive. */
> >> +	if (info->dimen.d[0] < 0)
> >> +		return false;
> >> +	members = info->dimen.d[0];
> >> +
> >> +	if (members > 0) {
> >> +		for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
> >> +			if (info->dimen.d[i] < 0)
> >> +				return false;
> >> +			if (info->dimen.d[i] == 0)
> >> +				break;
> >> +
> >> +			/* Prevention of division by zero, for safe. */
> >> +			if (members == 0)
> >> +				return false;
> >> +			/* Prevent arithmetic overflow. */
> >> +			if (info->dimen.d[i] > UINT_MAX / members)
> >> +				return false;
> >> +
> >> +			members *= info->dimen.d[i];
> >> +		}
> >> +	}
> >> +
> >> +	/* The rest of level should be zero. */
> >> +	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
> >> +		if (info->dimen.d[i] != 0)
> >> +			return false;
> >> +	}
> >> +
> >> +	return members <= info->count;
> > 
> > Well, since the dimen is 16bit short, it's easier to check the
> > overflow just by comparing the value with info->count at each time
> > (supposing info->count being the right value).
> 
> Your logic still causes arithmetic overflow. Let's assume a case that
> the first element of dimension info has 3 or more and the second has
> SHRT_MAX.

Remember that elements is 32bit unsigned int...

> It's better to avoid evaluation after calculation.
> 
> > That is, something like this:
> > 
> > 	/* If no dimension is given, it's OK */
> > 	if (!info->dimen.d[0])
> > 		return true;
> > 
> > 	elements = 1;
> > 	for (i = 0; i < ARRAY_SIZE(info->dimen.d); i++) {
> > 		if (info->dimen.d[i] < 0)
> > 			return false;
> > 		if (!info->dimen.d[i])
> > 			break;
> 
> Were I you, I would insert codes to evaluate the element of dimension
> info; i.e.
> 
>                 if (info->dimen.d[i] > 512)
>                         break;
> 
> Here, 512 is the maximum number of members which an element can have. In
> this case, it's certainly an element of byte type.

It's superfluous.  If info->count is already a sane value, it'd be
enough to compare with this.


Takashi

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

[Takashi Sakamoto]

On Jul 1 2016 17:50, Takashi Iwai wrote:
> On Fri, 01 Jul 2016 10:30:20 +0200,
> Takashi Sakamoto wrote:
>>
>> On Jul 1 2016 16:19, Takashi Iwai wrote:
>>> On Fri, 01 Jul 2016 06:15:10 +0200,
>>> 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@sakamocchi.jp>
>>>> ---
>>>>  sound/core/control.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/sound/core/control.c b/sound/core/control.c
>>>> index a85d455..eeb080f 100644
>>>> --- a/sound/core/control.c
>>>> +++ b/sound/core/control.c
>>>> @@ -805,6 +805,43 @@ 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;
>>>> +
>>>> +	/* The value for each level should be zero or positive. */
>>>> +	if (info->dimen.d[0] < 0)
>>>> +		return false;
>>>> +	members = info->dimen.d[0];
>>>> +
>>>> +	if (members > 0) {
>>>> +		for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
>>>> +			if (info->dimen.d[i] < 0)
>>>> +				return false;
>>>> +			if (info->dimen.d[i] == 0)
>>>> +				break;
>>>> +
>>>> +			/* Prevention of division by zero, for safe. */
>>>> +			if (members == 0)
>>>> +				return false;
>>>> +			/* Prevent arithmetic overflow. */
>>>> +			if (info->dimen.d[i] > UINT_MAX / members)
>>>> +				return false;
>>>> +
>>>> +			members *= info->dimen.d[i];
>>>> +		}
>>>> +	}
>>>> +
>>>> +	/* The rest of level should be zero. */
>>>> +	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
>>>> +		if (info->dimen.d[i] != 0)
>>>> +			return false;
>>>> +	}
>>>> +
>>>> +	return members <= info->count;
>>>
>>> Well, since the dimen is 16bit short, it's easier to check the
>>> overflow just by comparing the value with info->count at each time
>>> (supposing info->count being the right value).
>>
>> Your logic still causes arithmetic overflow. Let's assume a case that
>> the first element of dimension info has 3 or more and the second has
>> SHRT_MAX.
> 
> Remember that elements is 32bit unsigned int...

Oops. A document in my hand has wrong value about it...

Anyway, it's better to avoid evaluation after calculation because we can
assume that SHRT_MAX x SHRT_MAX from raw dimension info.

>> It's better to avoid evaluation after calculation.
>>
>>> That is, something like this:
>>>
>>> 	/* If no dimension is given, it's OK */
>>> 	if (!info->dimen.d[0])
>>> 		return true;
>>>
>>> 	elements = 1;
>>> 	for (i = 0; i < ARRAY_SIZE(info->dimen.d); i++) {
>>> 		if (info->dimen.d[i] < 0)
>>> 			return false;
>>> 		if (!info->dimen.d[i])
>>> 			break;
>>
>> Were I you, I would insert codes to evaluate the element of dimension
>> info; i.e.
>>
>>                 if (info->dimen.d[i] > 512)
>>                         break;
>>
>> Here, 512 is the maximum number of members which an element can have. In
>> this case, it's certainly an element of byte type.
> 
> It's superfluous.  If info->count is already a sane value, it'd be
> enough to compare with this.

The info->count comes from userspace or each driver. It's dangerous to
use it for avoiding arithmetic overflow.


Regards

Takashi Sakamoto

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

[Takashi Iwai]

On Fri, 01 Jul 2016 11:08:23 +0200,
Takashi Sakamoto wrote:
> 
> On Jul 1 2016 17:50, Takashi Iwai wrote:
> > On Fri, 01 Jul 2016 10:30:20 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> On Jul 1 2016 16:19, Takashi Iwai wrote:
> >>> On Fri, 01 Jul 2016 06:15:10 +0200,
> >>> 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@sakamocchi.jp>
> >>>> ---
> >>>>  sound/core/control.c | 39 +++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 39 insertions(+)
> >>>>
> >>>> diff --git a/sound/core/control.c b/sound/core/control.c
> >>>> index a85d455..eeb080f 100644
> >>>> --- a/sound/core/control.c
> >>>> +++ b/sound/core/control.c
> >>>> @@ -805,6 +805,43 @@ 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;
> >>>> +
> >>>> +	/* The value for each level should be zero or positive. */
> >>>> +	if (info->dimen.d[0] < 0)
> >>>> +		return false;
> >>>> +	members = info->dimen.d[0];
> >>>> +
> >>>> +	if (members > 0) {
> >>>> +		for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
> >>>> +			if (info->dimen.d[i] < 0)
> >>>> +				return false;
> >>>> +			if (info->dimen.d[i] == 0)
> >>>> +				break;
> >>>> +
> >>>> +			/* Prevention of division by zero, for safe. */
> >>>> +			if (members == 0)
> >>>> +				return false;
> >>>> +			/* Prevent arithmetic overflow. */
> >>>> +			if (info->dimen.d[i] > UINT_MAX / members)
> >>>> +				return false;
> >>>> +
> >>>> +			members *= info->dimen.d[i];
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	/* The rest of level should be zero. */
> >>>> +	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
> >>>> +		if (info->dimen.d[i] != 0)
> >>>> +			return false;
> >>>> +	}
> >>>> +
> >>>> +	return members <= info->count;
> >>>
> >>> Well, since the dimen is 16bit short, it's easier to check the
> >>> overflow just by comparing the value with info->count at each time
> >>> (supposing info->count being the right value).
> >>
> >> Your logic still causes arithmetic overflow. Let's assume a case that
> >> the first element of dimension info has 3 or more and the second has
> >> SHRT_MAX.
> > 
> > Remember that elements is 32bit unsigned int...
> 
> Oops. A document in my hand has wrong value about it...
> 
> Anyway, it's better to avoid evaluation after calculation because we can
> assume that SHRT_MAX x SHRT_MAX from raw dimension info.

SHRT_MAX * SHRT_MAX can't overflow.  You can compare the result with
info->count safely.  Take a look at the code closely.  It's comparison
*in the loop* at each multiplication.  This is how to check the
overflow more easily.  (And it's a kind of standard procedure.)


> >> It's better to avoid evaluation after calculation.
> >>
> >>> That is, something like this:
> >>>
> >>> 	/* If no dimension is given, it's OK */
> >>> 	if (!info->dimen.d[0])
> >>> 		return true;
> >>>
> >>> 	elements = 1;
> >>> 	for (i = 0; i < ARRAY_SIZE(info->dimen.d); i++) {
> >>> 		if (info->dimen.d[i] < 0)
> >>> 			return false;
> >>> 		if (!info->dimen.d[i])
> >>> 			break;
> >>
> >> Were I you, I would insert codes to evaluate the element of dimension
> >> info; i.e.
> >>
> >>                 if (info->dimen.d[i] > 512)
> >>                         break;
> >>
> >> Here, 512 is the maximum number of members which an element can have. In
> >> this case, it's certainly an element of byte type.
> > 
> > It's superfluous.  If info->count is already a sane value, it'd be
> > enough to compare with this.
> 
> The info->count comes from userspace or each driver. It's dangerous to
> use it for avoiding arithmetic overflow.

Your function is to verify the dimen array.  And for that, a sane
info->count value is prerequisite.  Otherwise how can you validate it
at all...?


Takashi

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

[Takashi Sakamoto]

On Jul 1 2016 18:52, Takashi Iwai wrote:
>>>> Were I you, I would insert codes to evaluate the element of dimension
>>>> info; i.e.
>>>>
>>>>                 if (info->dimen.d[i] > 512)
>>>>                         break;
>>>>
>>>> Here, 512 is the maximum number of members which an element can have. In
>>>> this case, it's certainly an element of byte type.
>>>
>>> It's superfluous.  If info->count is already a sane value, it'd be
>>> enough to compare with this.
>>
>> The info->count comes from userspace or each driver. It's dangerous to
>> use it for avoiding arithmetic overflow.
> 
> Your function is to verify the dimen array.  And for that, a sane
> info->count value is prerequisite.  Otherwise how can you validate it
> at all...?

Hm.

There's a large difference between us for the design of this local
function. You're interested in the result of calculation, while I
consider about the soundness of whole members in the dimension info.

In this time, I'll follow your insistence. I'd like to save my time
against this long-abandoned feature.


Regards

Takashi Sakamoto

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

[Takashi Iwai]

On Fri, 01 Jul 2016 12:46:39 +0200,
Takashi Sakamoto wrote:
> 
> On Jul 1 2016 18:52, Takashi Iwai wrote:
> >>>> Were I you, I would insert codes to evaluate the element of dimension
> >>>> info; i.e.
> >>>>
> >>>>                 if (info->dimen.d[i] > 512)
> >>>>                         break;
> >>>>
> >>>> Here, 512 is the maximum number of members which an element can have. In
> >>>> this case, it's certainly an element of byte type.
> >>>
> >>> It's superfluous.  If info->count is already a sane value, it'd be
> >>> enough to compare with this.
> >>
> >> The info->count comes from userspace or each driver. It's dangerous to
> >> use it for avoiding arithmetic overflow.
> > 
> > Your function is to verify the dimen array.  And for that, a sane
> > info->count value is prerequisite.  Otherwise how can you validate it
> > at all...?
> 
> Hm.
> 
> There's a large difference between us for the design of this local
> function. You're interested in the result of calculation, while I
> consider about the soundness of whole members in the dimension info.

No, my code cares about each member's overflow, too.
You just need to know how to detect the integer overflow more
efficiently.  It's a standard idiom.


Takashi

[PATCH 3/3] ALSA: control: add dimension validator for kernel driver

[Takashi Sakamoto]

Currently, kernel drivers are allowed to set arbitrary dimension
information to elements. The total number of members calculated by the
dimension information should be within the number of members in the
element, while there's no validator. When userspace applications have quite
simple implementation, this can cause buffer-over-run over
'struct snd_ctl_elem_value' data.

This commit adds the validation. Unfortunately, the dimension information
is set at runtime, thus the validation cannot run in advance.

As of Linux 4.7, there's no drivers to use the dimen information
except for Echo Audio PCI cards.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index eeb080f..19464d6 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -854,28 +854,44 @@ static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
 	down_read(&card->controls_rwsem);
 	kctl = snd_ctl_find_id(card, &info->id);
 	if (kctl == NULL) {
-		up_read(&card->controls_rwsem);
-		return -ENOENT;
+		result = -ENOENT;
+		goto end;
 	}
 #ifdef CONFIG_SND_DEBUG
 	info->access = 0;
 #endif
 	result = kctl->info(kctl, info);
-	if (result >= 0) {
-		snd_BUG_ON(info->access);
-		index_offset = snd_ctl_get_ioff(kctl, &info->id);
-		vd = &kctl->vd[index_offset];
-		snd_ctl_build_ioff(&info->id, kctl, index_offset);
-		info->access = vd->access;
-		if (vd->owner) {
-			info->access |= SNDRV_CTL_ELEM_ACCESS_LOCK;
-			if (vd->owner == ctl)
-				info->access |= SNDRV_CTL_ELEM_ACCESS_OWNER;
-			info->owner = pid_vnr(vd->owner->pid);
-		} else {
-			info->owner = -1;
-		}
+	if (result < 0)
+		goto end;
+
+	snd_BUG_ON(info->access);
+
+	/* 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;
 	}
+
+	index_offset = snd_ctl_get_ioff(kctl, &info->id);
+	vd = &kctl->vd[index_offset];
+	snd_ctl_build_ioff(&info->id, kctl, index_offset);
+	info->access = vd->access;
+
+	/* This element is not locked by any processes. */
+	if (vd->owner == NULL) {
+		info->owner = -1;
+		goto end;
+	}
+
+	info->owner = pid_vnr(vd->owner->pid);
+	info->access |= SNDRV_CTL_ELEM_ACCESS_LOCK;
+
+	/* This element is locked by this process. */
+	if (vd->owner == ctl)
+		info->access |= SNDRV_CTL_ELEM_ACCESS_OWNER;
+end:
 	up_read(&card->controls_rwsem);
 	return result;
 }
-- 
2.7.4

[PATCH 0/3 v3] ALSA: ctl: add dimension information validator

[Takashi Sakamoto]

This patchset is revised version of my previous one:
[alsa-devel] [PATCH 0/3 v2] ALSA: ctl: add dimension information validator
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-July/109830.html

Changes:
 * Apply simpler prevention of arithmetic overflow.

Takashi Sakamoto (3):
  ALSA: echoaudio: purge contradictions between dimension matrix members
    and total number of members
  ALSA: control: add dimension validator for userspace element
  ALSA: control: add dimension validator for kernel driver

 sound/core/control.c            | 78 ++++++++++++++++++++++++++++++++---------
 sound/pci/echoaudio/echoaudio.c |  6 ++--
 2 files changed, 65 insertions(+), 19 deletions(-)

-- 
2.7.4

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

[Takashi Sakamoto]

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@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;
+
+	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;
+		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;
+}
+
 static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
 			     struct snd_ctl_elem_info *info)
 {
@@ -1272,6 +1300,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_element_member_dimension(info))
+		return -EINVAL;
 	private_size = value_sizes[info->type] * info->count;
 
 	/*
-- 
2.7.4

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

[Takashi Sakamoto]

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@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;
> +
> +	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.

> +		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;
> +}
> +
>  static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
>  			     struct snd_ctl_elem_info *info)
>  {
> @@ -1272,6 +1300,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_element_member_dimension(info))
> +		return -EINVAL;
>  	private_size = value_sizes[info->type] * info->count;
>  
>  	/*
> 

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

[Takashi Iwai]

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@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;


thanks,

Takashi

Re: [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@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

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

[Takashi Iwai]

On Wed, 06 Jul 2016 15:07:41 +0200,
Takashi Sakamoto wrote:
> 
> 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@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?

Yes, a validation of info->count is a good idea.
And it can be even a simple WARN_ON().  It's a clear driver bug and
it's better to be exposed as loudly as possible.


Takashi

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

[Takashi Sakamoto]

On Jul 6 2016 22:34, Takashi Iwai wrote:
> Yes, a validation of info->count is a good idea.
> And it can be even a simple WARN_ON().  It's a clear driver bug and
> it's better to be exposed as loudly as possible.

OK.

Then another question. The same function, snd_ctl_elem_info() uses a
combination of  at CONFIG_SND_DEBUG and snd_BUG_ON() to detect
info->access bug of each driver.

http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n823

I guess that this is an assertion that each driver must not touch
info->access in its implementations of 'snd_kcontrol_info_t'. On the
other hand, the detection is just enabled at CONFIG_SND_DEBUG.

To me, it's strange, because ALSA control core changes its behaviour
depending on CONFIG_SND_DEBUG. If the option is off, buggy driver works.
Else, it brings kernel panic. This is quite confusing to both developers
of each driver and userspace applications.

In ALSA kernel/userspace interfaces, info->access is described as read-only.
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h#n886

Therefore, I think it better to set zero in advance of calling
'snd_kcontrol_info_t' then check the return value with WARN_ON macro,
regardless of CONFIG_SND_DEBUG.

Or should I apply snd_WARN_ON() or snd_BUG_ON() to validators of
info->type and info->count to keep code consistency?


Regards

Takashi Sakamoto

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

[Takashi Iwai]

On Wed, 06 Jul 2016 16:18:48 +0200,
Takashi Sakamoto wrote:
> 
> On Jul 6 2016 22:34, Takashi Iwai wrote:
> > Yes, a validation of info->count is a good idea.
> > And it can be even a simple WARN_ON().  It's a clear driver bug and
> > it's better to be exposed as loudly as possible.
> 
> OK.
> 
> Then another question. The same function, snd_ctl_elem_info() uses a
> combination of  at CONFIG_SND_DEBUG and snd_BUG_ON() to detect
> info->access bug of each driver.
> 
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n823
> 
> I guess that this is an assertion that each driver must not touch
> info->access in its implementations of 'snd_kcontrol_info_t'. On the
> other hand, the detection is just enabled at CONFIG_SND_DEBUG.
> 
> To me, it's strange, because ALSA control core changes its behaviour
> depending on CONFIG_SND_DEBUG. If the option is off, buggy driver works.
> Else, it brings kernel panic. This is quite confusing to both developers
> of each driver and userspace applications.
> 
> In ALSA kernel/userspace interfaces, info->access is described as read-only.
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h#n886
> 
> Therefore, I think it better to set zero in advance of calling
> 'snd_kcontrol_info_t' then check the return value with WARN_ON macro,
> regardless of CONFIG_SND_DEBUG.
> 
> Or should I apply snd_WARN_ON() or snd_BUG_ON() to validators of
> info->type and info->count to keep code consistency?

It's because we preferred the size optimization over the sanity check
in the past.  A sanity check is a thing to be done only during the
debug / development phase, and a production kernel we already have
must be clean from such a stupid bug -- that's the idea behind it.
That's why snd_BUG_ON() is enabled only with the debug option.
(BTW, there is no snd_WARN_ON().  snd_BUG_ON() corresponds to
 WARN_ON().)

This kind of concept was commonly seen in the earlier Linux kernels,
as we had to care more about the memory footprint ("each byte
matteres!"). Meanwhile, we have plenty of memory nowadays, and only
few people really care about the kernel size.  Hence people prefer a
standard macro (WARN_ON()) to an ALSA-specific one (snd_BUG_ON()) in
general recently.  But, it doesn't necessarily mean that we *should*
use / convert to WARN_ON().  It would still increase the kernel memory
footprint.

If I were to implement something, I'd take WARN_ON() for now.  But
keeping the same snd_BUG_ON() would be also good.  That said, I have
no preference at all.  This is merely a sanity check, and each driver
developer should test both with and without the debug option once,
after all.


Takashi

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

[Takashi Sakamoto]

On Jul 6 2016 23:40, Takashi Iwai wrote:
> On Wed, 06 Jul 2016 16:18:48 +0200,
> Takashi Sakamoto wrote:
>>
>> On Jul 6 2016 22:34, Takashi Iwai wrote:
>>> Yes, a validation of info->count is a good idea.
>>> And it can be even a simple WARN_ON().  It's a clear driver bug and
>>> it's better to be exposed as loudly as possible.
>>
>> OK.
>>
>> Then another question. The same function, snd_ctl_elem_info() uses a
>> combination of  at CONFIG_SND_DEBUG and snd_BUG_ON() to detect
>> info->access bug of each driver.
>>
>> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n823
>>
>> I guess that this is an assertion that each driver must not touch
>> info->access in its implementations of 'snd_kcontrol_info_t'. On the
>> other hand, the detection is just enabled at CONFIG_SND_DEBUG.
>>
>> To me, it's strange, because ALSA control core changes its behaviour
>> depending on CONFIG_SND_DEBUG. If the option is off, buggy driver works.
>> Else, it brings kernel panic. This is quite confusing to both developers
>> of each driver and userspace applications.
>>
>> In ALSA kernel/userspace interfaces, info->access is described as read-only.
>> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h#n886
>>
>> Therefore, I think it better to set zero in advance of calling
>> 'snd_kcontrol_info_t' then check the return value with WARN_ON macro,
>> regardless of CONFIG_SND_DEBUG.
>>
>> Or should I apply snd_WARN_ON() or snd_BUG_ON() to validators of
>> info->type and info->count to keep code consistency?
>
> It's because we preferred the size optimization over the sanity check
> in the past.  A sanity check is a thing to be done only during the
> debug / development phase, and a production kernel we already have
> must be clean from such a stupid bug -- that's the idea behind it.
> That's why snd_BUG_ON() is enabled only with the debug option.
>
> (BTW, there is no snd_WARN_ON().  snd_BUG_ON() corresponds to
>   WARN_ON().)

Oh, it's too misleading to kernel newbies.
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/sound/core.h#n351

Not intuitive at all. It's better to rename it to snd_WARN_ON()...

> This kind of concept was commonly seen in the earlier Linux kernels,
> as we had to care more about the memory footprint ("each byte
> matteres!"). Meanwhile, we have plenty of memory nowadays, and only
> few people really care about the kernel size.  Hence people prefer a
> standard macro (WARN_ON()) to an ALSA-specific one (snd_BUG_ON()) in
> general recently.  But, it doesn't necessarily mean that we *should*
> use / convert to WARN_ON().  It would still increase the kernel memory
> footprint.

The reduction of memory footprint is still interests to embedded 
developers. It's still better to care.

> If I were to implement something, I'd take WARN_ON() for now.  But
> keeping the same snd_BUG_ON() would be also good.  That said, I have
> no preference at all.  This is merely a sanity check, and each driver
> developer should test both with and without the debug option once,
> after all.

In my taste, enabling debug options is to retrieve more information 
about processing. It's worse practices to change bahaviours. (I can 
remember our discussion about tracepoints for snd-firewire-lib.)

Currently, I have no good idea to achieve both of small footprint and 
sanity check without the debug option. And what I'd like to achieve in 
this patchset is not to start discussion about it. In this time, I'll 
revise and post the second patch to improve handling of user-defined 
control element sets.


Regards

Takashi Sakamoto

[PATCH 0/3] ALSA: add dimension information validator

[Takashi Sakamoto]

Hi,

This patchset is to add a validator of dimension information for drivers
in kernel/userspace.

The dimension information was added to ALSA control interface. With the
information, members in an element compose multi-dimensional matrix. For
example, this matrix can be used for row/column table with values. In this
case, dimension level 1 and 2 are utilized to describe the number of rows
and columns by element member unit.

When dimension information has no contradictions to the number of members
in an element, there's no problem. Once it has, it can cause an issue. In
worst case, it can cause buffer-overrun in userspace.

The aim of this patchset is to prevent this situation.

Takashi Sakamoto (3):
  ALSA: echoaudio: purge contradictions between dimension matrix members
    and total number of members
  ALSA: control: add dimension validator for userspace element
  ALSA: control: add dimension validator for kernel driver

 sound/core/control.c            | 77 ++++++++++++++++++++++++++++++++---------
 sound/pci/echoaudio/echoaudio.c |  6 ++--
 2 files changed, 64 insertions(+), 19 deletions(-)

-- 
2.7.4

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

[Takashi Sakamoto]

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@sakamocchi.jp>
---
 sound/core/control.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index a85d455..af167ff 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -805,6 +805,33 @@ static int snd_ctl_elem_list(struct snd_card *card,
 	return 0;
 }
 
+static bool validate_dimension(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;
+		if (info->dimen.d[i] < 0)
+			return false;
+		elements *= info->dimen.d[i];
+	}
+
+	/* The rest of level should be zero. */
+	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
+		if (info->dimen.d[i] != 0)
+			return false;
+	}
+
+	return elements <= info->count;
+}
+
 static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
 			     struct snd_ctl_elem_info *info)
 {
@@ -1272,6 +1299,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_dimension(info))
+		return -EINVAL;
 	private_size = value_sizes[info->type] * info->count;
 
 	/*
-- 
2.7.4

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

[Takashi Iwai]

On Thu, 30 Jun 2016 16:04:44 +0200,
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@sakamocchi.jp>
> ---
>  sound/core/control.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index a85d455..af167ff 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -805,6 +805,33 @@ static int snd_ctl_elem_list(struct snd_card *card,
>  	return 0;
>  }
>  
> +static bool validate_dimension(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;
> +		if (info->dimen.d[i] < 0)
> +			return false;
> +		elements *= info->dimen.d[i];
> +	}

Just minor nit-picks:

- No check for the negative sign of the first element?

- It'd be clearer to check zero of the first element before the
  loop.

- Safer to have an integer overflow check in such calculation.



thanks,

Takashi


> +	/* The rest of level should be zero. */
> +	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
> +		if (info->dimen.d[i] != 0)
> +			return false;
> +	}
> +
> +	return elements <= info->count;
> +}
> +
>  static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
>  			     struct snd_ctl_elem_info *info)
>  {
> @@ -1272,6 +1299,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_dimension(info))
> +		return -EINVAL;
>  	private_size = value_sizes[info->type] * info->count;
>  
>  	/*
> -- 
> 2.7.4
> 

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

[Takashi Sakamoto]

Hi,

On Jun 30 2016 23:56, Takashi Iwai wrote:
> On Thu, 30 Jun 2016 16:04:44 +0200,
> 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@sakamocchi.jp>
>> ---
>>  sound/core/control.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index a85d455..af167ff 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -805,6 +805,33 @@ static int snd_ctl_elem_list(struct snd_card *card,
>>  	return 0;
>>  }
>>  
>> +static bool validate_dimension(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;
>> +		if (info->dimen.d[i] < 0)
>> +			return false;
>> +		elements *= info->dimen.d[i];
>> +	}
> 
> Just minor nit-picks:
> 
> - No check for the negative sign of the first element?
> 
> - It'd be clearer to check zero of the first element before the
>   loop.
> 
> - Safer to have an integer overflow check in such calculation.

Indeed. I'll post revised version, later.


Thanks

Takashi Sakamoto