Re: [PATCH 2/3] ALSA: control: add dimension validator for userspace element

From: Takashi Iwai <tiwai@suse.de>
To: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Cc: alsa-devel@alsa-project.org, clemens@ladisch.de,
	ffado-devel@lists.sf.net
Subject: Re: [PATCH 2/3] ALSA: control: add dimension validator for userspace element
Date: Wed, 06 Jul 2016 15:34:59 +0200	[thread overview]
Message-ID: <s5h60sizy7w.wl-tiwai@suse.de> (raw)
In-Reply-To: <577D029D.9090701@sakamocchi.jp>

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