[alsa-devel] [PATCH 0/3 v3] ALSA: ctl: add dimension information validator
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(-)
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 | 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;
/*
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;
/*
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
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/...
What do you think about it?
Regards
Takashi Sakamoto
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/...
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
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/...
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/uap...
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
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/...
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/uap...
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
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/...
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/uap...
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/sou...
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
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 54da910..a0927ae 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -845,28 +845,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; }
participants (2)
-
Takashi Iwai -
Takashi Sakamoto