[alsa-devel] [PATCH 2/3] ALSA: control: add dimension validator for userspace element

[Takashi Iwai]

On Fri, 01 Jul 2016 11:08:23 +0200,
Takashi Sakamoto wrote:
> 
> On Jul 1 2016 17:50, Takashi Iwai wrote:
> > On Fri, 01 Jul 2016 10:30:20 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> On Jul 1 2016 16:19, Takashi Iwai wrote:
> >>> On Fri, 01 Jul 2016 06:15:10 +0200,
> >>> 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 at sakamocchi.jp>
> >>>> ---
> >>>>  sound/core/control.c | 39 +++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 39 insertions(+)
> >>>>
> >>>> diff --git a/sound/core/control.c b/sound/core/control.c
> >>>> index a85d455..eeb080f 100644
> >>>> --- a/sound/core/control.c
> >>>> +++ b/sound/core/control.c
> >>>> @@ -805,6 +805,43 @@ 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;
> >>>> +
> >>>> +	/* The value for each level should be zero or positive. */
> >>>> +	if (info->dimen.d[0] < 0)
> >>>> +		return false;
> >>>> +	members = info->dimen.d[0];
> >>>> +
> >>>> +	if (members > 0) {
> >>>> +		for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
> >>>> +			if (info->dimen.d[i] < 0)
> >>>> +				return false;
> >>>> +			if (info->dimen.d[i] == 0)
> >>>> +				break;
> >>>> +
> >>>> +			/* Prevention of division by zero, for safe. */
> >>>> +			if (members == 0)
> >>>> +				return false;
> >>>> +			/* Prevent arithmetic overflow. */
> >>>> +			if (info->dimen.d[i] > UINT_MAX / members)
> >>>> +				return false;
> >>>> +
> >>>> +			members *= info->dimen.d[i];
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	/* The rest of level should be zero. */
> >>>> +	for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
> >>>> +		if (info->dimen.d[i] != 0)
> >>>> +			return false;
> >>>> +	}
> >>>> +
> >>>> +	return members <= info->count;
> >>>
> >>> Well, since the dimen is 16bit short, it's easier to check the
> >>> overflow just by comparing the value with info->count at each time
> >>> (supposing info->count being the right value).
> >>
> >> Your logic still causes arithmetic overflow. Let's assume a case that
> >> the first element of dimension info has 3 or more and the second has
> >> SHRT_MAX.
> > 
> > Remember that elements is 32bit unsigned int...
> 
> Oops. A document in my hand has wrong value about it...
> 
> Anyway, it's better to avoid evaluation after calculation because we can
> assume that SHRT_MAX x SHRT_MAX from raw dimension info.

SHRT_MAX * SHRT_MAX can't overflow.  You can compare the result with
info->count safely.  Take a look at the code closely.  It's comparison
*in the loop* at each multiplication.  This is how to check the
overflow more easily.  (And it's a kind of standard procedure.)


> >> It's better to avoid evaluation after calculation.
> >>
> >>> That is, something like this:
> >>>
> >>> 	/* If no dimension is given, it's OK */
> >>> 	if (!info->dimen.d[0])
> >>> 		return true;
> >>>
> >>> 	elements = 1;
> >>> 	for (i = 0; i < ARRAY_SIZE(info->dimen.d); i++) {
> >>> 		if (info->dimen.d[i] < 0)
> >>> 			return false;
> >>> 		if (!info->dimen.d[i])
> >>> 			break;
> >>
> >> Were I you, I would insert codes to evaluate the element of dimension
> >> info; i.e.
> >>
> >>                 if (info->dimen.d[i] > 512)
> >>                         break;
> >>
> >> Here, 512 is the maximum number of members which an element can have. In
> >> this case, it's certainly an element of byte type.
> > 
> > It's superfluous.  If info->count is already a sane value, it'd be
> > enough to compare with this.
> 
> The info->count comes from userspace or each driver. It's dangerous to
> use it for avoiding arithmetic overflow.

Your function is to verify the dimen array.  And for that, a sane
info->count value is prerequisite.  Otherwise how can you validate it
at all...?


Takashi