[PATCH 0/5] ALSA: Fix yet more bugs reported by Sashiko

[PATCH 0/5] ALSA: Fix yet more bugs reported by Sashiko

[Takashi Iwai]

Hi,

here is yet another series of bug fixes for low-handing fruis that
have been reported by Sashiko recently as the existing bugs.


Takashi

===

Takashi Iwai (5):
  ALSA: seq: Fix missing direction and ump_group handling in 32bit
    compat ioctl
  ALSA: pcmtest: Fix leak at probe error
  ALSA: aloop: Avoid a bad mixure of guard() and goto
  ALSA: core: Fix leaks at snd_card_init() error paths
  ALSA: seq: Don't lose partial read failure

 sound/core/init.c              |  14 +--
 sound/core/seq/seq_clientmgr.c |   6 +-
 sound/core/seq/seq_compat.c    |  12 ++-
 sound/drivers/aloop.c          | 154 ++++++++++++++++-----------------
 sound/drivers/pcmtest.c        |   3 +-
 5 files changed, 98 insertions(+), 91 deletions(-)

-- 
2.55.0

[PATCH 1/5] ALSA: seq: Fix missing direction and ump_group handling in 32bit compat ioctl

[Takashi Iwai]

I forgot to cover the two new fields, direction and ump_group, in
struct snd_seq_port_info for the 32bit compat ioctls of
SNDRV_SEQ_IOCTL_GET_PORT_INFO & co, which ended up with the garbage
copies in those fields.  Add the handling of those two fields in the
compat layer.

Fixes: ff166a9d19fa ("ALSA: seq: Add port direction to snd_seq_port_info")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/seq/seq_compat.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/sound/core/seq/seq_compat.c b/sound/core/seq/seq_compat.c
index 80110501da6f..9ea205e1f2dc 100644
--- a/sound/core/seq/seq_compat.c
+++ b/sound/core/seq/seq_compat.c
@@ -25,7 +25,9 @@ struct snd_seq_port_info32 {
 	u32 kernel;			/* reserved for kernel use (must be NULL) */
 	u32 flags;		/* misc. conditioning */
 	unsigned char time_queue;	/* queue # for timestamping */
-	char reserved[59];		/* for future use */
+	unsigned char direction;	/* port usage direction (r/w/bidir) */
+	unsigned char ump_group;	/* 0 = UMP EP (no conversion), 1-16 = UMP group number */
+	char reserved[57];		/* for future use */
 };
 
 static int snd_seq_call_port_info_ioctl(struct snd_seq_client *client, unsigned int cmd,
@@ -40,7 +42,9 @@ static int snd_seq_call_port_info_ioctl(struct snd_seq_client *client, unsigned
 
 	if (copy_from_user(data, data32, sizeof(*data32)) ||
 	    get_user(data->flags, &data32->flags) ||
-	    get_user(data->time_queue, &data32->time_queue))
+	    get_user(data->time_queue, &data32->time_queue) ||
+	    get_user(data->direction, &data32->direction) ||
+	    get_user(data->ump_group, &data32->ump_group))
 		return -EFAULT;
 	data->kernel = NULL;
 
@@ -52,7 +56,9 @@ static int snd_seq_call_port_info_ioctl(struct snd_seq_client *client, unsigned
 
 	if (copy_to_user(data32, data, sizeof(*data32)) ||
 	    put_user(data->flags, &data32->flags) ||
-	    put_user(data->time_queue, &data32->time_queue))
+	    put_user(data->time_queue, &data32->time_queue) ||
+	    put_user(data->direction, &data32->direction) ||
+	    put_user(data->ump_group, &data32->ump_group))
 		return -EFAULT;
 
 	return err;
-- 
2.55.0

[PATCH 2/5] ALSA: pcmtest: Fix leak at probe error

[Takashi Iwai]

When an error happens at probe of pcmtest driver before
snd_card_register() succeeds, we have to release the card explicitly,
but it's not done, leading to a potential memory leak.

Use the new auto-clean mechanism to handle the error case at probe.

Fixes: 315a3d57c64c ("ALSA: Implement the new Virtual PCM Test Driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/drivers/pcmtest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/drivers/pcmtest.c b/sound/drivers/pcmtest.c
index fea9580593e6..4733f7c7729f 100644
--- a/sound/drivers/pcmtest.c
+++ b/sound/drivers/pcmtest.c
@@ -596,7 +596,7 @@ static int snd_pcmtst_create(struct snd_card *card, struct platform_device *pdev
 
 static int pcmtst_probe(struct platform_device *pdev)
 {
-	struct snd_card *card;
+	struct snd_card *card __free(snd_card_free) = NULL;
 	struct pcmtst *pcmtst;
 	int err;
 
@@ -620,6 +620,7 @@ static int pcmtst_probe(struct platform_device *pdev)
 		return err;
 
 	platform_set_drvdata(pdev, pcmtst);
+	card = NULL; /* probe succeeded, don't release as error */
 
 	return 0;
 }
-- 
2.55.0

[PATCH 3/5] ALSA: aloop: Avoid a bad mixure of guard() and goto

[Takashi Iwai]

The rewrite of aloop driver code using guard() leaded to a mixture of
guard() and some goto; although it currently works, it isn't really a
good match.

For avoiding the bad match, rewrite with scoped_guard(), so that we
can replace goto with break gracefully.

Fixes: ebd9b6c91d4e ("ALSA: aloop: Use guard() for mutex locks")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/drivers/aloop.c | 154 +++++++++++++++++++++---------------------
 1 file changed, 77 insertions(+), 77 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 5f0e2dc96769..f9659f436bfc 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -1384,94 +1384,94 @@ static int loopback_open(struct snd_pcm_substream *substream)
 	int err = 0;
 	int dev = get_cable_index(substream);
 
-	guard(mutex)(&loopback->cable_lock);
-	dpcm = kzalloc_obj(*dpcm);
-	if (!dpcm)
-		return -ENOMEM;
-	dpcm->loopback = loopback;
-	dpcm->substream = substream;
+	scoped_guard(mutex, &loopback->cable_lock) {
+		dpcm = kzalloc_obj(*dpcm);
+		if (!dpcm)
+			return -ENOMEM;
+		dpcm->loopback = loopback;
+		dpcm->substream = substream;
 
-	cable = loopback->cables[substream->number][dev];
-	if (!cable) {
-		cable = kzalloc_obj(*cable);
+		cable = loopback->cables[substream->number][dev];
 		if (!cable) {
-			err = -ENOMEM;
-			goto unlock;
-		}
-		spin_lock_init(&cable->lock);
-		snd_refcount_init(&cable->stop_count);
-		cable->hw = loopback_pcm_hardware;
+			cable = kzalloc_obj(*cable);
+			if (!cable) {
+				err = -ENOMEM;
+				break;
+			}
+			spin_lock_init(&cable->lock);
+			snd_refcount_init(&cable->stop_count);
+			cable->hw = loopback_pcm_hardware;
 #ifdef CONFIG_HIGH_RES_TIMERS
-		if (loopback->timer_source && !strcmp(loopback->timer_source, "hrtimer"))
-			cable->ops = &loopback_hrtimer_ops;
-		else
+			if (loopback->timer_source && !strcmp(loopback->timer_source, "hrtimer"))
+				cable->ops = &loopback_hrtimer_ops;
+			else
 #endif
-		if (loopback->timer_source && loopback->timer_source[0])
-			cable->ops = &loopback_snd_timer_ops;
-		else
-			cable->ops = &loopback_jiffies_timer_ops;
-		loopback->cables[substream->number][dev] = cable;
-	}
-	dpcm->cable = cable;
-	runtime->private_data = dpcm;
+				if (loopback->timer_source && loopback->timer_source[0])
+					cable->ops = &loopback_snd_timer_ops;
+				else
+					cable->ops = &loopback_jiffies_timer_ops;
+			loopback->cables[substream->number][dev] = cable;
+		}
+		dpcm->cable = cable;
+		runtime->private_data = dpcm;
 
-	if (cable->ops->open) {
-		err = cable->ops->open(dpcm);
-		if (err < 0)
-			goto unlock;
-	}
+		if (cable->ops->open) {
+			err = cable->ops->open(dpcm);
+			if (err < 0)
+				break;
+		}
 
-	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
+		snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
 
-	/* use dynamic rules based on actual runtime->hw values */
-	/* note that the default rules created in the PCM midlevel code */
-	/* are cached -> they do not reflect the actual state */
-	err = snd_pcm_hw_rule_add(runtime, 0,
-				  SNDRV_PCM_HW_PARAM_FORMAT,
-				  rule_format, dpcm,
-				  SNDRV_PCM_HW_PARAM_FORMAT, -1);
-	if (err < 0)
-		goto unlock;
-	err = snd_pcm_hw_rule_add(runtime, 0,
-				  SNDRV_PCM_HW_PARAM_RATE,
-				  rule_rate, dpcm,
-				  SNDRV_PCM_HW_PARAM_RATE, -1);
-	if (err < 0)
-		goto unlock;
-	err = snd_pcm_hw_rule_add(runtime, 0,
-				  SNDRV_PCM_HW_PARAM_CHANNELS,
-				  rule_channels, dpcm,
-				  SNDRV_PCM_HW_PARAM_CHANNELS, -1);
-	if (err < 0)
-		goto unlock;
-
-	/* In case of sound timer the period time of both devices of the same
-	 * loop has to be the same.
-	 * This rule only takes effect if a sound timer was chosen
-	 */
-	if (cable->snd_timer.instance) {
+		/* use dynamic rules based on actual runtime->hw values */
+		/* note that the default rules created in the PCM midlevel code */
+		/* are cached -> they do not reflect the actual state */
 		err = snd_pcm_hw_rule_add(runtime, 0,
-					  SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
-					  rule_period_bytes, dpcm,
-					  SNDRV_PCM_HW_PARAM_PERIOD_BYTES, -1);
+					  SNDRV_PCM_HW_PARAM_FORMAT,
+					  rule_format, dpcm,
+					  SNDRV_PCM_HW_PARAM_FORMAT, -1);
 		if (err < 0)
-			goto unlock;
+			break;
+		err = snd_pcm_hw_rule_add(runtime, 0,
+					  SNDRV_PCM_HW_PARAM_RATE,
+					  rule_rate, dpcm,
+					  SNDRV_PCM_HW_PARAM_RATE, -1);
+		if (err < 0)
+			break;
+		err = snd_pcm_hw_rule_add(runtime, 0,
+					  SNDRV_PCM_HW_PARAM_CHANNELS,
+					  rule_channels, dpcm,
+					  SNDRV_PCM_HW_PARAM_CHANNELS, -1);
+		if (err < 0)
+			break;
+
+		/* In case of sound timer the period time of both devices of the same
+		 * loop has to be the same.
+		 * This rule only takes effect if a sound timer was chosen
+		 */
+		if (cable->snd_timer.instance) {
+			err = snd_pcm_hw_rule_add(runtime, 0,
+						  SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
+						  rule_period_bytes, dpcm,
+						  SNDRV_PCM_HW_PARAM_PERIOD_BYTES, -1);
+			if (err < 0)
+				break;
+		}
+
+		/* loopback_runtime_free() has not to be called if kfree(dpcm) was
+		 * already called here. Otherwise it will end up with a double free.
+		 */
+		runtime->private_free = loopback_runtime_free;
+		if (get_notify(dpcm))
+			runtime->hw = loopback_pcm_hardware;
+		else
+			runtime->hw = cable->hw;
+
+		scoped_guard(spinlock_irq, &cable->lock) {
+			cable->streams[substream->stream] = dpcm;
+		}
 	}
 
-	/* loopback_runtime_free() has not to be called if kfree(dpcm) was
-	 * already called here. Otherwise it will end up with a double free.
-	 */
-	runtime->private_free = loopback_runtime_free;
-	if (get_notify(dpcm))
-		runtime->hw = loopback_pcm_hardware;
-	else
-		runtime->hw = cable->hw;
-
-	scoped_guard(spinlock_irq, &cable->lock) {
-		cable->streams[substream->stream] = dpcm;
-	}
-
- unlock:
 	if (err < 0) {
 		free_cable(substream);
 		kfree(dpcm);
-- 
2.55.0

[PATCH 4/5] ALSA: core: Fix leaks at snd_card_init() error paths

[Takashi Iwai]

When the snd_card object is initialized internally in snd_card_init(),
the error path of card->value_buf allocation returns straightly as an
error without invoking the destructor, which would leak resources.

Also, the other error paths in snd_card_init() release the card object
only via put_device(), and this misses the cleanups that are done in
snd_card_disconnect(); namely, the reserved slot bit in snd_cards_lock
is never cleared, so the card index remains occupied, and the debugfs
directory created for the card is left over.

Fix them by calling snd_card_free() at the error path, which performs
both the disconnection and the release properly.  As snd_card_free()
calls snd_device_free_all() internally, the explicit call is dropped,
and the error labels are unified.

Fixes: 84446536f63d ("ALSA: control: Verify put() result when in debug mode")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/init.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sound/core/init.c b/sound/core/init.c
index dbe2acfa59fe..9deb5e920d0d 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -334,7 +334,7 @@ static int snd_card_init(struct snd_card *card, struct device *parent,
 	err = snd_info_card_create(card);
 	if (err < 0) {
 		dev_err(parent, "unable to create card info\n");
-		goto __error_ctl;
+		goto __error;
 	}
 
 #ifdef CONFIG_SND_DEBUG
@@ -343,16 +343,16 @@ static int snd_card_init(struct snd_card *card, struct device *parent,
 #endif
 #ifdef CONFIG_SND_CTL_DEBUG
 	card->value_buf = kmalloc_obj(*card->value_buf);
-	if (!card->value_buf)
-		return -ENOMEM;
+	if (!card->value_buf) {
+		err = -ENOMEM;
+		goto __error;
+	}
 #endif
 	return 0;
 
-      __error_ctl:
-	snd_device_free_all(card);
       __error:
-	put_device(&card->card_dev);
-  	return err;
+	snd_card_free(card);
+	return err;
 }
 
 /**
-- 
2.55.0

[PATCH 5/5] ALSA: seq: Don't lose partial read failure

[Takashi Iwai]

snd_seq_read() returns an error even if one or more events have been
successfully read before the error (except for -EAGAIN case), but this
rather doesn't follow the POSIX standard that wants the processed
bytes.  Change the behavior to return the processed bytes when it's
positive, instead.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/seq/seq_clientmgr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
index ba5619d0b0b0..0711a9e23b5f 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -480,11 +480,11 @@ static ssize_t snd_seq_read(struct file *file, char __user *buf, size_t count,
 	if (err < 0) {
 		if (cell)
 			snd_seq_fifo_cell_putback(fifo, cell);
-		if (err == -EAGAIN && result > 0)
-			err = 0;
 	}
 
-	return (err < 0) ? err : result;
+	if (result > 0)
+		return result;
+	return err < 0 ? err : 0;
 }
 
 
-- 
2.55.0