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