[alsa-devel] [PATCH] ctl: fix to handle several elements added by one operation for userspace element

[Takashi Iwai]

At Sun, 12 Apr 2015 18:51:16 +0900,
Takashi Sakamoto wrote:
> 
> Iwai-san,
> 
> On Apr 12 2015 14:51, Takashi Iwai wrote:
> > At Sun, 12 Apr 2015 10:12:25 +0900,
> > Takashi Sakamoto wrote:
> >>
> >> An element instance can have several elements with the same feature.
> >> Some userspace applications can add such an element instance by add
> >> operation with the number of elements. Then, the element instance
> >> gets a memory object to keep states of these elements.
> >>
> >> But the element instance has just one memory object for the elements.
> >> This causes the same result to each read/write operations to the
> >> different elements.
> >>
> >> This commit fixes this bug by allocating enough memory objects to the
> >> element instance for each of elements.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
> >> ---
> >>  sound/core/control.c | 16 +++++++++++-----
> >>  1 file changed, 11 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/sound/core/control.c b/sound/core/control.c
> >> index ccb1ca2..5b2c352 100644
> >> --- a/sound/core/control.c
> >> +++ b/sound/core/control.c
> >> @@ -1029,7 +1029,7 @@ static int snd_ctl_elem_unlock(struct snd_ctl_file *file,
> >>  struct user_element {
> >>  	struct snd_ctl_elem_info info;
> >>  	struct snd_card *card;
> >> -	void *elem_data;		/* element data */
> >> +	char *elem_data;		/* element data */
> >>  	unsigned long elem_data_size;	/* size of element data in bytes */
> >>  	void *tlv_data;			/* TLV data */
> >>  	unsigned long tlv_data_size;	/* TLV data size */
> >> @@ -1078,9 +1078,12 @@ static int snd_ctl_elem_user_get(struct snd_kcontrol *kcontrol,
> >>  				 struct snd_ctl_elem_value *ucontrol)
> >>  {
> >>  	struct user_element *ue = kcontrol->private_data;
> >> +	unsigned int size = ue->elem_data_size;
> >> +	char *src = ue->elem_data +
> >> +			snd_ctl_get_ioff(kcontrol, &ucontrol->id) * size;
> > 
> > You still need to check the validity of snd_ctl_get_ioff() value.
> > The user-space can give any value here, and accessing without the
> > check will expose the kernel memory to user-space at get or even
> > corrupt at put.
> 
> I don't agree this indication.
> 
> The 'ucontrol' has ID information re-calculated by the caller
> (snd_ctl_elem_read() / snd_ctl_elem_write()), thus it's not directly
> passed from userspace. If the information includes something wrong, the
> caller returns error and don't execute callee.
> 
> The snd_ctl_elem_add() has already allocated enough memory, so there're
> no wrong reference as long as the re-calculated ID includes larger numid
> than (kctl->numid + kctl->count), or larger index than kctl->count.
> 
> Are there any cases that the re-calculated ID includes such wrong
> information after passing these functions below?
>  * snd_ctl_find_id()    -> find kctl with given ID info
>  * snd_ctl_get_ioff()   -> calculate offset with kctl and given ID
>  * snd_ctl_build_ioff() -> re-calculate ID with kctl and the offset
> 
> If it still includes wrong information, it's the caller's fault and
> should be fixed.

Hrm, OK, although I prefer having a safer check in the callee side as
a pessimist (there are lots of direct calls of put callbacks in many
driver codes).  Let's cross fingers.

Applied now.  Thanks.


Takashi