[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

[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

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