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@xxxxxxxxxxxxx> > >>> --- > >>> 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel