[alsa-devel] [PATCH] ALSA: control: Add sanity checks for user ctl id name string

[Takashi Iwai]

At Thu, 12 Mar 2015 09:08:31 +0900,
Takashi Sakamoto wrote:
> 
> Iwai-san,
> 
> I have some questions for this patch.
> 
> On Mar 12 2015 02:15, Takashi Iwai wrote:
> > There was no check about the id string of user control elements, so we
> > accepted even a control element with an empty string, which is
> > obviously bogus.  This patch adds more sanity checks of id strings.
> > 
> > Cc: <stable at vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > ---
> >  sound/core/control.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/sound/core/control.c b/sound/core/control.c
> > index 35324a8e83c8..7ed2b214b16d 100644
> > --- a/sound/core/control.c
> > +++ b/sound/core/control.c
> > @@ -1170,6 +1170,10 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> >  
> >  	if (info->count < 1)
> >  		return -EINVAL;
> > +	if (*info->id.name)
> > +		return -EINVAL;
> 
> This evaluates the first byte of 'struct snd_ctl_elem_id.name[44]' and
> return -EINVAL if the byte is non-zero. This means that userspace
> application cannot set arbitrary strings into this member because the
> first byte should be zero.

Gah, ! was dropped mistakenly while rebasing from the old version..

> > +	if (strnlen(info->id.name, sizeof(info->id.name)) > sizeof(info->id.name))
> > +		return -EINVAL;
> >  	access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
> >  		(info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
> >  				 SNDRV_CTL_ELEM_ACCESS_INACTIVE|
> > 
> 
> The strnlen() return the length excluding terminator (\0). Even if all
> of the 44 bytes are filled without terminator, this conditional
> statement doesn't return -EINVAL. But I think this is the case that we
> should prevent in this patch.
> 
> Therefore, 'if (strnlen(info->id.name, sizeof(info->id.name)) >=
> sizeof(info->id.name))' fully satisfies the aim of this patch.

Right, the reason of this mistake was again due to rebase --
namely, the old version of the patch used strnlen_user() in the caller
side, and this function returns the length *including* terminator.

Will refresh it now...


thanks,

Takashi