[alsa-devel] [PATCH 0/3] ALSA: add dimension information validator
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(-)
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; }
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;
/*
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
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
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 af167ff..4dbff2a 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -844,28 +844,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_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; }
participants (2)
-
Takashi Iwai -
Takashi Sakamoto