-----Original Message-----
From: cip-dev@... <cip-dev@...> On
Behalf Of Alexander Grund
Sent: Saturday, March 25, 2023 2:18 AM
To: cip-dev@...
Cc: uli+cip@...
Subject: [cip-dev] [PATCH 4.4 1/3] ALSA: control: code refactoring for
ELEM_READ/ELEM_WRITE operations

From: Takashi Sakamoto <o-takashi@...>

ALSA control core handles ELEM_READ/ELEM_WRITE requests within lock
acquisition of a counting semaphore. The lock is acquired in helper functions
in the end of call path before calling implementations of each driver.

ioctl(2) with SNDRV_CTL_ELEM_READ
...
->snd_ctl_ioctl()
->snd_ctl_elem_read_user()
->snd_ctl_elem_read()
->down_read(controls_rwsem)
->snd_ctl_find_id()
->struct snd_kcontrol.get()
->up_read(controls_rwsem)

ioctl(2) with SNDRV_CTL_ELEM_WRITE
...
->snd_ctl_ioctl()
->snd_ctl_elem_write_user()
->snd_ctl_elem_write()
->down_read(controls_rwsem)
->snd_ctl_find_id()
->struct snd_kcontrol.put()
->up_read(controls_rwsem)

This commit moves the lock acquisition to middle of the call graph to simplify
the helper functions. As a result:

ioctl(2) with SNDRV_CTL_ELEM_READ
...
->snd_ctl_ioctl()
->snd_ctl_elem_read_user()
->down_read(controls_rwsem)
->snd_ctl_elem_read()
->snd_ctl_find_id()
->struct snd_kcontrol.get()
->up_read(controls_rwsem)

ioctl(2) with SNDRV_CTL_ELEM_WRITE
...
->snd_ctl_ioctl()
->snd_ctl_elem_write_user()
->down_read(controls_rwsem)
->snd_ctl_elem_write()
->snd_ctl_find_id()
->struct snd_kcontrol.put()
->up_read(controls_rwsem)

Change-Id: I6b39209aaf08afcbeca7c759b77bc96c67db4c77

Signed-off-by: Takashi Sakamoto <o-takashi@...>
Signed-off-by: Takashi Iwai <tiwai@...>

Fixes: e8064dec769e6 "ALSA: pcm: Move rwsem lock inside
snd_ctl_elem_read to prevent UAF"
Signed-off-by: Alexander Grund <theflamefire89@...>
---
sound/core/control.c | 78
+++++++++++++++++++++-----------------------
1 file changed, 38 insertions(+), 40 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c index
43c8eac250b8a..a042a30d6a728 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -877,24 +877,18 @@ static int snd_ctl_elem_read(struct snd_card *card,
struct snd_kcontrol *kctl;
struct snd_kcontrol_volatile *vd;
unsigned int index_offset;
- int result;

- down_read(&card->controls_rwsem);
kctl = snd_ctl_find_id(card, &control->id);
- if (kctl == NULL) {
- result = -ENOENT;
- } else {
- index_offset = snd_ctl_get_ioff(kctl, &control->id);
- vd = &kctl->vd[index_offset];
- if ((vd->access & SNDRV_CTL_ELEM_ACCESS_READ) &&
- kctl->get != NULL) {
- snd_ctl_build_ioff(&control->id, kctl, index_offset);
- result = kctl->get(kctl, control);
- } else
- result = -EPERM;
- }
- up_read(&card->controls_rwsem);
- return result;
+ if (kctl == NULL)
+ return -ENOENT;
+
+ index_offset = snd_ctl_get_ioff(kctl, &control->id);
+ vd = &kctl->vd[index_offset];
+ if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && kctl->get
== NULL)
+ return -EPERM;
+
+ snd_ctl_build_ioff(&control->id, kctl, index_offset);
+ return kctl->get(kctl, control);
}

static int snd_ctl_elem_read_user(struct snd_card *card, @@ -909,8 +903,11
@@ static int snd_ctl_elem_read_user(struct snd_card *card,

snd_power_lock(card);
result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
- if (result >= 0)
+ if (result >= 0) {
+ down_read(&card->controls_rwsem);
result = snd_ctl_elem_read(card, control);
+ up_read(&card->controls_rwsem);
+ }
snd_power_unlock(card);
if (result >= 0)
if (copy_to_user(_control, control, sizeof(*control))) @@
-927,30 +924,28 @@ static int snd_ctl_elem_write(struct snd_card *card,
struct snd_ctl_file *file,
unsigned int index_offset;
int result;

- down_read(&card->controls_rwsem);
kctl = snd_ctl_find_id(card, &control->id);
- if (kctl == NULL) {
- result = -ENOENT;
- } else {
- index_offset = snd_ctl_get_ioff(kctl, &control->id);
- vd = &kctl->vd[index_offset];
- if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) ||
- kctl->put == NULL ||
- (file && vd->owner && vd->owner != file)) {
- result = -EPERM;
- } else {
- snd_ctl_build_ioff(&control->id, kctl, index_offset);
- result = kctl->put(kctl, control);
- }
- if (result > 0) {
- struct snd_ctl_elem_id id = control->id;
- up_read(&card->controls_rwsem);
- snd_ctl_notify(card,
SNDRV_CTL_EVENT_MASK_VALUE, &id);
- return 0;
- }
+ if (kctl == NULL)
+ return -ENOENT;
+
+ index_offset = snd_ctl_get_ioff(kctl, &control->id);
+ vd = &kctl->vd[index_offset];
+ if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kctl->put
== NULL ||
+ (file && vd->owner && vd->owner != file)) {
+ return -EPERM;
}
- up_read(&card->controls_rwsem);
- return result;
+
+ snd_ctl_build_ioff(&control->id, kctl, index_offset);
+ result = kctl->put(kctl, control);
+ if (result < 0)
+ return result;
+
+ if (result > 0) {
+ struct snd_ctl_elem_id id = control->id;
+ snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE,
&id);
+ }
+
+ return 0;
}

static int snd_ctl_elem_write_user(struct snd_ctl_file *file, @@ -967,8
+962,11 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file,
card = file->card;
snd_power_lock(card);
result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
- if (result >= 0)
+ if (result >= 0) {
+ down_read(&card->controls_rwsem);
result = snd_ctl_elem_write(card, file, control);
+ up_read(&card->controls_rwsem);
+ }
snd_power_unlock(card);
if (result >= 0)
if (copy_to_user(_control, control, sizeof(*control)))
--
2.40.0