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

[Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>]

On 8/9/2026 12:16 AM, sashiko-bot@kernel.org wrote:
> 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 <prasad.kumpatla@oss.qualcomm.com>
>
> 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/20260804070307.117119-1-prasad.kumpatla@oss.qualcomm.com
>
>> 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?

Hi Sashiko,

Thanks for Reviewing.

These issues are pre-existing and not introduced by this change,
which only fixes the initialization of audioreach_module_config.
I'd prefer to keep this patch focused and handle the error-path
and teardown concerns separately.

Thanks,
Prasad
>
> 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;
> [ ... ]
>