Re: [PATCH] ALSA: jack: Access input_dev under mutex

[Cezary Rojewski <cezary.rojewski@xxxxxxxxx>]

On 2022-04-01 2:29 PM, Amadeusz Sławiński wrote:
> It is possible when using ASoC that input_dev is unregistered while
> calling snd_jack_report, which causes NULL pointer dereference.
> In order to prevent this serialize access to input_dev using mutex lock.
>
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>

Nitpick: "when using ASoC" is quite a generic statement. By that I guess 
you relate to concept of splitting audio functionality into smaller 
logical blocks - components (struct snd_soc_components) - and the 
possible synchronization issues that are part of that division.


That alone is probably not a reason for re-send so:

Reviewed-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>

> ---
>   include/sound/jack.h |  1 +
>   sound/core/jack.c    | 37 ++++++++++++++++++++++++++++++-------
>   2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/include/sound/jack.h b/include/sound/jack.h
> index 1181f536557e..1ed90e2109e9 100644
> --- a/include/sound/jack.h
> +++ b/include/sound/jack.h
> @@ -62,6 +62,7 @@ struct snd_jack {
>   	const char *id;
>   #ifdef CONFIG_SND_JACK_INPUT_DEV
>   	struct input_dev *input_dev;
> +	struct mutex input_dev_lock;
>   	int registered;
>   	int type;
>   	char name[100];
> diff --git a/sound/core/jack.c b/sound/core/jack.c
> index d1e3055f2b6a..58b9ebf5e7e1 100644
> --- a/sound/core/jack.c
> +++ b/sound/core/jack.c
> @@ -42,8 +42,11 @@ static int snd_jack_dev_disconnect(struct snd_device *device)
>   #ifdef CONFIG_SND_JACK_INPUT_DEV
>   	struct snd_jack *jack = device->device_data;
>   
> -	if (!jack->input_dev)
> +	mutex_lock(&jack->input_dev_lock);
> +	if (!jack->input_dev) {
> +		mutex_unlock(&jack->input_dev_lock);
>   		return 0;
> +	}
>   
>   	/* If the input device is registered with the input subsystem
>   	 * then we need to use a different deallocator. */
> @@ -52,6 +55,7 @@ static int snd_jack_dev_disconnect(struct snd_device *device)
>   	else
>   		input_free_device(jack->input_dev);
>   	jack->input_dev = NULL;
> +	mutex_unlock(&jack->input_dev_lock);
>   #endif /* CONFIG_SND_JACK_INPUT_DEV */
>   	return 0;
>   }
> @@ -90,8 +94,11 @@ static int snd_jack_dev_register(struct snd_device *device)
>   	snprintf(jack->name, sizeof(jack->name), "%s %s",
>   		 card->shortname, jack->id);
>   
> -	if (!jack->input_dev)
> +	mutex_lock(&jack->input_dev_lock);
> +	if (!jack->input_dev) {
> +		mutex_unlock(&jack->input_dev_lock);
>   		return 0;
> +	}
>   
>   	jack->input_dev->name = jack->name;
>   
> @@ -116,6 +123,7 @@ static int snd_jack_dev_register(struct snd_device *device)
>   	if (err == 0)
>   		jack->registered = 1;
>   
> +	mutex_unlock(&jack->input_dev_lock);
>   	return err;
>   }
>   #endif /* CONFIG_SND_JACK_INPUT_DEV */
> @@ -517,11 +525,15 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
>   		return -ENOMEM;
>   	}
>   
> -	/* don't creat input device for phantom jack */
> -	if (!phantom_jack) {
>   #ifdef CONFIG_SND_JACK_INPUT_DEV
> +	mutex_init(&jack->input_dev_lock);
> +
> +	/* don't create input device for phantom jack */
> +	if (!phantom_jack) {
>   		int i;
>   
> +		mutex_lock(&jack->input_dev_lock);
> +
>   		jack->input_dev = input_allocate_device();
>   		if (jack->input_dev == NULL) {
>   			err = -ENOMEM;
> @@ -537,8 +549,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
>   				input_set_capability(jack->input_dev, EV_SW,
>   						     jack_switch_types[i]);
>   
> -#endif /* CONFIG_SND_JACK_INPUT_DEV */
> +		mutex_unlock(&jack->input_dev_lock);
>   	}
> +#endif /* CONFIG_SND_JACK_INPUT_DEV */
>   
>   	err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops);
>   	if (err < 0)
> @@ -556,7 +569,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
>   
>   fail_input:
>   #ifdef CONFIG_SND_JACK_INPUT_DEV
> +	mutex_lock(&jack->input_dev_lock);
>   	input_free_device(jack->input_dev);
> +	mutex_unlock(&jack->input_dev_lock);
>   #endif
>   	kfree(jack->id);
>   	kfree(jack);
> @@ -578,10 +593,14 @@ EXPORT_SYMBOL(snd_jack_new);
>   void snd_jack_set_parent(struct snd_jack *jack, struct device *parent)
>   {
>   	WARN_ON(jack->registered);
> -	if (!jack->input_dev)
> +	mutex_lock(&jack->input_dev_lock);
> +	if (!jack->input_dev) {
> +		mutex_unlock(&jack->input_dev_lock);
>   		return;
> +	}
>   
>   	jack->input_dev->dev.parent = parent;
> +	mutex_unlock(&jack->input_dev_lock);
>   }
>   EXPORT_SYMBOL(snd_jack_set_parent);
>   
> @@ -654,8 +673,11 @@ void snd_jack_report(struct snd_jack *jack, int status)
>   					     status & jack_kctl->mask_bits);
>   
>   #ifdef CONFIG_SND_JACK_INPUT_DEV
> -	if (!jack->input_dev)
> +	mutex_lock(&jack->input_dev_lock);
> +	if (!jack->input_dev) {
> +		mutex_unlock(&jack->input_dev_lock);
>   		return;
> +	}
>   
>   	for (i = 0; i < ARRAY_SIZE(jack->key); i++) {
>   		int testbit = ((SND_JACK_BTN_0 >> i) & ~mask_bits);
> @@ -675,6 +697,7 @@ void snd_jack_report(struct snd_jack *jack, int status)
>   	}
>   
>   	input_sync(jack->input_dev);
> +	mutex_unlock(&jack->input_dev_lock);
>   #endif /* CONFIG_SND_JACK_INPUT_DEV */
>   }
>   EXPORT_SYMBOL(snd_jack_report);