Re: [PATCH 2/2] ASoC: SDCA: Add port constraints into sdca_asoc_set_constraints()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] ASoC: SDCA: wrong array index in get_port_masks()
- [High] ASoC: SDCA: reversed GENMASK() arguments in get_port_masks()
- [High] ASoC: SDCA: u32 underflow and zeroed masks in get_port_masks()
- [High] ASoC: SDCA: use-after-free of hw_constraint_list in sdca_asoc_set_constraints()
- [Medium] ASoC: SDCA: zero channel mask in sdca_asoc_set_constraints() removes constraints
--

commit de604ca1528e256ed698775b3d19458c5cd6b4ab
Author: Charles Keepax <ckeepax@opensource.cirrus.com>

ASoC: SDCA: Add port constraints into sdca_asoc_set_constraints()

This commit adds SoundWire data port channel count and sample bit width
constraints into sdca_asoc_set_constraints() for both Input and Output
Terminals.

> diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
> index b801cb92cc3db..4a96ec5d11743 100644
> --- a/sound/soc/sdca/sdca_asoc.c
> +++ b/sound/soc/sdca/sdca_asoc.c

[ ... ]

> @@ -1465,9 +1467,56 @@ static int get_cluster_mask(struct device *dev, struct sdca_function_data *funct
>  	return 0;
>  }
>  
> +static int get_port_masks(struct sdw_slave *sdw, unsigned int port,
> +			  struct snd_pcm_substream *substream,
> +			  unsigned int *word_mask, unsigned int *ch_mask)
> +{
> +	struct sdw_dpn_prop *prop;
> +	unsigned int nprop;
> +	int i, j;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		prop = sdw->prop.sink_dpn_prop;
> +		nprop = hweight32(sdw->prop.sink_ports);
> +	} else {
> +		prop = sdw->prop.src_dpn_prop;
> +		nprop = hweight32(sdw->prop.source_ports);
> +	}
> +
> +	*word_mask = 0;
> +	*ch_mask = 0;
> +
> +	for (i = 0; i < nprop; i++) {
> +		if (prop[i].num != port)
> +			continue;
> +
> +		if (prop->min_word && prop->max_word) {
> +			*word_mask = GENMASK(prop->min_word - 1, prop->max_word - 1);
> +		} else {
> +			for (j = 0; j < prop->num_words; j++)
> +				*word_mask |= (1 << (prop->words[j] - 1));
> +		}
> +
> +		/* Only handle the number of channels for constraints. */
> +		if (prop->num_ch_combinations) {
> +			for (j = 0; j < prop->num_ch_combinations; j++)
> +				*ch_mask |= (1 << (hweight32(prop->ch_combinations[j]) - 1));
> +		} else if (prop->min_ch && prop->max_ch) {
> +			*ch_mask = GENMASK(prop->min_ch - 1, prop->max_ch - 1);
> +		} else {
> +			*ch_mask = GENMASK(0, prop->num_channels - 1);
> +		}

[Severity: High]
In get_port_masks(), after matching prop[i].num == port, all of the
subsequent field accesses dereference prop (i.e. prop[0]) instead of
prop[i].

When a peripheral has multiple SoundWire data ports and the matched port
is at index i > 0 of sink_dpn_prop or src_dpn_prop, won't this compute
word_mask and ch_mask from the first port's properties rather than the
matched port?

[Severity: High]
Are the arguments to GENMASK() reversed in get_port_masks()?

GENMASK(h, l) takes the high bit position first and the low bit position
second:

  GENMASK(prop[i].max_word - 1, prop[i].min_word - 1)
  GENMASK(prop[i].max_ch - 1, prop[i].min_ch - 1)
  GENMASK(prop[i].num_channels - 1, 0)

With the low bit passed as h and the high bit passed as l, whenever
min_word < max_word, min_ch < max_ch, or num_channels > 1, the shift masks
have no overlapping bits and evaluate to 0.

[Severity: High]
What happens in get_port_masks() when optional data port channel or
wordlength properties are omitted?

In MIPI DisCo for SoundWire (sdw_slave_read_dpn()) and in SDCA peripheral
drivers such as wcd9378_sdca_read_prop(), data port channel properties
(num_ch_combinations, min_ch/max_ch, num_channels) and wordlength
properties (min_word/max_word, num_words) are optional.

When num_ch_combinations, min_ch/max_ch, and num_channels are all 0,
prop->num_channels - 1 underflows u32 to 0xffffffff inside GENMASK(),
causing a shift-out-of-bounds and leaving *ch_mask as 0. That zero
port_ch_mask then zeroes out cluster_ch_mask & port_ch_mask in
sdca_asoc_set_constraints().

> +
> +		return 0;
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  /**
>   * sdca_asoc_set_constraints - constrain channels available on a DAI
>   * @dev: Pointer to the device, used for error messages.
> + * @sdw: Pointer to the SoundWire peripheral device.
>   * @regmap: Pointer to the Function register map.
>   * @function: Pointer to the Function information.
>   * @substream: Pointer to the PCM substream.
> @@ -1477,51 +1526,82 @@ static int get_cluster_mask(struct device *dev, struct sdca_function_data *funct
>   *
>   * Return: Returns zero on success, and a negative error code on failure.
>   */
> -int sdca_asoc_set_constraints(struct device *dev, struct regmap *regmap,
> +int sdca_asoc_set_constraints(struct device *dev, struct sdw_slave *sdw,
> +			      struct regmap *regmap,
>  			      struct sdca_function_data *function,
>  			      struct snd_pcm_substream *substream,
>  			      struct snd_soc_dai *dai)
>  {
> -	static const unsigned int channel_list[] = {
> +	static const unsigned int constraint_list[] = {
>  		 1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15, 16,
>  		17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32,
>  	};
>  	struct sdca_entity *entity = &function->entities[dai->id];
> -	struct snd_pcm_hw_constraint_list *constraint;
> +	struct snd_pcm_hw_constraint_list *constraints;
> +	unsigned int port_ch_mask, port_width_mask;
>  	unsigned int cluster_ch_mask;
> +	int port;
>  	int ret;
>  
> -	static_assert(ARRAY_SIZE(channel_list) == SDCA_MAX_CHANNEL_COUNT);
> +	static_assert(ARRAY_SIZE(constraint_list) == SDCA_MAX_CHANNEL_COUNT);
>  	static_assert(sizeof(cluster_ch_mask) * BITS_PER_BYTE >= SDCA_MAX_CHANNEL_COUNT);
>  
> -	if (entity->type != SDCA_ENTITY_TYPE_IT)
> -		return 0;
> +	/* FIXME: Currently only a single unshared port is supported */
> +	port = sdca_asoc_get_port(dev, regmap, function, dai);
> +	if (port < 0)
> +		return port;
>  
> -	ret = get_cluster_mask(dev, function, entity, &cluster_ch_mask);
> -	if (ret)
> +	ret = get_port_masks(sdw, port, substream, &port_width_mask, &port_ch_mask);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: no properties for port %d\n", entity->label, port);
>  		return ret;
> +	}
>  
> -	dev_dbg(dev, "%s: set channel constraint mask: %#x\n",
> -		entity->label, cluster_ch_mask);
> +	/* FIXME: OTs should inherit cluster information */
> +	if (entity->type == SDCA_ENTITY_TYPE_IT) {
> +		ret = get_cluster_mask(dev, function, entity, &cluster_ch_mask);
> +		if (ret)
> +			return ret;
> +	} else {
> +		cluster_ch_mask = UINT_MAX;
> +	}
>  
> -	constraint = kzalloc_obj(*constraint);
> -	if (!constraint)
> +	constraints = kzalloc_objs(*constraints, 2);
> +	if (!constraints)
>  		return -ENOMEM;
>  
> -	constraint->count = ARRAY_SIZE(channel_list);
> -	constraint->list = channel_list;
> -	constraint->mask = cluster_ch_mask;
> +	constraints[0].count = ARRAY_SIZE(constraint_list);
> +	constraints[0].list = constraint_list;
> +	constraints[0].mask = cluster_ch_mask & port_ch_mask;

[Severity: Medium]
If cluster_ch_mask & port_ch_mask evaluates to 0 in
sdca_asoc_set_constraints() (for example, if the cluster and port channel
masks have no common channel counts), constraints[0].mask is set to 0 and
passed to snd_pcm_hw_constraint_list().

In sound/core/pcm_lib.c, snd_interval_list() treats mask == 0 as meaning no
mask filter should be applied:

	for (k = 0; k < count; k++) {
		if (mask && !(mask & (1 << k)))
			continue;

Won't a zero mask cause snd_interval_list() to permit all 1..32 channels in
constraint_list rather than rejecting the empty intersection?

> +
> +	constraints[1].count = ARRAY_SIZE(constraint_list);
> +	constraints[1].list = constraint_list;
> +	constraints[1].mask = port_width_mask;
> +
> +	dev_dbg(dev, "%s: set constraint masks, channel: %#x, width: %#x\n",
> +		entity->label, constraints[0].mask, constraints[1].mask);
>  
>  	ret = snd_pcm_hw_constraint_list(substream->runtime, 0,
>  					 SNDRV_PCM_HW_PARAM_CHANNELS,
> -					 constraint);
> +					 &constraints[0]);
>  	if (ret) {
> -		dev_err(dev, "%s: failed to add constraint: %d\n", entity->label, ret);
> -		kfree(constraint);
> +		dev_err(dev, "%s: failed to add channel constraint: %d\n",
> +			entity->label, ret);
> +		kfree(constraints);
> +		return ret;
> +	}
> +
> +	ret = snd_pcm_hw_constraint_list(substream->runtime, 0,
> +					 SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
> +					 &constraints[1]);
> +	if (ret) {
> +		dev_err(dev, "%s: failed to add word constraint: %d\n",
> +			entity->label, ret);
> +		kfree(constraints);
>  		return ret;
>  	}

[Severity: High]
Can freeing constraints here (or in sdca_asoc_free_constraints()) leave a
dangling pointer in substream->runtime->hw_constraints.rules?

When the first snd_pcm_hw_constraint_list() call succeeds,
snd_pcm_hw_rule_add() stores &constraints[0] in rule->private inside
substream->runtime->hw_constraints.rules.

If the second snd_pcm_hw_constraint_list() call fails, kfree(constraints)
frees the array while &constraints[0] remains registered in
runtime->hw_constraints.rules. Similarly, on DPCM SoundWire setups where
dpcm_be_dai_startup() aliases be_substream->runtime = fe_substream->runtime,
if a backend startup rolls back or the backend shuts down via
dpcm_be_dai_shutdown() -> sdca_asoc_free_constraints() while the frontend
substream remains open, subsequent hw_refine or hw_params calls on the
frontend will invoke snd_pcm_hw_rule_list() and dereference the freed
constraints memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105134952.1728147-1-ckeepax@opensource.cirrus.com?part=2