[PATCH] ALSA: jack: Access input_dev under mutex

[PATCH] ALSA: jack: Access input_dev under mutex

[Amadeusz Sławiński]

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@linux.intel.com>
---
 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);
-- 
2.25.1

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

[Cezary Rojewski]

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@linux.intel.com>

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@intel.com>

> ---
>   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);

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

[Takashi Iwai]

On Fri, 01 Apr 2022 14:29:31 +0200,
Amadeusz Sławiński wrote:
> 
> @@ -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);
> +

This lock is superfluous.  The object is being created here and isn't
registered anywhere, so no one else can use it yet.
And moreover ....

>  		jack->input_dev = input_allocate_device();
>  		if (jack->input_dev == NULL) {
>  			err = -ENOMEM;

... you forgot unlock here, and this error path will lead to a
deadlock.


> @@ -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);

Ditto, no need for mutex lock yet.

One thing I'm not sure is whether it's good to use mutex.
Basically snd_jack_report() is considered to be callable from
non-atomic context, too, so far, and we don't need to prohibit it.
OTOH, all current calls are in the sleepable context, so it's likely
no big problem.  But if we use mutex, it should be documented in
snd_jack_report() function, too.


thanks,

Takashi