[PATCH 4.4 0/3] ALSA: control: Fix locking around snd_ctl_elem_read/write

[PATCH 4.4 0/3] ALSA: control: Fix locking around snd_ctl_elem_read/write

[Alexander Grund]

I found a bug in v4.4-st38 caused by e8064dec769e6 "ALSA: pcm: Move rwsem lock inside snd_ctl_elem_read to prevent UAF"
The most serious part is a `down_write` call before calling `snd_ctl_elem_write` which will do a `down_read` on the same mutex causing a deadlock.
I fix this by taking missing commits from 4.14.y, especially "ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations"
which moves the locking out of those 2 functions such that the bug fixed
by the above commit is actually introduced so that it can be fixed
avoiding the deadlock.
This also requires a follow-up commit to fix a bug introduced in a
condition by that commit.
The final commit replaces `down_read` by `down_write` in `snd_ctl_elem_write_user`
similar to what e8064dec769e6 does in control_compat.c so that this use
is uniform (and correct).


Richard Fitzgerald (1):
  ALSA: control: Fix memory corruption risk in snd_ctl_elem_read

Takashi Sakamoto (2):
  ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations
  ALSA: control: use counting semaphore as write lock for ELEM_WRITE
    operation

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

-- 
2.40.0

[PATCH 4.4 1/3] ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations

[Alexander Grund]

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

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@sakamocchi.jp>
Signed-off-by: Takashi Iwai <tiwai@suse.de>

Fixes: e8064dec769e6 "ALSA: pcm: Move rwsem lock inside snd_ctl_elem_read to prevent UAF"
Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
---
 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

[PATCH 4.4 2/3] ALSA: control: Fix memory corruption risk in snd_ctl_elem_read

[Alexander Grund]

From: Richard Fitzgerald <rf@opensource.cirrus.com>

commit 5a23699a39abc5328921a81b89383d088f6ba9cc upstream.

The patch "ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE
operations" introduced a potential for kernel memory corruption due
to an incorrect if statement allowing non-readable controls to fall
through and call the get function. For TLV controls a driver can omit
SNDRV_CTL_ELEM_ACCESS_READ to ensure that only the TLV get function
can be called. Instead the normal get() can be invoked unexpectedly
and as the driver expects that this will only be called for controls
<= 512 bytes, potentially try to copy >512 bytes into the 512 byte
return array, so corrupting kernel memory.

The problem is an attempt to refactor the snd_ctl_elem_read function
to invert the logic so that it conditionally aborted if the control
is unreadable instead of conditionally executing. But the if statement
wasn't inverted correctly.

The correct inversion of

    if (a && !b)

is
    if (!a || b)

Fixes: becf9e5d553c2 ("ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations")
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
---
 sound/core/control.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index a042a30d6a728..3ca81e85a1492 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -884,7 +884,7 @@ static int snd_ctl_elem_read(struct snd_card *card,
 
 	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)
+	if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) || kctl->get == NULL)
 		return -EPERM;
 
 	snd_ctl_build_ioff(&control->id, kctl, index_offset);
-- 
2.40.0

[PATCH 4.4 3/3] ALSA: control: use counting semaphore as write lock for ELEM_WRITE operation

[Alexander Grund]

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

In ALSA control interface, applications can execute two types of request
for value of members on each element; ELEM_READ and ELEM_WRITE. In ALSA
control core, these two requests are handled within read lock of a
counting semaphore, therefore several processes can run to execute these
two requests at the same time. This has an issue because ELEM_WRITE
requests have an effect to change state of the target element. Concurrent
access should be controlled for each of ELEM_READ/ELEM_WRITE case.

This commit uses the counting semaphore as write lock for ELEM_WRITE
requests, while use it as read lock for ELEM_READ requests. The state of
a target element is maintained exclusively between ELEM_WRITE/ELEM_READ
operations.

There's a concern. If the counting semaphore is acquired for read lock
in implementations of 'struct snd_kcontrol.put()' in each driver, this
commit shall cause dead lock. As of v4.13-rc5, 'snd-mixer-oss.ko',
'snd-emu10k1.ko' and 'snd-soc-sst-atom-hifi2-platform.ko' includes codes
for read locks, but these are not in a call graph from
'struct snd_kcontrol.put(). Therefore, this commit is safe.

In current implementation, the same solution is applied for the other
operations to element; e.g. ELEM_LOCK and ELEM_UNLOCK. There's another
discussion about an overhead to maintain concurrent access to an element
during operating the other elements on the same card instance, because the
lock primitive is originally implemented to maintain a list of elements on
the card instance. There's a substantial difference between
per-element-list lock and per-element lock.

Here, let me investigate another idea to add per-element lock to maintain
the concurrent accesses with inquiry/change requests to an element. It's
not so frequent for applications to operate members on elements, while
adding a new lock primitive to structure increases memory footprint for
all of element sets somehow. Experimentally, inquiry operation is more
frequent than change operation and usage of counting semaphore for the
inquiry operation brings no blocking to the other inquiry operations. Thus
the overhead is not so critical for usual applications. For the above
reasons, in this commit, the per-element lock is not introduced.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
---
 sound/core/control.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 3ca81e85a1492..04b939df3768b 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -963,9 +963,9 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file,
 	snd_power_lock(card);
 	result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
 	if (result >= 0) {
-		down_read(&card->controls_rwsem);
+		down_write(&card->controls_rwsem);
 		result = snd_ctl_elem_write(card, file, control);
-		up_read(&card->controls_rwsem);
+		up_write(&card->controls_rwsem);
 	}
 	snd_power_unlock(card);
 	if (result >= 0)
-- 
2.40.0

RE: [cip-dev] [PATCH 4.4 1/3] ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations

[nobuhiro1.iwamatsu]

Hi,

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

Please add original commit id.

> 
> 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

The original commit doesn't seem to have this tag.

> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Fixes: e8064dec769e6 "ALSA: pcm: Move rwsem lock inside
> snd_ctl_elem_read to prevent UAF"
> Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
> ---
>  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

Best regards,
  Nobuhiro

RE: [cip-dev] [PATCH 4.4 2/3] ALSA: control: Fix memory corruption risk in snd_ctl_elem_read

[nobuhiro1.iwamatsu]

Hi,

Thanks for your patch!

> -----Original Message-----
> From: cip-dev@lists.cip-project.org <cip-dev@lists.cip-project.org> On
> Behalf Of Alexander Grund
> Sent: Saturday, March 25, 2023 2:18 AM
> To: cip-dev@lists.cip-project.org
> Cc: uli+cip@fpond.eu
> Subject: [cip-dev] [PATCH 4.4 2/3] ALSA: control: Fix memory corruption risk in
> snd_ctl_elem_read
> 
> From: Richard Fitzgerald <rf@opensource.cirrus.com>
> 
> commit 5a23699a39abc5328921a81b89383d088f6ba9cc upstream.
> 
> The patch "ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE
> operations" introduced a potential for kernel memory corruption due to an
> incorrect if statement allowing non-readable controls to fall through and call
> the get function. For TLV controls a driver can omit
> SNDRV_CTL_ELEM_ACCESS_READ to ensure that only the TLV get function
> can be called. Instead the normal get() can be invoked unexpectedly and as the
> driver expects that this will only be called for controls <= 512 bytes, potentially
> try to copy >512 bytes into the 512 byte return array, so corrupting kernel
> memory.
> 
> The problem is an attempt to refactor the snd_ctl_elem_read function to invert
> the logic so that it conditionally aborted if the control is unreadable instead of
> conditionally executing. But the if statement wasn't inverted correctly.
> 
> The correct inversion of
> 
>     if (a && !b)
> 
> is
>     if (!a || b)
> 
> Fixes: becf9e5d553c2 ("ALSA: control: code refactoring for
> ELEM_READ/ELEM_WRITE operations")
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 

Space here is not required.

> Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
> ---
>  sound/core/control.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c index
> a042a30d6a728..3ca81e85a1492 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -884,7 +884,7 @@ static int snd_ctl_elem_read(struct snd_card *card,
> 
>  	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)
> +	if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) || kctl->get
> == NULL)
>  		return -EPERM;
> 
>  	snd_ctl_build_ioff(&control->id, kctl, index_offset);
> --
> 2.40.0

Best regards,
  Nobuhiro

RE: [cip-dev] [PATCH 4.4 3/3] ALSA: control: use counting semaphore as write lock for ELEM_WRITE operation

[nobuhiro1.iwamatsu]

Hi,

Thanks for your patch.

> -----Original Message-----
> From: cip-dev@lists.cip-project.org <cip-dev@lists.cip-project.org> On
> Behalf Of Alexander Grund
> Sent: Saturday, March 25, 2023 2:18 AM
> To: cip-dev@lists.cip-project.org
> Cc: uli+cip@fpond.eu
> Subject: [cip-dev] [PATCH 4.4 3/3] ALSA: control: use counting semaphore as
> write lock for ELEM_WRITE operation
> 
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Please add backports commit id. 

> 
> In ALSA control interface, applications can execute two types of request for
> value of members on each element; ELEM_READ and ELEM_WRITE. In ALSA
> control core, these two requests are handled within read lock of a counting
> semaphore, therefore several processes can run to execute these two requests
> at the same time. This has an issue because ELEM_WRITE requests have an
> effect to change state of the target element. Concurrent access should be
> controlled for each of ELEM_READ/ELEM_WRITE case.
> 
> This commit uses the counting semaphore as write lock for ELEM_WRITE
> requests, while use it as read lock for ELEM_READ requests. The state of a
> target element is maintained exclusively between ELEM_WRITE/ELEM_READ
> operations.
> 
> There's a concern. If the counting semaphore is acquired for read lock in
> implementations of 'struct snd_kcontrol.put()' in each driver, this commit shall
> cause dead lock. As of v4.13-rc5, 'snd-mixer-oss.ko', 'snd-emu10k1.ko' and
> 'snd-soc-sst-atom-hifi2-platform.ko' includes codes for read locks, but these
> are not in a call graph from 'struct snd_kcontrol.put(). Therefore, this commit is
> safe.
> 
> In current implementation, the same solution is applied for the other operations
> to element; e.g. ELEM_LOCK and ELEM_UNLOCK. There's another discussion
> about an overhead to maintain concurrent access to an element during
> operating the other elements on the same card instance, because the lock
> primitive is originally implemented to maintain a list of elements on the card
> instance. There's a substantial difference between per-element-list lock and
> per-element lock.
> 
> Here, let me investigate another idea to add per-element lock to maintain the
> concurrent accesses with inquiry/change requests to an element. It's not so
> frequent for applications to operate members on elements, while adding a new
> lock primitive to structure increases memory footprint for all of element sets
> somehow. Experimentally, inquiry operation is more frequent than change
> operation and usage of counting semaphore for the inquiry operation brings no
> blocking to the other inquiry operations. Thus the overhead is not so critical for
> usual applications. For the above reasons, in this commit, the per-element lock
> is not introduced.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
> ---
>  sound/core/control.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c index
> 3ca81e85a1492..04b939df3768b 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -963,9 +963,9 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file
> *file,
>  	snd_power_lock(card);
>  	result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
>  	if (result >= 0) {
> -		down_read(&card->controls_rwsem);
> +		down_write(&card->controls_rwsem);
>  		result = snd_ctl_elem_write(card, file, control);
> -		up_read(&card->controls_rwsem);
> +		up_write(&card->controls_rwsem);
>  	}
>  	snd_power_unlock(card);
>  	if (result >= 0)
> --
> 2.40.0

Best regards,
  Nobuhiro

Re: [PATCH 4.4 1/3] ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations

[Alexander Grund]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

Hi,
> Please add original commit id.

becf9e5d553c2389d857a3c178ce80fdb34a02e1

> The original commit doesn't seem to have this tag.

Oof, this gets auto-added when operating/comitting inside the Android tree. Just ignore/remove this.

Do I need to submit the updated patch (actually only the description changes) as a new mail? If so how'd I do that, e.g. with git send-mail?

[-- Attachment #2: Type: text/html, Size: 435 bytes --]

Re: [PATCH 4.4 3/3] ALSA: control: use counting semaphore as write lock for ELEM_WRITE operation

[Alexander Grund]

[-- Attachment #1: Type: text/plain, Size: 381 bytes --]

On Thu, Mar 30, 2023 at 12:39 AM, Nobuhiro Iwamatsu wrote:

> 
> Please add backports commit id.

Can you help me here please: What exactly should this look like?
I backported/cherry-picked this from 5bbb1ab5bd0b01c4f0b19ae03fdfec487f839517 from the linux-4.14.y branch

Do I need to submit the updated patch as a new mail? If so how'd I do that, e.g. with git send-mail?

[-- Attachment #2: Type: text/html, Size: 414 bytes --]

Re: [PATCH 4.4 2/3] ALSA: control: Fix memory corruption risk in snd_ctl_elem_read

[Alexander Grund]

[-- Attachment #1: Type: text/plain, Size: 125 bytes --]

> 
> Space here is not required.

I agree. Can this be just ignored when applying this or do I need to resend the patch?

[-- Attachment #2: Type: text/html, Size: 142 bytes --]

Re: [cip-dev] [PATCH 4.4 0/3] ALSA: control: Fix locking around snd_ctl_elem_read/write

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

Hi!

> I found a bug in v4.4-st38 caused by e8064dec769e6 "ALSA: pcm: Move
>  rwsem lock inside snd_ctl_elem_read to prevent UAF"

One option would be to revert e8064dec769e6. 

Second option is to cherry-pick these:

> Richard Fitzgerald (1):
>   ALSA: control: Fix memory corruption risk in snd_ctl_elem_read
> 
> Takashi Sakamoto (2):
>   ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations
>   ALSA: control: use counting semaphore as write lock for ELEM_WRITE
>     operation

They are in 4.14, it is probably easiest to cherry-pick them from
there.

Thanks for submission!

Best regards,
								Pavel

-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 4.4 0/3] ALSA: control: Fix locking around snd_ctl_elem_read/write

[Alexander Grund]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

> 
> Second option is to cherry-pick these:

That's exactly what I did here. From my submissions to the upstream kernels I was told that a 0/x EMail is required for a multi-commit change.

I haven't found a guideline how to submit patches to CIP kernels, so I just followed that procedure. See: https://lists.cip-project.org/g/cip-dev/message/11117 https://lists.cip-project.org/g/cip-dev/message/11118 https://lists.cip-project.org/g/cip-dev/message/11119

It seems that was wrong. So can you tell me please how to submit patches to CIP in the future and whether the current mails are acceptable (despite the minor description changes requested there)?

Thanks,

Alex

[-- Attachment #2: Type: text/html, Size: 1028 bytes --]

Re: [cip-dev] [PATCH 4.4 0/3] ALSA: control: Fix locking around snd_ctl_elem_read/write

[Ulrich Hecht]

> On 04/02/2023 6:30 PM CEST Pavel Machek <pavel@denx.de> wrote:
> Second option is to cherry-pick these:
> 
> > Richard Fitzgerald (1):
> >   ALSA: control: Fix memory corruption risk in snd_ctl_elem_read
> > 
> > Takashi Sakamoto (2):
> >   ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations
> >   ALSA: control: use counting semaphore as write lock for ELEM_WRITE
> >     operation
> 
> They are in 4.14, it is probably easiest to cherry-pick them from
> there.

They don't apply cleanly, though, so I'm happy to pick up Alexander's series.

CU
Uli

Re: [cip-dev] [PATCH 4.4 3/3] ALSA: control: use counting semaphore as write lock for ELEM_WRITE operation

[Ulrich Hecht]

> On 03/31/2023 5:51 PM CEST Alexander Grund <theflamefire89@gmail.com> wrote:
> Do I need to submit the updated patch as a new mail? If so how'd I do that, e.g. with git send-mail? 

Normally, even for small changes, you would re-format the whole series with "git format-patch --cover-letter --subject-prefix 'PATCH v2'" and send it all again with git send-mail, adding a note to the cover letter on what has changed in this revision.

CU
Uli

Re: [PATCH 4.4 3/3] ALSA: control: use counting semaphore as write lock for ELEM_WRITE operation

[Alexander Grund]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

> 
> Please add backports commit id.

I would have added this below the first line (after an empty line):
> 
> commit 5bbb1ab5bd0b01c4f0b19ae03fdfec487f839517 upstream.

Would that have been correct? Or would something else have been required in addition?

> 
> 
> 
> Normally, even for small changes, you would re-format the whole series
> with "git format-patch --cover-letter --subject-prefix 'PATCH v2'" and
> send it all again with git send-mail, adding a note to the cover letter on
> what has changed in this revision.
> 
> 

So it isn't a problem that this opens a new (series of) threads instead of replying to the mails requesting the changes, is it?

I use `git send-email` directly (I have set sendemail.annotate=true & sendemail.confirm=always), so my command would have looked like this: `git send-email --cover-letter --subject-prefix="PATCH v2" --suppress-cc=all  --to=cip-dev@lists.cip-project.org cip/linux-4.4.y-st..fix-alsa` which is basically how I created the current 4 mails.

I'm wondering if this is correct, especially as I used the prefix "PATCH 4.4" before to denote that it is meant for the 4.4.y branch. This doesn't seem to be required, so what should I use as the prefix instead for a (future) patch/patch series?

Anyway I noticed you already applied the commits fixing the mentioned issues before tagging, thanks!

I'd still appreciate a reply or a link to a page answering those question if one already exists, so the process can be smoother next time.
Thanks a lot!

[-- Attachment #2: Type: text/html, Size: 1632 bytes --]