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