LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] ALSA: control: fix a redundant-copy issue
@ 2018-05-05 18:38 Wenwen Wang
  2018-05-13  7:06 ` Takashi Iwai
  0 siblings, 1 reply; 2+ messages in thread
From: Wenwen Wang @ 2018-05-05 18:38 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Jaroslav Kysela, Takashi Iwai, moderated list:SOUND,
	open list

In snd_ctl_elem_add_compat(), the fields of the struct 'data' need to be
copied from the corresponding fields of the struct 'data32' in userspace.
This is achieved by invoking copy_from_user() and get_user() functions. The
problem here is that the 'type' field is copied twice. One is by
copy_from_user() and one is by get_user(). Given that the 'type' field is
not used between the two copies, the second copy is *completely* redundant
and should be removed for better performance and cleanup. Also, these two
copies can cause inconsistent data: as the struct 'data32' resides in
userspace and a malicious userspace process can race to change the 'type'
field between the two copies to cause inconsistent data. Depending on how
the data is used in the future, such an inconsistency may cause potential
security risks.

For above reasons, we should take out the second copy.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 sound/core/control_compat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index a848836..507fd52 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -396,8 +396,7 @@ static int snd_ctl_elem_add_compat(struct snd_ctl_file *file,
 	if (copy_from_user(&data->id, &data32->id, sizeof(data->id)) ||
 	    copy_from_user(&data->type, &data32->type, 3 * sizeof(u32)))
 		goto error;
-	if (get_user(data->owner, &data32->owner) ||
-	    get_user(data->type, &data32->type))
+	if (get_user(data->owner, &data32->owner))
 		goto error;
 	switch (data->type) {
 	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
-- 
2.7.4

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] ALSA: control: fix a redundant-copy issue
  2018-05-05 18:38 [PATCH] ALSA: control: fix a redundant-copy issue Wenwen Wang
@ 2018-05-13  7:06 ` Takashi Iwai
  0 siblings, 0 replies; 2+ messages in thread
From: Takashi Iwai @ 2018-05-13  7:06 UTC (permalink / raw)
  To: Wenwen Wang; +Cc: moderated list:SOUND, Jaroslav Kysela, Kangjie Lu, open list

On Sat, 05 May 2018 20:38:03 +0200,
Wenwen Wang wrote:
> 
> In snd_ctl_elem_add_compat(), the fields of the struct 'data' need to be
> copied from the corresponding fields of the struct 'data32' in userspace.
> This is achieved by invoking copy_from_user() and get_user() functions. The
> problem here is that the 'type' field is copied twice. One is by
> copy_from_user() and one is by get_user(). Given that the 'type' field is
> not used between the two copies, the second copy is *completely* redundant
> and should be removed for better performance and cleanup. Also, these two
> copies can cause inconsistent data: as the struct 'data32' resides in
> userspace and a malicious userspace process can race to change the 'type'
> field between the two copies to cause inconsistent data. Depending on how
> the data is used in the future, such an inconsistency may cause potential
> security risks.
> 
> For above reasons, we should take out the second copy.
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>

Applied now, thanks.


Takashi

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-05-13  7:06 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-05 18:38 [PATCH] ALSA: control: fix a redundant-copy issue Wenwen Wang
2018-05-13  7:06 ` Takashi Iwai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).