Re: [PATCH v7 1/7] ASoC: qcom: qdsp6: add topology-driven Audio IF support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Uninitialized stack memory is used to configure the DSP, which can lead to unpredictable behavior or DSP crashes.
--

--- Patch [1]: [PATCH v7 1/7] ASoC: qcom: qdsp6: add topology-driven Audio IF support ---
commit 0fae44a436543afc97061ba27a78d9bfd55821bf
Author: Prasad Kumpatla <[email protected]>

ASoC: qcom: qdsp6: add topology-driven Audio IF support

Add topology parsing and media-format programming for Audio IF source and
sink modules. This introduces new Audio IF module IDs, required topology
tokens, and the media-format path to send interface configuration to the DSP.

> diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
> index 0cc840aca69dd..cce0ad31ff0c0 100644
> --- a/sound/soc/qcom/qdsp6/audioreach.c
> +++ b/sound/soc/qcom/qdsp6/audioreach.c
[ ... ]
> @@ -1052,6 +1066,81 @@ static int audioreach_i2s_set_media_format(struct q6apm_graph *graph,
>  	return q6apm_send_cmd_sync(graph->apm, pkt, 0);
>  }
>  
> +static int audioreach_audio_if_set_media_format(struct q6apm_graph *graph,
> +						const struct audioreach_module *module,
> +						const struct audioreach_module_config *cfg)
> +{
> +	struct apm_module_hw_ep_frame_duration_cfg *fd_cfg;
> +	struct apm_module_param_data *param_data;
> +	struct apm_audio_if_module_intf_cfg *intf_cfg;
> +	struct apm_module_hw_ep_mf_cfg *hw_cfg;
> +	int ic_sz = APM_AUDIO_IF_INTF_CFG_PSIZE;
> +	int ep_sz = APM_HW_EP_CFG_PSIZE;
> +	int fd_sz = APM_HW_EP_FRAME_DURATION_PSIZE;
> +	int size = ic_sz + ep_sz + fd_sz;
> +	u32 slot_mask = cfg->slot_mask ? cfg->slot_mask : module->slot_mask;
> +	u16 nslots_per_frame = cfg->nslots_per_frame ?
> +				 (u16)cfg->nslots_per_frame : module->nslots_per_frame;
> +	u16 slot_width = cfg->slot_width ? (u16)cfg->slot_width : module->slot_width;

[Severity: High]
Could this lead to sending garbage configuration parameters to the DSP?

Callers of audioreach_set_media_format() (like q6apm_dai_prepare() in 
q6apm-dai.c) allocate `struct audioreach_module_config cfg` on the stack 
without zero-initializing it.

Since callers generally only populate specific fields for their own needs,
the newly added fields (slot_mask, nslots_per_frame, and slot_width) will
often contain uninitialized stack memory. 

Because the ternary operators here fall back to the module defaults only if 
the config fields are exactly zero, this stack garbage will frequently evaluate
as true. This overrides the valid topology configuration and sends malformed 
interface parameters to the DSP, which might cause audio path failures or
DSP crashes.

> +	void *p;
> +
> +	struct gpr_pkt *pkt __free(kfree) = audioreach_alloc_apm_cmd_pkt(size, APM_CMD_SET_CFG, 0);
> +	if (IS_ERR(pkt))
> +		return PTR_ERR(pkt);
[ ... ]
> diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h
> index 62a2fd79bbcb9..35541b2d8c998 100644
> --- a/sound/soc/qcom/qdsp6/audioreach.h
> +++ b/sound/soc/qcom/qdsp6/audioreach.h
[ ... ]
> @@ -907,6 +994,9 @@ struct audioreach_module_config {
>  	u32	channel_allocation;
>  	u32	sd_line_mask;
>  	int	fmt;
> +	u32	slot_mask;
> +	u16	nslots_per_frame;
> +	u16	slot_width;
>  	struct snd_codec codec;
>  	u8 channel_map[AR_PCM_MAX_NUM_CHANNEL];
>  };

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