Re: [PATCH v1 3/5] ASoC: qcom: qdsp6: Zero-initialize AudioReach module config

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Memory leak of audio_buffer array when q6apm_dai_prepare fails.
- [High] Concurrent Use-After-Free in gpr_do_rx_callback() due to lack of synchronization with port teardown.
--

--- Patch [3]: [PATCH v1 3/5] ASoC: qcom: qdsp6: Zero-initialize AudioReach module config ---
commit f7d8f4b6351377745201f54e092ff89f2f6f845e
Author: Prasad Kumpatla <[email protected]>

ASoC: qcom: qdsp6: Zero-initialize AudioReach module config
    
This commit updates q6apm_dai_prepare() and q6apm_dai_compr_set_params() to
zero-initialize the AudioReach module config structure. This ensures that
uninitialized stack data doesn't override topology defaults sent to the DSP.

Link: https://sashiko.dev/#/patchset/[email protected]

> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
> index bf1f872a09f45..033197bdd8705 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
> @@ -216,7 +216,7 @@ static int q6apm_dai_prepare(struct snd_soc_component *component,
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct q6apm_dai_rtd *prtd = runtime->private_data;
> -	struct audioreach_module_config cfg;
> +	struct audioreach_module_config cfg = {};

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the changes
in q6apm_dai_prepare(), I noticed a potential memory leak on the error path.

If q6apm_alloc_fragments() succeeds, but a subsequent command like
q6apm_graph_media_format_pcm() fails:

sound/soc/qcom/qdsp6/q6apm-dai.c:q6apm_dai_prepare() {
    ...
    ret = q6apm_alloc_fragments(...);
    if (ret < 0) {
        ...
    }

    ret = q6apm_graph_media_format_pcm(prtd->graph, &cfg);
    if (ret < 0) {
        dev_err(dev, "%s: CMD Format block failed\n", __func__);
        return ret;
    }
    ...
    prtd->state = Q6APM_STREAM_RUNNING;
}

The function will return without setting prtd->state to Q6APM_STREAM_RUNNING.
Later, when the stream is closed, q6apm_dai_close() skips freeing the
fragments because prtd->state was never set:

sound/soc/qcom/qdsp6/q6apm-dai.c:q6apm_dai_close() {
    ...
    if (prtd->state) {
        q6apm_graph_stop(prtd->graph);
        q6apm_free_fragments(prtd->graph, substream->stream);
    }

    q6apm_graph_close(prtd->graph);
}

Does this leave the allocated audio buffer arrays permanently leaked?

[Severity: High]
This is another pre-existing issue I noticed related to closing the stream.
Can an unprivileged local user trigger a use-after-free by repeatedly
opening and closing a stream while the DSP generates events?

When userspace closes the stream, q6apm_graph_close() eventually invokes
gpr_free_port(), which frees the port immediately:

drivers/soc/qcom/apr.c:gpr_free_port() {
    spin_lock_irqsave(&gpr->svcs_lock, flags);
    idr_remove(&gpr->svcs_idr, port->id);
    spin_unlock_irqrestore(&gpr->svcs_lock, flags);

    kfree(port);
}

However, gpr_do_rx_callback() processes DSP IPC messages in a workqueue,
dropping the lock before dereferencing the callback:

drivers/soc/qcom/apr.c:gpr_do_rx_callback() {
    ...
    spin_lock_irqsave(&gpr->svcs_lock, flags);
    svc = idr_find(&gpr->svcs_idr, hdr->dest_port);
    spin_unlock_irqrestore(&gpr->svcs_lock, flags);

    if (!svc) { ... }

    if (svc->callback)
        svc->callback(&resp, svc->priv, 0);
}

If the workqueue is executing concurrently, could it dereference the
already-freed port memory to invoke the callback since the workqueue
isn't flushed prior to freeing the port?

>  	struct device *dev = component->dev;
>  	struct q6apm_dai_data *pdata;
>  	int ret;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3