ASoC: qdsp6: q6asm-dai: Off by one in of_q6asm_parse_dai_data()

The q6asm_fe_dais[] array has MAX_SESSIONS (8) elements so the > comparison should be >= or we access one element beyond the end of the array.

---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5b986b74dd36..9d738b4c1e05 100644
@@ -874,7 +874,7 @@ static int of_q6asm_parse_dai_data(struct device *dev,
 
 	for_each_child_of_node(dev->of_node, node) {
 		ret = of_property_read_u32(node, "reg", &id);
-		if (ret || id > MAX_SESSIONS || id < 0) {
+		if (ret || id >= MAX_SESSIONS || id < 0) {
 			dev_err(dev, "valid dai id not found:%d\n", ret);
 			continue;
 		}

2.17.1

The q6asm_audio_client_alloc() doesn't return NULL, it returns error pointers.

---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 9d738b4c1e05..3407e51b8861 100644
@@ -570,10 +570,11 @@ static int q6asm_dai_compr_open(struct snd_compr_stream *stream)
 	prtd->audio_client = q6asm_audio_client_alloc(dev,
 					(q6asm_cb)compress_event_handler,
 					prtd, stream_id, LEGACY_PCM_MODE);
-	if (!prtd->audio_client) {
+	if (IS_ERR(prtd->audio_client)) {
 		dev_err(dev, "Could not allocate memory\n");
+		ret = PTR_ERR(prtd->audio_client);
 		kfree(prtd);
-		return -ENOMEM;
+		return ret;
 	}
 
 	size = COMPR_PLAYBACK_MAX_FRAGMENT_SIZE *

2.17.1

Thanks for the patch!

The patch

ASoC: qdsp6: q6asm-dai: Fix a NULL vs IS_ERR() bug

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

From 168f6a0b0d2a9c662a6b5d839385dcce70e8c518 Mon Sep 17 00:00:00 2001
From: Dan Carpenter <[email protected]>
Date: Fri, 21 Dec 2018 12:05:16 +0300
Subject: [PATCH] ASoC: qdsp6: q6asm-dai: Fix a NULL vs IS_ERR() bug

The q6asm_audio_client_alloc() doesn't return NULL, it returns error pointers.

---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 9d738b4c1e05..3407e51b8861 100644
@@ -570,10 +570,11 @@ static int q6asm_dai_compr_open(struct snd_compr_stream *stream)
 	prtd->audio_client = q6asm_audio_client_alloc(dev,
 					(q6asm_cb)compress_event_handler,
 					prtd, stream_id, LEGACY_PCM_MODE);
-	if (!prtd->audio_client) {
+	if (IS_ERR(prtd->audio_client)) {
 		dev_err(dev, "Could not allocate memory\n");
+		ret = PTR_ERR(prtd->audio_client);
 		kfree(prtd);
-		return -ENOMEM;
+		return ret;
 	}
 
 	size = COMPR_PLAYBACK_MAX_FRAGMENT_SIZE *

2.20.1

We can't return directly if snd_dma_alloc_pages() fails; we first need to free prtd->audio_client and prtd.

---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

index 3407e51b8861..548eb4fa2da6 100644
@@ -573,8 +573,7 @@ static int q6asm_dai_compr_open(struct snd_compr_stream *stream)
 	if (IS_ERR(prtd->audio_client)) {
 		dev_err(dev, "Could not allocate memory\n");
 		ret = PTR_ERR(prtd->audio_client);
-		kfree(prtd);
-		return ret;
+		goto free_prtd;
 	}
 
 	size = COMPR_PLAYBACK_MAX_FRAGMENT_SIZE *

@@ -583,7 +582,7 @@ static int q6asm_dai_compr_open(struct snd_compr_stream *stream)
 				  &prtd->dma_buffer);
 	if (ret) {
 		dev_err(dev, "Cannot allocate buffer(s)\n");
-		return ret;
+		goto free_client;
 	}
 
 	if (pdata->sid < 0)

@@ -596,6 +595,13 @@ static int q6asm_dai_compr_open(struct snd_compr_stream *stream)
 	runtime->private_data = prtd;
 
 	return 0;
+
+free_client:
+	q6asm_audio_client_free(prtd->audio_client);
+free_prtd:
+	kfree(prtd);
+
+	return ret;
 }
 
 static int q6asm_dai_compr_free(struct snd_compr_stream *stream)

2.17.1

Thanks for the patch! I was due to send something similar!

The patch

ASoC: qdsp6: q6asm-dai: Fix a small memory leak

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

From 686a6dace3bd783ef9f747e9c2cff68038be0e3a Mon Sep 17 00:00:00 2001
From: Dan Carpenter <[email protected]>
Date: Fri, 21 Dec 2018 12:06:10 +0300
Subject: [PATCH] ASoC: qdsp6: q6asm-dai: Fix a small memory leak

We can't return directly if snd_dma_alloc_pages() fails; we first need to free prtd->audio_client and prtd.

---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

index 3407e51b8861..548eb4fa2da6 100644
@@ -573,8 +573,7 @@ static int q6asm_dai_compr_open(struct snd_compr_stream *stream)
 	if (IS_ERR(prtd->audio_client)) {
 		dev_err(dev, "Could not allocate memory\n");
 		ret = PTR_ERR(prtd->audio_client);
-		kfree(prtd);
-		return ret;
+		goto free_prtd;
 	}
 
 	size = COMPR_PLAYBACK_MAX_FRAGMENT_SIZE *

@@ -583,7 +582,7 @@ static int q6asm_dai_compr_open(struct snd_compr_stream *stream)
 				  &prtd->dma_buffer);
 	if (ret) {
 		dev_err(dev, "Cannot allocate buffer(s)\n");
-		return ret;
+		goto free_client;
 	}
 
 	if (pdata->sid < 0)

@@ -596,6 +595,13 @@ static int q6asm_dai_compr_open(struct snd_compr_stream *stream)
 	runtime->private_data = prtd;
 
 	return 0;
+
+free_client:
+	q6asm_audio_client_free(prtd->audio_client);
+free_prtd:
+	kfree(prtd);
+
+	return ret;
 }
 
 static int q6asm_dai_compr_free(struct snd_compr_stream *stream)

2.20.1

The problem is seen in the q6asm_dai_compr_set_params() function:

	ret = q6asm_map_memory_regions(dir, prtd->audio_client, prtd->phys,
				       (prtd->pcm_size / prtd->periods),
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
				       prtd->periods);

In this code prtd->pcm_size is the buffer_size and prtd->periods comes from params->buffer.fragments. If we allow the number of fragments to be zero then it results in a divide by zero bug. One possible fix would be to use prtd->pcm_count directly instead of using the division to re-calculate it. But I decided that it doesn't really make sense to allow zero fragments.

---
I am not very sure of this patch. Please review it extra carefully because it is an API change.

 sound/core/compress_offload.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index a5b09e75e787..f7d2b373da0a 100644
@@ -541,7 +541,8 @@ static int snd_compress_check_input(struct snd_compr_params *params)
 {
 	/* first let's check the buffer parameter's */
 	if (params->buffer.fragment_size = 0 ||
-	    params->buffer.fragments > INT_MAX / params->buffer.fragment_size)
+	    params->buffer.fragments > INT_MAX / params->buffer.fragment_size ||
+	    params->buffer.fragments = 0)
 		return -EINVAL;
 
 	/* now codec parameters */

2.17.1

The patch

ALSA: compress: prevent potential divide by zero bugs

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

From e9d960271d89c0cabf094123e2ddc94705de2a31 Mon Sep 17 00:00:00 2001
From: Dan Carpenter <[email protected]>
Date: Fri, 21 Dec 2018 12:06:58 +0300
Subject: [PATCH] ALSA: compress: prevent potential divide by zero bugs

The problem is seen in the q6asm_dai_compr_set_params() function:

	ret = q6asm_map_memory_regions(dir, prtd->audio_client, prtd->phys,
				       (prtd->pcm_size / prtd->periods),
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
				       prtd->periods);

In this code prtd->pcm_size is the buffer_size and prtd->periods comes from params->buffer.fragments. If we allow the number of fragments to be zero then it results in a divide by zero bug. One possible fix would be to use prtd->pcm_count directly instead of using the division to re-calculate it. But I decided that it doesn't really make sense to allow zero fragments.

---
 sound/core/compress_offload.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index a5b09e75e787..f7d2b373da0a 100644
@@ -541,7 +541,8 @@ static int snd_compress_check_input(struct snd_compr_params *params)
 {
 	/* first let's check the buffer parameter's */
 	if (params->buffer.fragment_size = 0 ||
-	    params->buffer.fragments > INT_MAX / params->buffer.fragment_size)
+	    params->buffer.fragments > INT_MAX / params->buffer.fragment_size ||
+	    params->buffer.fragments = 0)
 		return -EINVAL;
 
 	/* now codec parameters */

2.20.1

Thanks for the patch!

The patch

ASoC: qdsp6: q6asm-dai: Off by one in of_q6asm_parse_dai_data()

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

From 6f10aff97f5d2c9a26a4deff1482f845841b7307 Mon Sep 17 00:00:00 2001
From: Dan Carpenter <[email protected]>
Date: Fri, 21 Dec 2018 12:04:42 +0300
Subject: [PATCH] ASoC: qdsp6: q6asm-dai: Off by one in
of_q6asm_parse_dai_data()

The q6asm_fe_dais[] array has MAX_SESSIONS (8) elements so the > comparison should be >= or we access one element beyond the end of the array.

---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5b986b74dd36..9d738b4c1e05 100644
@@ -874,7 +874,7 @@ static int of_q6asm_parse_dai_data(struct device *dev,
 
 	for_each_child_of_node(dev->of_node, node) {
 		ret = of_property_read_u32(node, "reg", &id);
-		if (ret || id > MAX_SESSIONS || id < 0) {
+		if (ret || id >= MAX_SESSIONS || id < 0) {
 			dev_err(dev, "valid dai id not found:%d\n", ret);
 			continue;
 		}

2.20.1