ASoC: rt5640: Fix NULL dereference on module unload

PATCH Superseded Dmitry Osipenko 3 participants 7 messages Dec 29, 2019 – Jan 4, 2020

Subsystems (4)

Sound ASoC Tegra architecture support Realtek audio codecs

lore

Share & tools

lore

Dmitry Osipenko

author

2019-12-29 15:04 UTC #0 lore raw

PATCH 1/1 ASoC: rt5640: Fix NULL dereference on module unload

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 <[email protected]>

---
 sound/soc/codecs/rt5640.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

sound/soc/codecs/rt5640.c +14 -8

index adbae1f36a8a..b245c44cafbc 100644
@@ -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,

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2019-12-30 19:37 UTC #2 lore raw

30.12.2019 10:11, Takashi Iwai пишет:

Quoted 76 lines

Quoted 47 lines · 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 <[email protected]>

---
 sound/soc/codecs/rt5640.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

sound/soc/codecs/rt5640.c +7 -7

index adbae1f36a8a..b245c44cafbc 100644
@@ -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);
-	}

sound/soc/codecs/rt5640.c static void rt5640_disable_jack_detect(struct snd_soc_component *component) · line 2451

Quoted 10 lines

sound/soc/codecs/rt5640.c +7 -1

+		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.

sound/soc/codecs/rt5640.c +4 -0

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

Quoted 4 lines

thanks,
Takashi

Okay, I'll make v2.

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.

Dmitry Osipenko

author

2020-01-02 15:57 UTC #4 lore raw

31.12.2019 03:17, Mark Brown пишет:

Quoted 10 lines · Dmitry Osipenko

Quoted 8 lines

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

Quoted 7 lines

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.

Mark Brown

maintainer

2020-01-03 00:54 UTC #5 lore raw

Quoted 8 lines · Dmitry Osipenko

31.12.2019 03:17, Mark Brown пишет:
Quoted 5 lines
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.

Quoted 5 lines

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.

Dmitry Osipenko

author

2020-01-04 00:37 UTC #6 lore raw

03.01.2020 03:54, Mark Brown пишет:

Quoted 15 lines · Dmitry Osipenko

Quoted 8 lines
31.12.2019 03:17, Mark Brown пишет:
Quoted 5 lines
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.
Quoted 5 lines
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.

Quoted 5 lines

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.