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

[Takashi Iwai <tiwai@suse.de>]

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