Re: [PATCH v3 1/3] ASoC: qcom: audioreach: compute active channel maps from channel_map

On 7/28/26 11:03, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] The new `active_channels_mask` computation in `audioreach_codec_dma_set_media_format` introduces regressions by unconditionally iterating over all `AR_PCM_MAX_NUM_CHANNEL` elements.
> 
> Pre-existing issues:
> - [High] Kernel stack memory is leaked to the DSP and evaluated in control logic because `cfg.channel_map` is not zero-initialized.
> --
> 
> --- Patch [1]: [PATCH v3 1/3] ASoC: qcom: audioreach: compute active channel maps from channel_map ---
> commit 893598480a0705f69c55eefe5bf4e2b969edc6d8
> Author: Neil Armstrong <[email protected]>
> Subject: ASoC: qcom: audioreach: compute active channel maps from channel_map
> 
> This patch updates the Audioreach DSP code to compute the active channel
> mask and logical channel mappings using the physical channel mapping array
> (channel_map) rather than a simple bitmask derived from the channel count.
> This enables WSA2 speaker usage on specific hardware by correctly setting
> only the upper bits of the channel mask.
> 
>> diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
>> index e6e9eb2e85aa1..0cc840aca69dd 100644
>> --- a/sound/soc/qcom/qdsp6/audioreach.c
>> +++ b/sound/soc/qcom/qdsp6/audioreach.c
>> @@ -741,7 +742,12 @@ static int audioreach_codec_dma_set_media_format(struct q6apm_graph *graph,
>>   
>>   	intf_cfg->cfg.lpaif_type = module->hw_interface_type;
>>   	intf_cfg->cfg.intf_index = module->hw_interface_idx;
>> -	intf_cfg->cfg.active_channels_mask = (1 << cfg->num_channels) - 1;
>> +	intf_cfg->cfg.active_channels_mask = 0;
>> +	/* Convert the physical channel mapping into a bit field */
>> +	for (i = 0; i < AR_PCM_MAX_NUM_CHANNEL; i++)
>> +		if (cfg->channel_map[i])
>> +			intf_cfg->cfg.active_channels_mask |= BIT(i);
>> +
> 
> [Severity: High]
> If a stream is configured with 4 channels and later reconfigured to 2
> channels, audioreach_set_default_channel_mapping() in q6dma_hw_params() only
> overwrites the first 2 entries, leaving the remaining entries intact.

This is a valid issue, will fix.

> 
> Since this loop iterates unconditionally up to AR_PCM_MAX_NUM_CHANNEL, could
> it pick up those stale entries and activate unintended channels in the DSP?
> 
> Additionally, for channel counts greater than 4 (like 6 or 8),
> audioreach_set_default_channel_mapping() can leave the map as all zeros. Could
> this result in an active_channels_mask of 0 and break the DMA configuration?

Yes, the old shift should be kept if the result mask is 0

> 
> [Severity: High]
> This is a pre-existing issue, but struct audioreach_module_config cfg is
> allocated on the stack in q6apm_dai_prepare() without being initialized:
> 
> q6apm_dai_prepare() {
>      ...
>      struct audioreach_module_config cfg;
>      ...
> }
> 
> When an unsupported channel count is used,
> audioreach_set_default_channel_mapping() leaves the array untouched,
> containing stack garbage.
> 
> While the previous code copied this memory blindly, the new loop here branches
> on this uninitialized stack memory to construct the channel mask. Could this
> result in unpredictable DSP configurations and leak stack contents in the IPC
> payload?

Yep the mapping array should be set to 0 in audioreach_set_default_channel_mapping() whatever the channel count

> 
>>   	p += ic_sz;
>>   
>>   	pm_cfg = p;
> 
> [ ... ]
> 
>> @@ -1113,7 +1124,12 @@ static int audioreach_pcm_set_media_format(struct q6apm_graph *graph,
>>   	media_cfg->num_channels = mcfg->num_channels;
>>   	media_cfg->q_factor = mcfg->bit_width - 1;
>>   	media_cfg->bits_per_sample = mcfg->bit_width;
>> -	memcpy(media_cfg->channel_mapping, mcfg->channel_map, mcfg->num_channels);
>> +	/* Convert the physical mapping to a logical mapping of the channels */
>> +	for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < mcfg->num_channels; i++) {
>> +		if (!mcfg->channel_map[i])
>> +			continue;
>> +		media_cfg->channel_mapping[j++] = mcfg->channel_map[i];
>> +	}
> 
> [Severity: High]
> This is a pre-existing issue, but similar to the above concern, could this
> loop branch on uninitialized stack memory if mcfg->channel_map was left
> uninitialized in q6apm_dai_prepare()?
> 
> This would cause the code to evaluate stack garbage in its control flow and
> copy it into the DSP IPC payload.
> 
>>   
>>   	return q6apm_send_cmd_sync(graph->apm, pkt, 0);
>>   }
>