[PATCH 0/4] ALSA: ctl: fix userspace control element operations

[PATCH 0/4] ALSA: ctl: fix userspace control element operations

[Takashi Sakamoto]

This patchset is to fix some issues related to operations for userspace
control element. Three patches are related to identical information,
the other is related to several element added by one operation.

Unfortunately, current alsa-lib has no functionality to achieve the
operation to add several element. To demonstrate this and test these
patches, I prepare for a small GObject introspection library, alsa-gir.
https://github.com/takaswie/alsa-gir

This reposiroty includes sample scripts written by python. The file of
sample/ctl.py demonstrates userspace control elements.

If this library helps your review for this patchset, I'm feeling happy.


Takashi Sakamoto (4):
  ALSA: ctl: confirm to return all identical information in 'activate' event
  ALSA: ctl: fix a bug to return no identical information in info operation
    for userspace controls
  ALSA: ctl: fill identical information to return value when adding userspace
    elements
  ALSA: ctl: fix to handle several elements added by one operation for
    userspace element

 sound/core/control.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

-- 
2.1.0

[PATCH 1/4] ALSA: ctl: confirm to return all identical information

[Takashi Sakamoto]

When event originator doesn't set numerical ID in identical information,
the event data includes no numerical ID, thus userspace applications
cannot identify the control just by unique ID in event data.

This commit fix this bug so as the event data includes all of identical
information.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index d677c27..6d12e85 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -578,6 +578,7 @@ error:
  *
  * Finds the control instance with the given id, and activate or
  * inactivate the control together with notification, if changed.
+ * The given ID data is filled by full information.
  *
  * Return: 0 if unchanged, 1 if changed, or a negative error code on failure.
  */
@@ -609,6 +610,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
 	}
 	ret = 1;
  unlock:
+	*id = kctl->id;
 	up_write(&card->controls_rwsem);
 	if (ret > 0)
 		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
-- 
2.1.0

[PATCH 2/4] ALSA: ctl: fix a bug to return no identical information in

[Takashi Sakamoto]

In operations of SNDRV_CTL_IOCTL_ELEM_INFO, identical information in
returned value is cleared. This is not better to userspace application.

This commit confirms to return full identical information to the
operations.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index 6d12e85..37203a6 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1042,6 +1042,8 @@ static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
 	struct user_element *ue = kcontrol->private_data;
 
 	*uinfo = ue->info;
+	uinfo->id = kcontrol->id;
+
 	return 0;
 }
 
@@ -1055,6 +1057,7 @@ static int snd_ctl_elem_user_enum_info(struct snd_kcontrol *kcontrol,
 	item = uinfo->value.enumerated.item;
 
 	*uinfo = ue->info;
+	uinfo->id = kcontrol->id;
 
 	item = min(item, uinfo->value.enumerated.items - 1);
 	uinfo->value.enumerated.item = item;
-- 
2.1.0

[PATCH 3/4] ALSA: ctl: fill identical information to return value

[Takashi Sakamoto]

currently some members related identical information are not fiiled
in returned parameter of SNDRV_CTL_IOCTL_ELEM_ADD. This is not better
for userspace application.

This commit copies information to returned value.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 37203a6..b0b110a 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1312,6 +1312,13 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	err = snd_ctl_add(card, kctl);
 	if (err < 0)
 		return err;
+	info->id = kctl->id;
+	/*
+	 * We cannot fill any field for the number of elements added by this
+	 * operation because there're no specific fields. The usage of 'owner'
+	 * field for this purpose may cause any bugs to userspace applications
+	 * because the field originally means any applications lock it.
+	 */
 
 	down_write(&card->controls_rwsem);
 	card->user_ctl_count++;
@@ -1321,12 +1328,20 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 }
 
 static int snd_ctl_elem_add_user(struct snd_ctl_file *file,
-				 struct snd_ctl_elem_info __user *_info, int replace)
+				 struct snd_ctl_elem_info __user *_info,
+				 int replace)
 {
 	struct snd_ctl_elem_info info;
+	int err;
+
 	if (copy_from_user(&info, _info, sizeof(info)))
 		return -EFAULT;
-	return snd_ctl_elem_add(file, &info, replace);
+	err = snd_ctl_elem_add(file, &info, replace);
+	if (err < 0)
+		return err;
+	if (copy_to_user(_info, &info, sizeof(info)))
+		return -EFAULT;
+	return 0;
 }
 
 static int snd_ctl_elem_remove(struct snd_ctl_file *file,
-- 
2.1.0

[PATCH 4/4] ALSA: ctl: fix to handle several elements added by

[Takashi Sakamoto]

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, an userspace element
instance keeps a memory object to keep state of these elements.

But the element instance has just one memory object for the elements.
This forces each read/write operations to the different elements
brings the same result.

This commit fixes this bug by allocating enough memory objects to element
instance for each of elements.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index b0b110a..2ab7ee5 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1074,9 +1074,14 @@ static int snd_ctl_elem_user_get(struct snd_kcontrol *kcontrol,
 				 struct snd_ctl_elem_value *ucontrol)
 {
 	struct user_element *ue = kcontrol->private_data;
+	char *data = ue->elem_data;
+	unsigned int size = ue->elem_data_size;
+	unsigned int offset;
+
+	offset = (ucontrol->id.numid - kcontrol->id.numid) * size;
 
 	mutex_lock(&ue->card->user_ctl_lock);
-	memcpy(&ucontrol->value, ue->elem_data, ue->elem_data_size);
+	memcpy(&ucontrol->value, data + offset, size);
 	mutex_unlock(&ue->card->user_ctl_lock);
 	return 0;
 }
@@ -1084,13 +1089,18 @@ static int snd_ctl_elem_user_get(struct snd_kcontrol *kcontrol,
 static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
 				 struct snd_ctl_elem_value *ucontrol)
 {
-	int change;
 	struct user_element *ue = kcontrol->private_data;
+	char *data = ue->elem_data;
+	unsigned int size = ue->elem_data_size;
+	unsigned int offset;
+	int change;
+
+	offset = (ucontrol->id.numid - kcontrol->id.numid) * size;
 
 	mutex_lock(&ue->card->user_ctl_lock);
-	change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0;
+	change = memcmp(&ucontrol->value, data + offset, size) != 0;
 	if (change)
-		memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size);
+		memcpy(data + offset, &ucontrol->value, size);
 	mutex_unlock(&ue->card->user_ctl_lock);
 	return change;
 }
@@ -1273,7 +1283,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (err < 0)
 		return err;
 	memcpy(&kctl->id, &info->id, sizeof(kctl->id));
-	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size,
+	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count,
 				     GFP_KERNEL);
 	if (kctl->private_data == NULL) {
 		kfree(kctl);
-- 
2.1.0

Re: [PATCH 1/4] ALSA: ctl: confirm to return all identical information

[Takashi Iwai]

At Thu,  9 Apr 2015 02:07:15 +0900,
Takashi Sakamoto wrote:
> 
> When event originator doesn't set numerical ID in identical information,
> the event data includes no numerical ID, thus userspace applications
> cannot identify the control just by unique ID in event data.
> 
> This commit fix this bug so as the event data includes all of identical
> information.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/control.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index d677c27..6d12e85 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -578,6 +578,7 @@ error:
>   *
>   * Finds the control instance with the given id, and activate or
>   * inactivate the control together with notification, if changed.
> + * The given ID data is filled by full information.
>   *
>   * Return: 0 if unchanged, 1 if changed, or a negative error code on failure.
>   */
> @@ -609,6 +610,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
>  	}
>  	ret = 1;
>   unlock:
> +	*id = kctl->id;

This isn't always correct.  When the element is looked by a numid and
a kctl has multiple items (count > 0), this will overwrite with a
wrong numid.


Takashi

>  	up_write(&card->controls_rwsem);
>  	if (ret > 0)
>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
> -- 
> 2.1.0
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 1/4] ALSA: ctl: confirm to return all identical information

[Takashi Sakamoto]

On 2015年04月09日 14:36, Takashi Iwai wrote:
> At Thu,  9 Apr 2015 02:07:15 +0900,
> Takashi Sakamoto wrote:
>>
>> When event originator doesn't set numerical ID in identical information,
>> the event data includes no numerical ID, thus userspace applications
>> cannot identify the control just by unique ID in event data.
>>
>> This commit fix this bug so as the event data includes all of identical
>> information.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>  sound/core/control.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index d677c27..6d12e85 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -578,6 +578,7 @@ error:
>>   *
>>   * Finds the control instance with the given id, and activate or
>>   * inactivate the control together with notification, if changed.
>> + * The given ID data is filled by full information.
>>   *
>>   * Return: 0 if unchanged, 1 if changed, or a negative error code on failure.
>>   */
>> @@ -609,6 +610,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
>>  	}
>>  	ret = 1;
>>   unlock:
>> +	*id = kctl->id;
> 
> This isn't always correct.  When the element is looked by a numid and
> a kctl has multiple items (count > 0), this will overwrite with a
> wrong numid.

It also overwrites with a wrong index, too. I should have used a
combination of snd_ctl_get_ioff() and snd_ctl_build_ioff() for this
purpose. Thanks.

> Takashi
> 
>>  	up_write(&card->controls_rwsem);
>>  	if (ret > 0)
>>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
>> -- 
>> 2.1.0

Well, I'll re-post the patch 01 and 04 with these fixes because the
others has somewhat changes of API behaviour, expecially patch 03. I'll
postpone it to next developing period.

But how about patch 02? I think filling all identical information gives
advantage to userspace applications, instead of a part of given
information given.


And I have no confidence about giving index value in add operations for
userspace. In my understanding, the index is an offset from the first
element in the element set. But actually, via the add operation, we can
add new userspace element with an arbitrary index, like:

$ amixer controls
(added by an operation with index=0. As a result, 11 is given as numid
for this element set.)
numid=11,iface=MIXER,name='my-enum-element'
numid=12,iface=MIXER,name='my-enum-element',index=1
numid=13,iface=MIXER,name='my-enum-element',index=2
numid=14,iface=MIXER,name='my-enum-element',index=3
 - there're some other elements. -
(added by another operation with index=5. As a result, 21 is given as
numid for this element set.)
numid=21,iface=MIXER,name='my-enum-element',index=4
numid=22,iface=MIXER,name='my-enum-element',index=5
numid=23,iface=MIXER,name='my-enum-element',index=6
numid=24,iface=MIXER,name='my-enum-element',index=7
(FYI, addition with index=1-8 causes EBUSY.)

Of cource, we can do this.
(added by an operation with 100. As a result, 0 is given as numid for
this element set.)
numid=0,iface=MIXER,name='my-enum-element',index=100
numid=1,iface=MIXER,name='my-enum-element',index=101
numid=2,iface=MIXER,name='my-enum-element',index=102
numid=3,iface=MIXER,name='my-enum-element',index=103

Is this behaviour legal or not?

# I'm sorry to post these questions just before opening next merge
window but it takes me
# a long time to understand ALSA control interface and prepare for some
helper programs
# to confirm its behaviours...


Thanks

Takashi Sakamoto

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ALSA: ctl: confirm to return all identical information

[Takashi Iwai]

At Fri, 10 Apr 2015 21:00:22 +0900,
Takashi Sakamoto wrote:
> 
> On 2015年04月09日 14:36, Takashi Iwai wrote:
> > At Thu,  9 Apr 2015 02:07:15 +0900,
> > Takashi Sakamoto wrote:
> >>
> >> When event originator doesn't set numerical ID in identical information,
> >> the event data includes no numerical ID, thus userspace applications
> >> cannot identify the control just by unique ID in event data.
> >>
> >> This commit fix this bug so as the event data includes all of identical
> >> information.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> >> ---
> >>  sound/core/control.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/sound/core/control.c b/sound/core/control.c
> >> index d677c27..6d12e85 100644
> >> --- a/sound/core/control.c
> >> +++ b/sound/core/control.c
> >> @@ -578,6 +578,7 @@ error:
> >>   *
> >>   * Finds the control instance with the given id, and activate or
> >>   * inactivate the control together with notification, if changed.
> >> + * The given ID data is filled by full information.
> >>   *
> >>   * Return: 0 if unchanged, 1 if changed, or a negative error code on failure.
> >>   */
> >> @@ -609,6 +610,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
> >>  	}
> >>  	ret = 1;
> >>   unlock:
> >> +	*id = kctl->id;
> > 
> > This isn't always correct.  When the element is looked by a numid and
> > a kctl has multiple items (count > 0), this will overwrite with a
> > wrong numid.
> 
> It also overwrites with a wrong index, too. I should have used a
> combination of snd_ctl_get_ioff() and snd_ctl_build_ioff() for this
> purpose. Thanks.
> 
> > Takashi
> > 
> >>  	up_write(&card->controls_rwsem);
> >>  	if (ret > 0)
> >>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
> >> -- 
> >> 2.1.0
> 
> Well, I'll re-post the patch 01 and 04 with these fixes because the
> others has somewhat changes of API behaviour, expecially patch 03. I'll
> postpone it to next developing period.
> 
> But how about patch 02? I think filling all identical information gives
> advantage to userspace applications, instead of a part of given
> information given.

If each of them should be applied individually, let me know.
But I think this may have the very same problem as patch 1.

> And I have no confidence about giving index value in add operations for
> userspace. In my understanding, the index is an offset from the first
> element in the element set.

No, the index number is just to allow multiple kctls with the same
name.  Using counts > 1 is only one of the implementations for multi
indices.  You can implement one kctl per index, too.  (Many drivers do
so.)


Takashi

> But actually, via the add operation, we can
> add new userspace element with an arbitrary index, like:
> 
> $ amixer controls
> (added by an operation with index=0. As a result, 11 is given as numid
> for this element set.)
> numid=11,iface=MIXER,name='my-enum-element'
> numid=12,iface=MIXER,name='my-enum-element',index=1
> numid=13,iface=MIXER,name='my-enum-element',index=2
> numid=14,iface=MIXER,name='my-enum-element',index=3
>  - there're some other elements. -
> (added by another operation with index=5. As a result, 21 is given as
> numid for this element set.)
> numid=21,iface=MIXER,name='my-enum-element',index=4
> numid=22,iface=MIXER,name='my-enum-element',index=5
> numid=23,iface=MIXER,name='my-enum-element',index=6
> numid=24,iface=MIXER,name='my-enum-element',index=7
> (FYI, addition with index=1-8 causes EBUSY.)
> 
> Of cource, we can do this.
> (added by an operation with 100. As a result, 0 is given as numid for
> this element set.)
> numid=0,iface=MIXER,name='my-enum-element',index=100
> numid=1,iface=MIXER,name='my-enum-element',index=101
> numid=2,iface=MIXER,name='my-enum-element',index=102
> numid=3,iface=MIXER,name='my-enum-element',index=103
> 
> Is this behaviour legal or not?
> 
> # I'm sorry to post these questions just before opening next merge
> window but it takes me
> # a long time to understand ALSA control interface and prepare for some
> helper programs
> # to confirm its behaviours...
> 
> 
> Thanks
> 
> Takashi Sakamoto
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 0/4 v2] fix userspace control element operations

[Takashi Sakamoto]

This patchset is revised version of my previous one.

[alsa-devel] [PATCH 0/4] ALSA: ctl: fix userspace control element operations
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-April/090206.html

Changes:
 - Use helper functions to processing identical information
 - Revise a comment about the number of added element in returned value
 - Remove elements at add operation when EFAULT occurs

Takashi Sakamoto (4):
  ctl: confirm to return all identical information in 'activate' event
  ctl: fix a bug to return no identical information in info operation
    for userspace controls
  ctl: fill identical information to return value when adding userspace
    elements
  ctl: fix to handle several elements added by one operation for
    userspace element

 sound/core/control.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

-- 
2.1.0

[PATCH 1/4] ctl: confirm to return all identical information in 'activate' event

[Takashi Sakamoto]

When event originator doesn't set numerical ID in identical information,
the event data includes no numerical ID, thus userspace applications
cannot identify the control just by unique ID in event data.

This commit fix this bug so as the event data includes all of identical
information.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index 00fcaa0..90a9e5d 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -578,6 +578,7 @@ error:
  *
  * Finds the control instance with the given id, and activate or
  * inactivate the control together with notification, if changed.
+ * The given ID data is filled with full information.
  *
  * Return: 0 if unchanged, 1 if changed, or a negative error code on failure.
  */
@@ -607,6 +608,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
 			goto unlock;
 		vd->access |= SNDRV_CTL_ELEM_ACCESS_INACTIVE;
 	}
+	snd_ctl_build_ioff(id, kctl, index_offset);
 	ret = 1;
  unlock:
 	up_write(&card->controls_rwsem);
-- 
2.1.0

[PATCH 2/4] ctl: fix a bug to return no identical information in info operation for userspace controls

[Takashi Sakamoto]

In operations of SNDRV_CTL_IOCTL_ELEM_INFO, identical information in
returned value is cleared. This is not better to userspace application.

This commit confirms to return full identical information to the
operations.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index 90a9e5d..a750846 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1040,8 +1040,12 @@ static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_info *uinfo)
 {
 	struct user_element *ue = kcontrol->private_data;
+	unsigned int offset;
 
+	offset = snd_ctl_get_ioff(kcontrol, &uinfo->id);
 	*uinfo = ue->info;
+	snd_ctl_build_ioff(&uinfo->id, kcontrol, offset);
+
 	return 0;
 }
 
@@ -1051,10 +1055,13 @@ static int snd_ctl_elem_user_enum_info(struct snd_kcontrol *kcontrol,
 	struct user_element *ue = kcontrol->private_data;
 	const char *names;
 	unsigned int item;
+	unsigned int offset;
 
 	item = uinfo->value.enumerated.item;
 
+	offset = snd_ctl_get_ioff(kcontrol, &uinfo->id);
 	*uinfo = ue->info;
+	snd_ctl_build_ioff(&uinfo->id, kcontrol, offset);
 
 	item = min(item, uinfo->value.enumerated.items - 1);
 	uinfo->value.enumerated.item = item;
-- 
2.1.0

[PATCH 3/4] ctl: fill identical information to return value when adding userspace elements

[Takashi Sakamoto]

currently some members related identical information are not fiiled
in returned parameter of SNDRV_CTL_IOCTL_ELEM_ADD. This is not better
for userspace application.

This commit copies information to returned value. When failing to copy
into userspace, the added elements are going to be removed. Then, no
applications can lock these elements between adding and removing because
these are already locked.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index a750846..ccb1ca2 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1214,6 +1214,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	unsigned int access;
 	long private_size;
 	struct user_element *ue;
+	unsigned int offset;
 	int err;
 
 	if (!*info->id.name)
@@ -1316,6 +1317,15 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	err = snd_ctl_add(card, kctl);
 	if (err < 0)
 		return err;
+	offset = snd_ctl_get_ioff(kctl, &info->id);
+	snd_ctl_build_ioff(&info->id, kctl, offset);
+	/*
+	 * Here we cannot fill any field for the number of elements added by
+	 * this operation because there're no specific fields. The usage of
+	 * 'owner' field for this purpose may cause any bugs to userspace
+	 * applications because the field originally means PID of a process
+	 * which locks the element.
+	 */
 
 	down_write(&card->controls_rwsem);
 	card->user_ctl_count++;
@@ -1328,9 +1338,19 @@ static int snd_ctl_elem_add_user(struct snd_ctl_file *file,
 				 struct snd_ctl_elem_info __user *_info, int replace)
 {
 	struct snd_ctl_elem_info info;
+	int err;
+
 	if (copy_from_user(&info, _info, sizeof(info)))
 		return -EFAULT;
-	return snd_ctl_elem_add(file, &info, replace);
+	err = snd_ctl_elem_add(file, &info, replace);
+	if (err < 0)
+		return err;
+	if (copy_to_user(_info, &info, sizeof(info))) {
+		snd_ctl_remove_user_ctl(file, &info.id);
+		return -EFAULT;
+	}
+
+	return 0;
 }
 
 static int snd_ctl_elem_remove(struct snd_ctl_file *file,
-- 
2.1.0

[PATCH 4/4] ctl: fix to handle several elements added by one operation for userspace element

[Takashi Sakamoto]

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, an userspace element
instance keeps a memory object to keep state 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 element
instance for each of elements.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index ccb1ca2..23ea738 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -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;
+	/* The caller filled whole identical information. */
+	char *target = ue->elem_data + ucontrol->id.index * size;
 
 	mutex_lock(&ue->card->user_ctl_lock);
-	memcpy(&ucontrol->value, ue->elem_data, ue->elem_data_size);
+	memcpy(&ucontrol->value, target, size);
 	mutex_unlock(&ue->card->user_ctl_lock);
 	return 0;
 }
@@ -1090,11 +1093,14 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
 {
 	int change;
 	struct user_element *ue = kcontrol->private_data;
+	unsigned int size = ue->elem_data_size;
+	/* The caller filled whole identical information. */
+	char *target = ue->elem_data + ucontrol->id.index * size;
 
 	mutex_lock(&ue->card->user_ctl_lock);
-	change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0;
+	change = memcmp(&ucontrol->value, target, size) != 0;
 	if (change)
-		memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size);
+		memcpy(target, &ucontrol->value, size);
 	mutex_unlock(&ue->card->user_ctl_lock);
 	return change;
 }
@@ -1278,7 +1284,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (err < 0)
 		return err;
 	memcpy(&kctl->id, &info->id, sizeof(kctl->id));
-	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size,
+	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count,
 				     GFP_KERNEL);
 	if (kctl->private_data == NULL) {
 		kfree(kctl);
-- 
2.1.0

Re: [PATCH 1/4] ctl: confirm to return all identical information in 'activate' event

[Takashi Iwai]

At Sat, 11 Apr 2015 17:41:02 +0900,
Takashi Sakamoto wrote:
> 
> When event originator doesn't set numerical ID in identical information,
> the event data includes no numerical ID, thus userspace applications
> cannot identify the control just by unique ID in event data.
> 
> This commit fix this bug so as the event data includes all of identical
> information.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

> ---
>  sound/core/control.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 00fcaa0..90a9e5d 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -578,6 +578,7 @@ error:
>   *
>   * Finds the control instance with the given id, and activate or
>   * inactivate the control together with notification, if changed.
> + * The given ID data is filled with full information.
>   *
>   * Return: 0 if unchanged, 1 if changed, or a negative error code on failure.
>   */
> @@ -607,6 +608,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
>  			goto unlock;
>  		vd->access |= SNDRV_CTL_ELEM_ACCESS_INACTIVE;
>  	}
> +	snd_ctl_build_ioff(id, kctl, index_offset);
>  	ret = 1;
>   unlock:
>  	up_write(&card->controls_rwsem);
> -- 
> 2.1.0
> 

Re: [PATCH 2/4] ctl: fix a bug to return no identical information in info operation for userspace controls

[Takashi Iwai]

At Sat, 11 Apr 2015 17:41:03 +0900,
Takashi Sakamoto wrote:
> 
> In operations of SNDRV_CTL_IOCTL_ELEM_INFO, identical information in
> returned value is cleared. This is not better to userspace application.
> 
> This commit confirms to return full identical information to the
> operations.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

> ---
>  sound/core/control.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 90a9e5d..a750846 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1040,8 +1040,12 @@ static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
>  				  struct snd_ctl_elem_info *uinfo)
>  {
>  	struct user_element *ue = kcontrol->private_data;
> +	unsigned int offset;
>  
> +	offset = snd_ctl_get_ioff(kcontrol, &uinfo->id);
>  	*uinfo = ue->info;
> +	snd_ctl_build_ioff(&uinfo->id, kcontrol, offset);
> +
>  	return 0;
>  }
>  
> @@ -1051,10 +1055,13 @@ static int snd_ctl_elem_user_enum_info(struct snd_kcontrol *kcontrol,
>  	struct user_element *ue = kcontrol->private_data;
>  	const char *names;
>  	unsigned int item;
> +	unsigned int offset;
>  
>  	item = uinfo->value.enumerated.item;
>  
> +	offset = snd_ctl_get_ioff(kcontrol, &uinfo->id);
>  	*uinfo = ue->info;
> +	snd_ctl_build_ioff(&uinfo->id, kcontrol, offset);
>  
>  	item = min(item, uinfo->value.enumerated.items - 1);
>  	uinfo->value.enumerated.item = item;
> -- 
> 2.1.0
> 

Re: [PATCH 3/4] ctl: fill identical information to return value when adding userspace elements

[Takashi Iwai]

At Sat, 11 Apr 2015 17:41:04 +0900,
Takashi Sakamoto wrote:
> 
> currently some members related identical information are not fiiled
> in returned parameter of SNDRV_CTL_IOCTL_ELEM_ADD. This is not better
> for userspace application.
> 
> This commit copies information to returned value. When failing to copy
> into userspace, the added elements are going to be removed. Then, no
> applications can lock these elements between adding and removing because
> these are already locked.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

> ---
>  sound/core/control.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index a750846..ccb1ca2 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1214,6 +1214,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	unsigned int access;
>  	long private_size;
>  	struct user_element *ue;
> +	unsigned int offset;
>  	int err;
>  
>  	if (!*info->id.name)
> @@ -1316,6 +1317,15 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	err = snd_ctl_add(card, kctl);
>  	if (err < 0)
>  		return err;
> +	offset = snd_ctl_get_ioff(kctl, &info->id);
> +	snd_ctl_build_ioff(&info->id, kctl, offset);
> +	/*
> +	 * Here we cannot fill any field for the number of elements added by
> +	 * this operation because there're no specific fields. The usage of
> +	 * 'owner' field for this purpose may cause any bugs to userspace
> +	 * applications because the field originally means PID of a process
> +	 * which locks the element.
> +	 */
>  
>  	down_write(&card->controls_rwsem);
>  	card->user_ctl_count++;
> @@ -1328,9 +1338,19 @@ static int snd_ctl_elem_add_user(struct snd_ctl_file *file,
>  				 struct snd_ctl_elem_info __user *_info, int replace)
>  {
>  	struct snd_ctl_elem_info info;
> +	int err;
> +
>  	if (copy_from_user(&info, _info, sizeof(info)))
>  		return -EFAULT;
> -	return snd_ctl_elem_add(file, &info, replace);
> +	err = snd_ctl_elem_add(file, &info, replace);
> +	if (err < 0)
> +		return err;
> +	if (copy_to_user(_info, &info, sizeof(info))) {
> +		snd_ctl_remove_user_ctl(file, &info.id);
> +		return -EFAULT;
> +	}
> +
> +	return 0;
>  }
>  
>  static int snd_ctl_elem_remove(struct snd_ctl_file *file,
> -- 
> 2.1.0
> 

Re: [PATCH 0/4 v2] fix userspace control element operations

[Takashi Iwai]

At Sat, 11 Apr 2015 17:41:01 +0900,
Takashi Sakamoto wrote:
> 
> This patchset is revised version of my previous one.
> 
> [alsa-devel] [PATCH 0/4] ALSA: ctl: fix userspace control element operations
> http://mailman.alsa-project.org/pipermail/alsa-devel/2015-April/090206.html
> 
> Changes:
>  - Use helper functions to processing identical information
>  - Revise a comment about the number of added element in returned value
>  - Remove elements at add operation when EFAULT occurs
> 
> Takashi Sakamoto (4):
>   ctl: confirm to return all identical information in 'activate' event
>   ctl: fix a bug to return no identical information in info operation
>     for userspace controls
>   ctl: fill identical information to return value when adding userspace
>     elements
>   ctl: fix to handle several elements added by one operation for
>     userspace element
> 
>  sound/core/control.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)

If you resend a new patch series, please don't hang under the previous
thread.


thanks,

Takashi

Re: [PATCH 4/4] ctl: fix to handle several elements added by one operation for userspace element

[Takashi Iwai]

At Sat, 11 Apr 2015 17:41:05 +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, an userspace element
> instance keeps a memory object to keep state 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 element
> instance for each of elements.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/control.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index ccb1ca2..23ea738 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -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;
> +	/* The caller filled whole identical information. */
> +	char *target = ue->elem_data + ucontrol->id.index * size;
>  
>  	mutex_lock(&ue->card->user_ctl_lock);
> -	memcpy(&ucontrol->value, ue->elem_data, ue->elem_data_size);
> +	memcpy(&ucontrol->value, target, size);

You must check the validity of the address, i.e. index offset.
This is a security hole.


thanks,

Takashi

>  	mutex_unlock(&ue->card->user_ctl_lock);
>  	return 0;
>  }
> @@ -1090,11 +1093,14 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
>  {
>  	int change;
>  	struct user_element *ue = kcontrol->private_data;
> +	unsigned int size = ue->elem_data_size;
> +	/* The caller filled whole identical information. */
> +	char *target = ue->elem_data + ucontrol->id.index * size;
>  
>  	mutex_lock(&ue->card->user_ctl_lock);
> -	change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0;
> +	change = memcmp(&ucontrol->value, target, size) != 0;
>  	if (change)
> -		memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size);
> +		memcpy(target, &ucontrol->value, size);
>  	mutex_unlock(&ue->card->user_ctl_lock);
>  	return change;
>  }
> @@ -1278,7 +1284,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	if (err < 0)
>  		return err;
>  	memcpy(&kctl->id, &info->id, sizeof(kctl->id));
> -	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size,
> +	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count,
>  				     GFP_KERNEL);
>  	if (kctl->private_data == NULL) {
>  		kfree(kctl);
> -- 
> 2.1.0
> 

Re: [PATCH 4/4] ctl: fix to handle several elements added by one operation for userspace element

[Takashi Sakamoto]

On 2015年04月12日 00:42, Takashi Iwai wrote:
> At Sat, 11 Apr 2015 17:41:05 +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, an userspace element
>> instance keeps a memory object to keep state 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 element
>> instance for each of elements.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>  sound/core/control.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index ccb1ca2..23ea738 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -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;
>> +	/* The caller filled whole identical information. */
>> +	char *target = ue->elem_data + ucontrol->id.index * size;
>>  
>>  	mutex_lock(&ue->card->user_ctl_lock);
>> -	memcpy(&ucontrol->value, ue->elem_data, ue->elem_data_size);
>> +	memcpy(&ucontrol->value, target, size);
> 
> You must check the validity of the address, i.e. index offset.
> This is a security hole.

OK. I forgot the case that the value of kctl->index starts 1 or larger...


Thanks

Takashi Sakamoto

> thanks,
> 
> Takashi
> 
>>  	mutex_unlock(&ue->card->user_ctl_lock);
>>  	return 0;
>>  }
>> @@ -1090,11 +1093,14 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
>>  {
>>  	int change;
>>  	struct user_element *ue = kcontrol->private_data;
>> +	unsigned int size = ue->elem_data_size;
>> +	/* The caller filled whole identical information. */
>> +	char *target = ue->elem_data + ucontrol->id.index * size;
>>  
>>  	mutex_lock(&ue->card->user_ctl_lock);
>> -	change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0;
>> +	change = memcmp(&ucontrol->value, target, size) != 0;
>>  	if (change)
>> -		memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size);
>> +		memcpy(target, &ucontrol->value, size);
>>  	mutex_unlock(&ue->card->user_ctl_lock);
>>  	return change;
>>  }
>> @@ -1278,7 +1284,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>>  	if (err < 0)
>>  		return err;
>>  	memcpy(&kctl->id, &info->id, sizeof(kctl->id));
>> -	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size,
>> +	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count,
>>  				     GFP_KERNEL);
>>  	if (kctl->private_data == NULL) {
>>  		kfree(kctl);
>> -- 
>> 2.1.0
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel