[PATCH 0/5] ASoC: fsl_asrc/fsl_easrc: fix memory safety issues

[PATCH 0/5] ASoC: fsl_asrc/fsl_easrc: fix memory safety issues

[Stefan Ghetu]

This series fixes memory safety issues in the ASRC and EASRC drivers
that can be triggered by a user with access to their PCM or compress
devices:

1. fsl_asrc_m2m: the output FIFO drain can write past the end of the
   output buffer.
2. fsl_asrc: a repeated hw_params() leaves a stale asrc->pair[] entry
   that points to freed memory after close, and hw_free() after a
   failed hw_params() can release the pair of another stream.
3. fsl_asrc_dma: DMA channels are leaked on a repeated hw_params(), and
   released twice after a Back-End configuration error.
4. fsl_easrc: the same repeated hw_params() issue for EASRC contexts.
5. fsl_asrc_m2m: more than one task per compress stream leads to double
   DMA channel release and a stale pair entry.

Patches 2 and 4 make the pair/context release ownership-checked; patch
5 relies on them. Please apply in order.

Validated on i.MX8MP (EASRC) and i.MX8QM (ASRC).

Stefan Ghetu (5):
  ASoC: fsl_asrc_m2m: fix out-of-bounds write when draining the output
    FIFO
  ASoC: fsl_asrc: fix pair ownership on repeated or failed hw_params
  ASoC: fsl_asrc_dma: release DMA channels on repeated hw_params
  ASoC: fsl_easrc: fix context ownership on repeated hw_params
  ASoC: fsl_asrc_m2m: allow only one task at a time per stream

 sound/soc/fsl/fsl_asrc.c     | 22 ++++++++++++++++++++++
 sound/soc/fsl/fsl_asrc_dma.c | 27 ++++++++++++++++++---------
 sound/soc/fsl/fsl_asrc_m2m.c | 25 +++++++++++++++++--------
 sound/soc/fsl/fsl_easrc.c    | 10 ++++++++++
 4 files changed, 67 insertions(+), 17 deletions(-)


base-commit: b810335a0699d1810683796b0e3ffe8827ddfe6c
-- 
2.53.0

[PATCH 1/5] ASoC: fsl_asrc_m2m: fix out-of-bounds write when draining

[Stefan Ghetu]

asrc_read_last_fifo() checks the FIFO fill level, a number of frames,
against a length in bytes, and ignores the frames already drained by
previous iterations of its retry loop. With an output length close to
the 512 KiB buffer size, which userspace controls through the task input
size and formats, the drain writes past the end of the output buffer:
312 bytes for 10 channels of S24_LE with 8 frames left in the FIFO.
8-bit formats are also stored as 3 bytes per sample.

Check the space needed by all drained frames in bytes, and add a 1-byte
path for 8-bit formats.

Fixes: 24a01710f627 ("ASoC: fsl_asrc_m2m: Add memory to memory function")
Signed-off-by: Stefan Ghetu <stefan.ghetu@nxp.com>
---
 sound/soc/fsl/fsl_asrc_m2m.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc_m2m.c b/sound/soc/fsl/fsl_asrc_m2m.c
index 58101ce07c9f..cadfebb12e7c 100644
--- a/sound/soc/fsl/fsl_asrc_m2m.c
+++ b/sound/soc/fsl/fsl_asrc_m2m.c
@@ -53,21 +53,26 @@ static void asrc_read_last_fifo(struct fsl_asrc_pair *pair, void *dma_vaddr, u32
 {
 	struct fsl_asrc *asrc = pair->asrc;
 	enum asrc_pair_index index = pair->index;
-	u32 i, reg, size, t_size = 0, width;
+	u32 i, reg, size, t_size = 0, width, frame_bytes;
 	u32 *reg32 = NULL;
 	u16 *reg16 = NULL;
 	u8  *reg24 = NULL;
+	u8  *reg8 = NULL;
 
 	width = snd_pcm_format_physical_width(pair->sample_format[OUT]);
 	if (width == 32)
 		reg32 = dma_vaddr + *length;
 	else if (width == 16)
 		reg16 = dma_vaddr + *length;
+	else if (width == 8)
+		reg8 = dma_vaddr + *length;
 	else
 		reg24 = dma_vaddr + *length;
+
+	frame_bytes = pair->channels * width / 8;
 retry:
 	size = asrc->get_output_fifo_size(pair);
-	if (size + *length > ASRC_M2M_BUFFER_SIZE)
+	if (*length + (t_size + size) * frame_bytes > ASRC_M2M_BUFFER_SIZE)
 		goto end;
 
 	for (i = 0; i < size * pair->channels; i++) {
@@ -76,6 +81,8 @@ static void asrc_read_last_fifo(struct fsl_asrc_pair *pair, void *dma_vaddr, u32
 			*reg32++ = reg;
 		} else if (reg16) {
 			*reg16++ = (u16)reg;
+		} else if (reg8) {
+			*reg8++ = (u8)reg;
 		} else {
 			*reg24++ = (u8)reg;
 			*reg24++ = (u8)(reg >> 8);
@@ -89,12 +96,7 @@ static void asrc_read_last_fifo(struct fsl_asrc_pair *pair, void *dma_vaddr, u32
 		goto retry;
 end:
 	/* Update payload length */
-	if (reg32)
-		*length += t_size * pair->channels * 4;
-	else if (reg16)
-		*length += t_size * pair->channels * 2;
-	else
-		*length += t_size * pair->channels * 3;
+	*length += t_size * frame_bytes;
 }
 
 /* config dma channel */
-- 
2.53.0

[PATCH 2/5] ASoC: fsl_asrc: fix pair ownership on repeated or failed

[Stefan Ghetu]

hw_params() can be called again without hw_free() in between, and
hw_free() is also called after a failed hw_params().

fsl_asrc_request_pair() always takes a new pair, so a second hw_params()
leaves the stream in two asrc->pair[] entries. Only one is released, and
after close the other points to freed memory, which is dereferenced by
fsl_asrc_isr() and fsl_asrc_m2m_suspend(). After a failed hw_params(),
fsl_asrc_release_pair() releases a stale index that may belong to
another running stream.

Keep the owned pair and only update its channels when it is requested
again, and ignore a pair the stream does not own on release.

Fixes: 3117bb3109dc ("ASoC: fsl_asrc: Add ASRC ASoC CPU DAI and platform drivers")
Signed-off-by: Stefan Ghetu <stefan.ghetu@nxp.com>
---
 sound/soc/fsl/fsl_asrc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index faf348dd582d..5d97789b5c36 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -216,6 +216,8 @@ static void fsl_asrc_sel_proc(int inrate, int outrate,
  * It assigns pair by the order of A->C->B because allocation of pair B,
  * within range [ANCA, ANCA+ANCB-1], depends on the channels of pair A
  * while pair A and pair C are comparatively independent.
+ *
+ * If @pair already owns a pair, it is kept and only its channels are updated.
  */
 static int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
 {
@@ -226,6 +228,19 @@ static int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
 
 	guard(spinlock_irqsave)(&asrc->lock);
 
+	if (asrc->pair[pair->index] == pair) {
+		if (asrc->channel_avail + pair->channels < channels) {
+			dev_err(dev, "can't afford required channels: %d\n", channels);
+			return -EINVAL;
+		}
+
+		asrc->channel_avail += pair->channels;
+		asrc->channel_avail -= channels;
+		pair->channels = channels;
+
+		return 0;
+	}
+
 	for (i = ASRC_PAIR_A; i < ASRC_PAIR_MAX_NUM; i++) {
 		if (asrc->pair[i] != NULL)
 			continue;
@@ -257,12 +272,19 @@ static int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
  * @pair: pair to release
  *
  * It clears the resource from asrc and releases the occupied channels.
+ * Nothing is done if @pair does not own a pair.
  */
 static void fsl_asrc_release_pair(struct fsl_asrc_pair *pair)
 {
 	struct fsl_asrc *asrc = pair->asrc;
 	enum asrc_pair_index index = pair->index;
 
+	/* Only the owner clears its entry, so it cannot change after this check */
+	scoped_guard(spinlock_irqsave, &asrc->lock) {
+		if (asrc->pair[index] != pair)
+			return;
+	}
+
 	/* Make sure the pair is disabled */
 	regmap_update_bits(asrc->regmap, REG_ASRCTR,
 			   ASRCTR_ASRCEi_MASK(index), 0);
-- 
2.53.0

[PATCH 3/5] ASoC: fsl_asrc_dma: release DMA channels on repeated

[Stefan Ghetu]

fsl_asrc_dma_hw_params() requests new DMA channels on every call and
overwrites the previous ones, which are leaked when hw_params() is
called again without hw_free(). Its Back-End error path also releases a
channel without clearing the pointer, so the following hw_free()
releases it a second time.

Release the channels of a previous call first, and clear the pointer in
the error path.

Fixes: 3117bb3109dc ("ASoC: fsl_asrc: Add ASRC ASoC CPU DAI and platform drivers")
Fixes: 36124fb19f1a ("ASoC: fsl_asrc_dma: Fix dma_chan leak when config DMA channel failed")
Signed-off-by: Stefan Ghetu <stefan.ghetu@nxp.com>
---
 sound/soc/fsl/fsl_asrc_dma.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
index b417de6abebe..5ba4bdeff019 100644
--- a/sound/soc/fsl/fsl_asrc_dma.c
+++ b/sound/soc/fsl/fsl_asrc_dma.c
@@ -124,6 +124,19 @@ static int fsl_asrc_dma_trigger(struct snd_soc_component *component,
 	return 0;
 }
 
+static void fsl_asrc_dma_release_chans(struct fsl_asrc_pair *pair, u8 dir)
+{
+	if (pair->dma_chan[!dir])
+		dma_release_channel(pair->dma_chan[!dir]);
+
+	/* release dev_to_dev chan if we aren't reusing the Back-End one */
+	if (pair->dma_chan[dir] && pair->req_dma_chan)
+		dma_release_channel(pair->dma_chan[dir]);
+
+	pair->dma_chan[!dir] = NULL;
+	pair->dma_chan[dir] = NULL;
+}
+
 static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
 				  struct snd_pcm_substream *substream,
 				  struct snd_pcm_hw_params *params)
@@ -190,6 +203,9 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
 	dma_params_fe->addr = asrc->paddr + asrc->get_fifo_addr(!dir, index);
 	dma_params_fe->maxburst = dma_params_be->maxburst;
 
+	/* hw_params() may be called again without hw_free() in between */
+	fsl_asrc_dma_release_chans(pair, dir);
+
 	tmp_chan = asrc->get_dma_channel(pair, !dir);
 	if (IS_ERR(tmp_chan)) {
 		dev_err(dev, "failed to request DMA channel\n");
@@ -342,6 +358,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
 		dev_err(dev, "failed to config DMA channel for Back-End\n");
 		if (pair->req_dma_chan)
 			dma_release_channel(pair->dma_chan[dir]);
+		pair->dma_chan[dir] = NULL;
 		return ret;
 	}
 
@@ -356,15 +373,7 @@ static int fsl_asrc_dma_hw_free(struct snd_soc_component *component,
 	struct fsl_asrc_pair *pair = runtime->private_data;
 	u8 dir = tx ? OUT : IN;
 
-	if (pair->dma_chan[!dir])
-		dma_release_channel(pair->dma_chan[!dir]);
-
-	/* release dev_to_dev chan if we aren't reusing the Back-End one */
-	if (pair->dma_chan[dir] && pair->req_dma_chan)
-		dma_release_channel(pair->dma_chan[dir]);
-
-	pair->dma_chan[!dir] = NULL;
-	pair->dma_chan[dir] = NULL;
+	fsl_asrc_dma_release_chans(pair, dir);
 
 	return 0;
 }
-- 
2.53.0

[PATCH 4/5] ASoC: fsl_easrc: fix context ownership on repeated

[Stefan Ghetu]

hw_params() can be called again without hw_free() in between.
fsl_easrc_hw_params() then requests a second context while the stream
still owns the first one, which is never released. After close, that
easrc->pair[] entry points to freed memory and is dereferenced by
fsl_easrc_runtime_resume() on the next open.

The EASRC slots depend on the number of channels, so release the owned
context before requesting a new one, and make
fsl_easrc_release_context() ignore a context it does not own.

Fixes: 955ac624058f ("ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers")
Signed-off-by: Stefan Ghetu <stefan.ghetu@nxp.com>
---
 sound/soc/fsl/fsl_easrc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
index 5d715bc07357..ae0fbf6c316b 100644
--- a/sound/soc/fsl/fsl_easrc.c
+++ b/sound/soc/fsl/fsl_easrc.c
@@ -1347,6 +1347,10 @@ static void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
 
 	guard(spinlock_irqsave)(&easrc->lock);
 
+	/* Nothing to release if the context is not owned by @ctx */
+	if (easrc->pair[ctx->index] != ctx)
+		return;
+
 	fsl_easrc_release_slot(easrc, ctx->index);
 
 	easrc->channel_avail += ctx->channels;
@@ -1500,6 +1504,12 @@ static int fsl_easrc_hw_params(struct snd_pcm_substream *substream,
 	snd_pcm_format_t format = params_format(params);
 	int ret;
 
+	/* hw_params() may be called again without hw_free() in between */
+	if (ctx_priv->ctx_streams & BIT(substream->stream)) {
+		ctx_priv->ctx_streams &= ~BIT(substream->stream);
+		fsl_easrc_release_context(ctx);
+	}
+
 	ret = fsl_easrc_request_context(channels, ctx);
 	if (ret) {
 		dev_err(dev, "failed to request context\n");
-- 
2.53.0

[PATCH 5/5] ASoC: fsl_asrc_m2m: allow only one task at a time per

[Stefan Ghetu]

All tasks of a stream share one pair and one set of DMA channels, and
get_caps() advertises a single fragment, but the compress core allows
up to 64 tasks. A second task requests the pair again and overwrites the
DMA channels. Freeing both tasks then releases the same channels twice
and leaves a stale asrc->pair[] entry after close.

Return -EBUSY from task_create() while a task exists, and clear the DMA
channel pointers once they are released.

Fixes: 24a01710f627 ("ASoC: fsl_asrc_m2m: Add memory to memory function")
Signed-off-by: Stefan Ghetu <stefan.ghetu@nxp.com>
---
 sound/soc/fsl/fsl_asrc_m2m.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/soc/fsl/fsl_asrc_m2m.c b/sound/soc/fsl/fsl_asrc_m2m.c
index cadfebb12e7c..2e91073f14f9 100644
--- a/sound/soc/fsl/fsl_asrc_m2m.c
+++ b/sound/soc/fsl/fsl_asrc_m2m.c
@@ -471,6 +471,10 @@ static int fsl_asrc_m2m_comp_task_create(struct snd_compr_stream *stream,
 	struct dma_chan *tmp_chan;
 	int ret;
 
+	/* All tasks share the pair and DMA channels: one task at a time */
+	if (pair->dma_chan[IN])
+		return -EBUSY;
+
 	exp_info_in.ops = &fsl_asrc_m2m_dma_buf_ops;
 	exp_info_in.size = ASRC_M2M_BUFFER_SIZE;
 	exp_info_in.flags = O_RDWR;
@@ -527,6 +531,7 @@ static int fsl_asrc_m2m_comp_task_create(struct snd_compr_stream *stream,
 
 err_dma_channel_out:
 	dma_release_channel(pair->dma_chan[IN]);
+	pair->dma_chan[IN] = NULL;
 err_dma_channel_in:
 	if (asrc->m2m_unprepare)
 		asrc->m2m_unprepare(pair);
@@ -571,6 +576,8 @@ static int fsl_asrc_m2m_comp_task_free(struct snd_compr_stream *stream,
 		dma_release_channel(pair->dma_chan[IN]);
 	if (pair->dma_chan[OUT])
 		dma_release_channel(pair->dma_chan[OUT]);
+	pair->dma_chan[IN] = NULL;
+	pair->dma_chan[OUT] = NULL;
 
 	return 0;
 }
-- 
2.53.0