[alsa-devel] [PATCH v1] ASoC: rt5640: Fix NULL dereference on module unload

[alsa-devel] [PATCH v1] ASoC: rt5640: Fix NULL dereference on module unload

[Dmitry Osipenko]

The rt5640->jack is NULL if jack is already disabled at the time of
driver's module unloading.

 Unable to handle kernel NULL pointer dereference at virtual address 00000024
 ...
 (rt5640_set_jack [snd_soc_rt5640]) from [<bf86f7ed>] (snd_soc_component_set_jack+0x11/0x1c [snd_soc_core])
 (snd_soc_component_set_jack [snd_soc_core]) from [<bf8675cf>] (soc_remove_component+0x1b/0x54 [snd_soc_core])
 (soc_remove_component [snd_soc_core]) from [<bf868859>] (soc_cleanup_card_resources+0xad/0x1cc [snd_soc_core])
 (soc_cleanup_card_resources [snd_soc_core]) from [<bf86945f>] (snd_soc_unregister_card+0x47/0x78 [snd_soc_core])
 (snd_soc_unregister_card [snd_soc_core]) from [<bf8b4013>] (tegra_rt5640_remove+0x13/0x1c [snd_soc_tegra_rt5640])
 (tegra_rt5640_remove [snd_soc_tegra_rt5640]) from [<c0516d2f>] (platform_drv_remove+0x17/0x24)
 (platform_drv_remove) from [<c0515aed>] (device_release_driver_internal+0x95/0x114)
 (device_release_driver_internal) from [<c0515bd9>] (driver_detach+0x4d/0x90)
 (driver_detach) from [<c0514d59>] (bus_remove_driver+0x31/0x70)
 (bus_remove_driver) from [<bf8b4215>] (tegra_rt5640_driver_exit+0x9/0xdf4 [snd_soc_tegra_rt5640])
 (tegra_rt5640_driver_exit [snd_soc_tegra_rt5640]) from [<c019336f>] (sys_delete_module+0xe7/0x184)
 (sys_delete_module) from [<c0101001>] (ret_fast_syscall+0x1/0x28)

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 sound/soc/codecs/rt5640.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index adbae1f36a8a..b245c44cafbc 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -2432,16 +2432,22 @@ static void rt5640_disable_jack_detect(struct snd_soc_component *component)
 {
 	struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component);
 
-	disable_irq(rt5640->irq);
-	rt5640_cancel_work(rt5640);
+	/*
+	 * soc_remove_component() force-disables jack and thus rt5640->jack
+	 * could be NULL at the time of driver's module unloading.
+	 */
+	if (rt5640->jack) {
+		disable_irq(rt5640->irq);
+		rt5640_cancel_work(rt5640);
 
-	if (rt5640->jack->status & SND_JACK_MICROPHONE) {
-		rt5640_disable_micbias1_ovcd_irq(component);
-		rt5640_disable_micbias1_for_ovcd(component);
-		snd_soc_jack_report(rt5640->jack, 0, SND_JACK_BTN_0);
-	}
+		if (rt5640->jack->status & SND_JACK_MICROPHONE) {
+			rt5640_disable_micbias1_ovcd_irq(component);
+			rt5640_disable_micbias1_for_ovcd(component);
+			snd_soc_jack_report(rt5640->jack, 0, SND_JACK_BTN_0);
+		}
 
-	rt5640->jack = NULL;
+		rt5640->jack = NULL;
+	}
 }
 
 static int rt5640_set_jack(struct snd_soc_component *component,
-- 
2.24.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v1] ASoC: rt5640: Fix NULL dereference on module unload

[Takashi Iwai]

On Sun, 29 Dec 2019 16:04:54 +0100,
Dmitry Osipenko wrote:
> 
> The rt5640->jack is NULL if jack is already disabled at the time of
> driver's module unloading.
> 
>  Unable to handle kernel NULL pointer dereference at virtual address 00000024
>  ...
>  (rt5640_set_jack [snd_soc_rt5640]) from [<bf86f7ed>] (snd_soc_component_set_jack+0x11/0x1c [snd_soc_core])
>  (snd_soc_component_set_jack [snd_soc_core]) from [<bf8675cf>] (soc_remove_component+0x1b/0x54 [snd_soc_core])
>  (soc_remove_component [snd_soc_core]) from [<bf868859>] (soc_cleanup_card_resources+0xad/0x1cc [snd_soc_core])
>  (soc_cleanup_card_resources [snd_soc_core]) from [<bf86945f>] (snd_soc_unregister_card+0x47/0x78 [snd_soc_core])
>  (snd_soc_unregister_card [snd_soc_core]) from [<bf8b4013>] (tegra_rt5640_remove+0x13/0x1c [snd_soc_tegra_rt5640])
>  (tegra_rt5640_remove [snd_soc_tegra_rt5640]) from [<c0516d2f>] (platform_drv_remove+0x17/0x24)
>  (platform_drv_remove) from [<c0515aed>] (device_release_driver_internal+0x95/0x114)
>  (device_release_driver_internal) from [<c0515bd9>] (driver_detach+0x4d/0x90)
>  (driver_detach) from [<c0514d59>] (bus_remove_driver+0x31/0x70)
>  (bus_remove_driver) from [<bf8b4215>] (tegra_rt5640_driver_exit+0x9/0xdf4 [snd_soc_tegra_rt5640])
>  (tegra_rt5640_driver_exit [snd_soc_tegra_rt5640]) from [<c019336f>] (sys_delete_module+0xe7/0x184)
>  (sys_delete_module) from [<c0101001>] (ret_fast_syscall+0x1/0x28)
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  sound/soc/codecs/rt5640.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
> index adbae1f36a8a..b245c44cafbc 100644
> --- a/sound/soc/codecs/rt5640.c
> +++ b/sound/soc/codecs/rt5640.c
> @@ -2432,16 +2432,22 @@ static void rt5640_disable_jack_detect(struct snd_soc_component *component)
>  {
>  	struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component);
>  
> -	disable_irq(rt5640->irq);
> -	rt5640_cancel_work(rt5640);
> +	/*
> +	 * soc_remove_component() force-disables jack and thus rt5640->jack
> +	 * could be NULL at the time of driver's module unloading.
> +	 */
> +	if (rt5640->jack) {
> +		disable_irq(rt5640->irq);
> +		rt5640_cancel_work(rt5640);
>  
> -	if (rt5640->jack->status & SND_JACK_MICROPHONE) {
> -		rt5640_disable_micbias1_ovcd_irq(component);
> -		rt5640_disable_micbias1_for_ovcd(component);
> -		snd_soc_jack_report(rt5640->jack, 0, SND_JACK_BTN_0);
> -	}
> +		if (rt5640->jack->status & SND_JACK_MICROPHONE) {
> +			rt5640_disable_micbias1_ovcd_irq(component);
> +			rt5640_disable_micbias1_for_ovcd(component);
> +			snd_soc_jack_report(rt5640->jack, 0, SND_JACK_BTN_0);
> +		}
>  
> -	rt5640->jack = NULL;
> +		rt5640->jack = NULL;
> +	}
>  }

I guess it's simpler just returning if rt5640->jack is already NULL.

--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -2432,6 +2432,10 @@ static void rt5640_disable_jack_detect(struct snd_soc_component *component)
 {
 	struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component);
 
+	/* already disabled? */
+	if (!rt5640->jack)
+		return;
+
 	disable_irq(rt5640->irq);
 	rt5640_cancel_work(rt5640);
 

thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v1] ASoC: rt5640: Fix NULL dereference on module unload

[Dmitry Osipenko]

30.12.2019 10:11, Takashi Iwai пишет:
> On Sun, 29 Dec 2019 16:04:54 +0100,
> Dmitry Osipenko wrote:
>>
>> The rt5640->jack is NULL if jack is already disabled at the time of
>> driver's module unloading.
>>
>>  Unable to handle kernel NULL pointer dereference at virtual address 00000024
>>  ...
>>  (rt5640_set_jack [snd_soc_rt5640]) from [<bf86f7ed>] (snd_soc_component_set_jack+0x11/0x1c [snd_soc_core])
>>  (snd_soc_component_set_jack [snd_soc_core]) from [<bf8675cf>] (soc_remove_component+0x1b/0x54 [snd_soc_core])
>>  (soc_remove_component [snd_soc_core]) from [<bf868859>] (soc_cleanup_card_resources+0xad/0x1cc [snd_soc_core])
>>  (soc_cleanup_card_resources [snd_soc_core]) from [<bf86945f>] (snd_soc_unregister_card+0x47/0x78 [snd_soc_core])
>>  (snd_soc_unregister_card [snd_soc_core]) from [<bf8b4013>] (tegra_rt5640_remove+0x13/0x1c [snd_soc_tegra_rt5640])
>>  (tegra_rt5640_remove [snd_soc_tegra_rt5640]) from [<c0516d2f>] (platform_drv_remove+0x17/0x24)
>>  (platform_drv_remove) from [<c0515aed>] (device_release_driver_internal+0x95/0x114)
>>  (device_release_driver_internal) from [<c0515bd9>] (driver_detach+0x4d/0x90)
>>  (driver_detach) from [<c0514d59>] (bus_remove_driver+0x31/0x70)
>>  (bus_remove_driver) from [<bf8b4215>] (tegra_rt5640_driver_exit+0x9/0xdf4 [snd_soc_tegra_rt5640])
>>  (tegra_rt5640_driver_exit [snd_soc_tegra_rt5640]) from [<c019336f>] (sys_delete_module+0xe7/0x184)
>>  (sys_delete_module) from [<c0101001>] (ret_fast_syscall+0x1/0x28)
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  sound/soc/codecs/rt5640.c | 22 ++++++++++++++--------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
>> index adbae1f36a8a..b245c44cafbc 100644
>> --- a/sound/soc/codecs/rt5640.c
>> +++ b/sound/soc/codecs/rt5640.c
>> @@ -2432,16 +2432,22 @@ static void rt5640_disable_jack_detect(struct snd_soc_component *component)
>>  {
>>  	struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component);
>>  
>> -	disable_irq(rt5640->irq);
>> -	rt5640_cancel_work(rt5640);
>> +	/*
>> +	 * soc_remove_component() force-disables jack and thus rt5640->jack
>> +	 * could be NULL at the time of driver's module unloading.
>> +	 */
>> +	if (rt5640->jack) {
>> +		disable_irq(rt5640->irq);
>> +		rt5640_cancel_work(rt5640);
>>  
>> -	if (rt5640->jack->status & SND_JACK_MICROPHONE) {
>> -		rt5640_disable_micbias1_ovcd_irq(component);
>> -		rt5640_disable_micbias1_for_ovcd(component);
>> -		snd_soc_jack_report(rt5640->jack, 0, SND_JACK_BTN_0);
>> -	}
>> +		if (rt5640->jack->status & SND_JACK_MICROPHONE) {
>> +			rt5640_disable_micbias1_ovcd_irq(component);
>> +			rt5640_disable_micbias1_for_ovcd(component);
>> +			snd_soc_jack_report(rt5640->jack, 0, SND_JACK_BTN_0);
>> +		}
>>  
>> -	rt5640->jack = NULL;
>> +		rt5640->jack = NULL;
>> +	}
>>  }
> 
> I guess it's simpler just returning if rt5640->jack is already NULL.
> 
> --- a/sound/soc/codecs/rt5640.c
> +++ b/sound/soc/codecs/rt5640.c
> @@ -2432,6 +2432,10 @@ static void rt5640_disable_jack_detect(struct snd_soc_component *component)
>  {
>  	struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component);
>  
> +	/* already disabled? */
> +	if (!rt5640->jack)
> +		return;
> +
>  	disable_irq(rt5640->irq);
>  	rt5640_cancel_work(rt5640);
>  
> 
> thanks,
> 
> Takashi
> 

Okay, I'll make v2.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v1] ASoC: rt5640: Fix NULL dereference on module unload

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 969 bytes --]

On Sun, Dec 29, 2019 at 06:04:54PM +0300, Dmitry Osipenko wrote:
> The rt5640->jack is NULL if jack is already disabled at the time of
> driver's module unloading.
> 
>  Unable to handle kernel NULL pointer dereference at virtual address 00000024
>  ...
>  (rt5640_set_jack [snd_soc_rt5640]) from [<bf86f7ed>] (snd_soc_component_set_jack+0x11/0x1c [snd_soc_core])
>  (snd_soc_component_set_jack [snd_soc_core]) from [<bf8675cf>] (soc_remove_component+0x1b/0x54 [snd_soc_core])
>  (soc_remove_component [snd_soc_core]) from [<bf868859>] (soc_cleanup_card_resources+0xad/0x1cc [snd_soc_core])

In addition to what Takashi said:

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative then it's
usually better to pull out the relevant sections.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v1] ASoC: rt5640: Fix NULL dereference on module unload

[Dmitry Osipenko]

31.12.2019 03:17, Mark Brown пишет:
> On Sun, Dec 29, 2019 at 06:04:54PM +0300, Dmitry Osipenko wrote:
>> The rt5640->jack is NULL if jack is already disabled at the time of
>> driver's module unloading.
>>
>>  Unable to handle kernel NULL pointer dereference at virtual address 00000024
>>  ...
>>  (rt5640_set_jack [snd_soc_rt5640]) from [<bf86f7ed>] (snd_soc_component_set_jack+0x11/0x1c [snd_soc_core])
>>  (snd_soc_component_set_jack [snd_soc_core]) from [<bf8675cf>] (soc_remove_component+0x1b/0x54 [snd_soc_core])
>>  (soc_remove_component [snd_soc_core]) from [<bf868859>] (soc_cleanup_card_resources+0xad/0x1cc [snd_soc_core])
> 
> In addition to what Takashi said:
> 
> Please think hard before including complete backtraces in upstream
> reports, they are very large and contain almost no useful information
> relative to their size so often obscure the relevant content in your
> message. If part of the backtrace is usefully illustrative then it's
> usually better to pull out the relevant sections.

Yeah, perhaps it's not really useful to have backtrace in the commit's
description for the case of this patch in particular. But in general it
is very useful to have backtraces somewhere near the patch such that
online search engines, like google, could pick it up. I'll move the
backtrace below --- in v2, thanks.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v1] ASoC: rt5640: Fix NULL dereference on module unload

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1114 bytes --]

On Thu, Jan 02, 2020 at 06:57:14PM +0300, Dmitry Osipenko wrote:
> 31.12.2019 03:17, Mark Brown пишет:

> > Please think hard before including complete backtraces in upstream
> > reports, they are very large and contain almost no useful information
> > relative to their size so often obscure the relevant content in your
> > message. If part of the backtrace is usefully illustrative then it's
> > usually better to pull out the relevant sections.

> Yeah, perhaps it's not really useful to have backtrace in the commit's
> description for the case of this patch in particular. But in general it
> is very useful to have backtraces somewhere near the patch such that
> online search engines, like google, could pick it up. I'll move the
> backtrace below --- in v2, thanks.

Right, this is more directed at just pasting in the entire
backtrace (which can be huge with lots of generic bits before the
small number that are relevant) but some edited highlights can
definitely be helpful for search engines and for explaining the
problem.  I'll modify what I'm saying there to clarify this.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v1] ASoC: rt5640: Fix NULL dereference on module unload

[Dmitry Osipenko]

03.01.2020 03:54, Mark Brown пишет:
> On Thu, Jan 02, 2020 at 06:57:14PM +0300, Dmitry Osipenko wrote:
>> 31.12.2019 03:17, Mark Brown пишет:
> 
>>> Please think hard before including complete backtraces in upstream
>>> reports, they are very large and contain almost no useful information
>>> relative to their size so often obscure the relevant content in your
>>> message. If part of the backtrace is usefully illustrative then it's
>>> usually better to pull out the relevant sections.
> 
>> Yeah, perhaps it's not really useful to have backtrace in the commit's
>> description for the case of this patch in particular. But in general it
>> is very useful to have backtraces somewhere near the patch such that
>> online search engines, like google, could pick it up. I'll move the
>> backtrace below --- in v2, thanks.
> 
> Right, this is more directed at just pasting in the entire
> backtrace (which can be huge with lots of generic bits before the
> small number that are relevant) but some edited highlights can
> definitely be helpful for search engines and for explaining the
> problem.  I'll modify what I'm saying there to clarify this.

Thank you for the clarification.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel