Re: [PATCH] ALSA: pcm: Protect channel-map reads against runtime detach

[Takashi Iwai <tiwai@suse.de>]

On Mon, 19 Oct 2026 12:29:33 +0200,
Cen Zhang wrote:
> 
> Channel-map controls remain accessible between PCM opens. In-tree HDA
> code publishes the generic controls through add_std_chmaps(), so a
> SNDRV_CTL_IOCTL_ELEM_READ can overlap the final close of the same PCM
> substream.
> 
> pcm_chmap_ctl_get() checks substream->runtime and later reads its channel
> count. Final close clears and frees that runtime under pcm->open_mutex,
> while the control callback holds only controls_rwsem:
> 
>   Control GET                         Final PCM close
>   -----------                         ---------------
>   snd_ctl_elem_read()
>     down_read(controls_rwsem)
>     pcm_chmap_ctl_get()
>       check substream->runtime
>                                       lock(pcm->open_mutex)
>                                       snd_pcm_release_substream()
>                                         snd_pcm_detach_substream()
>                                           substream->runtime = NULL
>                                           kfree(runtime)
>                                       unlock(pcm->open_mutex)
>       substream->runtime->channels
>         NULL or freed-pointer access
>     up_read(controls_rwsem)
> 
> Reloading the cleared field can fault through NULL, while retaining the
> old pointer can access freed storage. controls_rwsem keeps the control
> alive, but does not serialize the matching PCM close.
> 
> Copy the channel count under open_mutex and use that scalar for the map
> lookup. Use trylock with -EBUSY because snd_pcm_dev_disconnect() takes
> open_mutex before removing controls under controls_rwsem; blocking in
> the reverse order from GET could deadlock.
> 
> Fixes: 2d3391ec0ecc ("ALSA: PCM: channel mapping API implementation")
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> 
> ---
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 4d665b4148d7059402a55abb62fb24da8ff73f69..1ec7db324196f60d8c72a879c62ddae0a90ac0ff 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2496,6 +2496,7 @@ static int pcm_chmap_ctl_get(struct snd_kcontrol *kcontrol,
>  	unsigned int idx = snd_ctl_get_ioffidx(kcontrol, &ucontrol->id);
>  	struct snd_pcm_substream *substream;
>  	const struct snd_pcm_chmap_elem *map;
> +	unsigned int channels;
>  
>  	if (!info->chmap)
>  		return -EINVAL;
> @@ -2504,11 +2505,20 @@ static int pcm_chmap_ctl_get(struct snd_kcontrol *kcontrol,
>  		return -ENODEV;
>  	memset(ucontrol->value.integer.value, 0,
>  	       sizeof(long) * info->max_channels);
> -	if (!substream->runtime)
> -		return 0; /* no channels set */
> +	/*
> +	 * PCM disconnect takes open_mutex before removing controls.
> +	 * Use trylock to avoid a deadlock with controls_rwsem.
> +	 */
> +	scoped_cond_guard(mutex_try, return -EBUSY,
> +			  &info->pcm->open_mutex) {
> +		if (!substream->runtime)
> +			return 0; /* no channels set */
> +		channels = substream->runtime->channels;
> +	}
>  	for (map = info->chmap; map->channels; map++) {
>  		int i;
> -		if (map->channels == substream->runtime->channels &&
> +
> +		if (map->channels == channels &&
>  		    valid_chmap_channels(info, map->channels)) {
>  			for (i = 0; i < map->channels; i++)
>  				ucontrol->value.integer.value[i] = map->map[i];

Taking a mutex from the kcontrol here sounds a bit scary, and
moreover, we need a saner and safer way to get the runtime reference
in general.  OTOH, for a workaround of this issue, this code change
looks  simple enough, so I took as is now.


thanks,

Takashi