Re: [PATCH 3/5] ALSA: control: fix logic error about control count in a device

[Takashi Iwai <tiwai@suse.de>]

At Wed, 11 Feb 2015 19:40:11 +0900,
Takashi Sakamoto wrote:
> 
> It's assumed that the number of userspace controls is just 1 in several
> parts, while this assumptions is not always true because the value of
> 'owner' member can be assigned to.
> 
> This commit fixes this issue.

Well, the current code isn't incorrect, it deals with the number of
grouped elements, not the total number of elements.  So, this is
rather a change of the semantics of card->user_ctl_count field than
a fix, and it's the question: whether we should limit for the whole
number of elements.

There is a very slight chance of user-space breakage by counting the
whole numbers, but pragmatically seen, I think it's acceptable from
the safety POV.

However, changing the error code is no-go.


thanks,

Takashi

> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/control.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 1edd6c5..bce4730 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -514,6 +514,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
>  {
>  	struct snd_card *card = file->card;
>  	struct snd_kcontrol *kctl;
> +	unsigned int count;
>  	int i, ret;
>  
>  	down_write(&card->controls_rwsem);
> @@ -531,10 +532,11 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
>  			ret = -EBUSY;
>  			goto error;
>  		}
> +	count = kctl->count;
>  	ret = snd_ctl_remove(card, kctl);
>  	if (ret < 0)
>  		goto error;
> -	card->user_ctl_count--;
> +	card->user_ctl_count -= count;
>  error:
>  	up_write(&card->controls_rwsem);
>  	return ret;
> @@ -1202,10 +1204,15 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  			return err;
>  	}
>  
> -	if (card->user_ctl_count >= MAX_USER_CONTROLS)
> -		return -ENOMEM;
> +	/*
> +	 * The number of controls with the same feature, distinguished by index.
> +	 */
> +	kctl.count = info->owner;
> +	if (kctl.count == 0)
> +		kctl.count = 1;
> +	if (card->user_ctl_count + kctl.count > MAX_USER_CONTROLS)
> +		return -ENOSPC;
>  
> -	kctl.count = info->owner ? info->owner : 1;
>  	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
>  		kctl.info = snd_ctl_elem_user_enum_info;
>  	else
> @@ -1259,7 +1266,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  		return err;
>  
>  	down_write(&card->controls_rwsem);
> -	card->user_ctl_count++;
> +	card->user_ctl_count += _kctl->count;
>  	up_write(&card->controls_rwsem);
>  
>  	return 0;
> -- 
> 2.1.0
>