[PATCH] ALSA: hda: fix NULL pointer dereference during suspend

[PATCH] ALSA: hda: fix NULL pointer dereference during suspend

[Ranjani Sridharan]

When the ASoC card registration fails and the codec component driver
never probes, the codec device is not initialized and therefore
memory for codec->wcaps is not allocated. This results in a NULL pointer
dereference when the codec driver suspend callback is invoked during
system suspend. Fix this by returning without performing any actions
during codec suspend/resume if the card was not registered successfully.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/pci/hda/hda_codec.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 3576e2d8452f..9b1f387d18e5 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2936,6 +2936,10 @@ static int hda_codec_runtime_suspend(struct device *dev)
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 	unsigned int state;
 
+	/* Nothing to do if card registration fails and the component driver never probes */
+	if (!codec->card)
+		return 0;
+
 	cancel_delayed_work_sync(&codec->jackpoll_work);
 	state = hda_call_codec_suspend(codec);
 	if (codec->link_down_at_suspend ||
@@ -2950,6 +2954,10 @@ static int hda_codec_runtime_resume(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
+	/* Nothing to do if card registration fails and the component driver never probes */
+	if (!codec->card)
+		return 0;
+
 	codec_display_power(codec, true);
 	snd_hdac_codec_link_up(&codec->core);
 	hda_call_codec_resume(codec);
-- 
2.25.1

Re: [PATCH] ALSA: hda: fix NULL pointer dereference during suspend

[Takashi Iwai]

On Wed, 29 Jul 2020 01:10:11 +0200,
Ranjani Sridharan wrote:
> 
> When the ASoC card registration fails and the codec component driver
> never probes, the codec device is not initialized and therefore
> memory for codec->wcaps is not allocated. This results in a NULL pointer
> dereference when the codec driver suspend callback is invoked during
> system suspend. Fix this by returning without performing any actions
> during codec suspend/resume if the card was not registered successfully.
> 
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The code changes look OK to apply, but I still wonder how the runtime
PM gets invoked even if the device is not instantiated properly?


thanks,

Takashi

> ---
>  sound/pci/hda/hda_codec.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 3576e2d8452f..9b1f387d18e5 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2936,6 +2936,10 @@ static int hda_codec_runtime_suspend(struct device *dev)
>  	struct hda_codec *codec = dev_to_hda_codec(dev);
>  	unsigned int state;
>  
> +	/* Nothing to do if card registration fails and the component driver never probes */
> +	if (!codec->card)
> +		return 0;
> +
>  	cancel_delayed_work_sync(&codec->jackpoll_work);
>  	state = hda_call_codec_suspend(codec);
>  	if (codec->link_down_at_suspend ||
> @@ -2950,6 +2954,10 @@ static int hda_codec_runtime_resume(struct device *dev)
>  {
>  	struct hda_codec *codec = dev_to_hda_codec(dev);
>  
> +	/* Nothing to do if card registration fails and the component driver never probes */
> +	if (!codec->card)
> +		return 0;
> +
>  	codec_display_power(codec, true);
>  	snd_hdac_codec_link_up(&codec->core);
>  	hda_call_codec_resume(codec);
> -- 
> 2.25.1
> 

Re: [PATCH] ALSA: hda: fix NULL pointer dereference during suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

On Wed, Jul 29, 2020 at 09:48:08AM +0200, Takashi Iwai wrote:

> The code changes look OK to apply, but I still wonder how the runtime
> PM gets invoked even if the device is not instantiated properly?

The components are registered and active at the device model level even
if they never get incorporated into a card.

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

Re: [PATCH] ALSA: hda: fix NULL pointer dereference during suspend

[Ranjani Sridharan]

On Wed, 2020-07-29 at 09:48 +0200, Takashi Iwai wrote:
> On Wed, 29 Jul 2020 01:10:11 +0200,
> Ranjani Sridharan wrote:
> > When the ASoC card registration fails and the codec component
> > driver
> > never probes, the codec device is not initialized and therefore
> > memory for codec->wcaps is not allocated. This results in a NULL
> > pointer
> > dereference when the codec driver suspend callback is invoked
> > during
> > system suspend. Fix this by returning without performing any
> > actions
> > during codec suspend/resume if the card was not registered
> > successfully.
> > 
> > Reviewed-by: Pierre-Louis Bossart <
> > pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com
> > >
> 
> The code changes look OK to apply, but I still wonder how the runtime
> PM gets invoked even if the device is not instantiated properly?
Hi Takashi,

Its not runtime PM suspend but rather the system PM suspend callback
that is invoked when the system is suspended that ends up callling the
the runtime PM callback. So, the sequence is:
hda_codec_pm_suspend()
   -> pm_runtime_force_suspend()
          -> hda_codec_runtime_suspend()

Thanks,
Ranjani

Re: [PATCH] ALSA: hda: fix NULL pointer dereference during suspend

[Takashi Iwai]

On Wed, 29 Jul 2020 17:03:22 +0200,
Ranjani Sridharan wrote:
> 
> On Wed, 2020-07-29 at 09:48 +0200, Takashi Iwai wrote:
> > On Wed, 29 Jul 2020 01:10:11 +0200,
> > Ranjani Sridharan wrote:
> > > When the ASoC card registration fails and the codec component
> > > driver
> > > never probes, the codec device is not initialized and therefore
> > > memory for codec->wcaps is not allocated. This results in a NULL
> > > pointer
> > > dereference when the codec driver suspend callback is invoked
> > > during
> > > system suspend. Fix this by returning without performing any
> > > actions
> > > during codec suspend/resume if the card was not registered
> > > successfully.
> > > 
> > > Reviewed-by: Pierre-Louis Bossart <
> > > pierre-louis.bossart@linux.intel.com>
> > > Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com
> > > >
> > 
> > The code changes look OK to apply, but I still wonder how the runtime
> > PM gets invoked even if the device is not instantiated properly?
> Hi Takashi,
> 
> Its not runtime PM suspend but rather the system PM suspend callback
> that is invoked when the system is suspended that ends up callling the
> the runtime PM callback. So, the sequence is:
> hda_codec_pm_suspend()
>    -> pm_runtime_force_suspend()
>           -> hda_codec_runtime_suspend()

OK, but the problem is still same.  The basic problem is that the
hda_codec_driver_probe() is called for the hda_codec object that
hasn't been initialized and bypasses to ext_ops.hdev_attach.

So, we can factor out the fundamental part of
snd_hda_codec_device_new() that is irrelevant with the card object and
call it in hdac_hda_dev_probe().  Most of the legacy HD-audio codec
can work without the card object, including suspend/resume.

But this will be a more intensive surgery, and maybe not much worth
for the corner case, so I already applied your patch as is.


thanks,

Takashi

Re: [PATCH] ALSA: hda: fix NULL pointer dereference during suspend

[Takashi Iwai]

On Wed, 29 Jul 2020 18:06:25 +0200,
Takashi Iwai wrote:
> 
> On Wed, 29 Jul 2020 17:03:22 +0200,
> Ranjani Sridharan wrote:
> > 
> > On Wed, 2020-07-29 at 09:48 +0200, Takashi Iwai wrote:
> > > On Wed, 29 Jul 2020 01:10:11 +0200,
> > > Ranjani Sridharan wrote:
> > > > When the ASoC card registration fails and the codec component
> > > > driver
> > > > never probes, the codec device is not initialized and therefore
> > > > memory for codec->wcaps is not allocated. This results in a NULL
> > > > pointer
> > > > dereference when the codec driver suspend callback is invoked
> > > > during
> > > > system suspend. Fix this by returning without performing any
> > > > actions
> > > > during codec suspend/resume if the card was not registered
> > > > successfully.
> > > > 
> > > > Reviewed-by: Pierre-Louis Bossart <
> > > > pierre-louis.bossart@linux.intel.com>
> > > > Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com
> > > > >
> > > 
> > > The code changes look OK to apply, but I still wonder how the runtime
> > > PM gets invoked even if the device is not instantiated properly?
> > Hi Takashi,
> > 
> > Its not runtime PM suspend but rather the system PM suspend callback
> > that is invoked when the system is suspended that ends up callling the
> > the runtime PM callback. So, the sequence is:
> > hda_codec_pm_suspend()
> >    -> pm_runtime_force_suspend()
> >           -> hda_codec_runtime_suspend()
> 
> OK, but the problem is still same.  The basic problem is that the
> hda_codec_driver_probe() is called for the hda_codec object that
> hasn't been initialized and bypasses to ext_ops.hdev_attach.
> 
> So, we can factor out the fundamental part of
> snd_hda_codec_device_new() that is irrelevant with the card object and
> call it in hdac_hda_dev_probe().

I meant something like below (totally untested)


Takashi


---
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index f4cc364d837f..1f01e4d6b923 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -303,10 +303,11 @@ struct hda_codec {
 /*
  * constructors
  */
-int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
-		      unsigned int codec_addr, struct hda_codec **codecp);
-int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
-		      unsigned int codec_addr, struct hda_codec *codec);
+int snd_hda_codec_new(struct hda_bus *bus, unsigned int codec_addr,
+		      struct hda_codec **codecp);
+int snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr,
+			      struct hda_codec *codec);
+int snd_hda_codec_assign_card(struct hda_codec *codec);
 int snd_hda_codec_configure(struct hda_codec *codec);
 int snd_hda_codec_update_widgets(struct hda_codec *codec);
 
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 40f3c175954d..3079d32ba64d 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -867,15 +867,13 @@ static void snd_hda_codec_dev_release(struct device *dev)
 
 #define DEV_NAME_LEN 31
 
-static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card,
-			unsigned int codec_addr, struct hda_codec **codecp)
+static int hda_codec_new(struct hda_bus *bus, unsigned int card_number,
+			 unsigned int codec_addr, struct hda_codec **codecp)
 {
 	char name[DEV_NAME_LEN];
 	struct hda_codec *codec;
 	int err;
 
-	dev_dbg(card->dev, "%s: entry\n", __func__);
-
 	if (snd_BUG_ON(!bus))
 		return -EINVAL;
 	if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS))
@@ -885,7 +883,7 @@ static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card,
 	if (!codec)
 		return -ENOMEM;
 
-	sprintf(name, "hdaudioC%dD%d", card->number, codec_addr);
+	sprintf(name, "hdaudioC%dD%d", card_number, codec_addr);
 	err = snd_hdac_device_init(&codec->core, &bus->core, name, codec_addr);
 	if (err < 0) {
 		kfree(codec);
@@ -901,37 +899,41 @@ static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card,
 /**
  * snd_hda_codec_new - create a HDA codec
  * @bus: the bus to assign
- * @card: card for this codec
  * @codec_addr: the codec address
  * @codecp: the pointer to store the generated codec
  *
  * Returns 0 if successful, or a negative error code.
  */
-int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
-		      unsigned int codec_addr, struct hda_codec **codecp)
+int snd_hda_codec_new(struct hda_bus *bus, unsigned int codec_addr,
+		      struct hda_codec **codecp)
 {
 	int ret;
 
-	ret = snd_hda_codec_device_init(bus, card, codec_addr, codecp);
+	ret = hda_codec_new(bus, bus->card->number, codec_addr, codecp);
 	if (ret < 0)
 		return ret;
 
-	return snd_hda_codec_device_new(bus, card, codec_addr, *codecp);
+	ret = snd_hda_codec_device_init(bus, codec_addr, *codecp);
+	if (ret < 0)
+		goto error;
+
+	ret = snd_hda_codec_assign_card(*codecp);
+	if (ret < 0)
+		goto error;
+
+	return 0;
+
+ error:
+	 put_device(hda_codec_dev(*codecp));
+	 return ret;
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_new);
 
-int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
-			unsigned int codec_addr, struct hda_codec *codec)
+int snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr,
+			      struct hda_codec *codec)
 {
-	char component[31];
 	hda_nid_t fg;
 	int err;
-	static const struct snd_device_ops dev_ops = {
-		.dev_register = snd_hda_codec_dev_register,
-		.dev_free = snd_hda_codec_dev_free,
-	};
-
-	dev_dbg(card->dev, "%s: entry\n", __func__);
 
 	if (snd_BUG_ON(!bus))
 		return -EINVAL;
@@ -942,7 +944,6 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
 	codec->core.exec_verb = codec_exec_verb;
 
 	codec->bus = bus;
-	codec->card = card;
 	codec->addr = codec_addr;
 	mutex_init(&codec->spdif_mutex);
 	mutex_init(&codec->control_mutex);
@@ -965,47 +966,50 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
 	codec->power_jiffies = jiffies;
 #endif
 
-	snd_hda_sysfs_init(codec);
-
 	if (codec->bus->modelname) {
 		codec->modelname = kstrdup(codec->bus->modelname, GFP_KERNEL);
-		if (!codec->modelname) {
-			err = -ENOMEM;
-			goto error;
-		}
+		if (!codec->modelname)
+			return -ENOMEM;
 	}
 
 	fg = codec->core.afg ? codec->core.afg : codec->core.mfg;
 	err = read_widget_caps(codec, fg);
 	if (err < 0)
-		goto error;
+		return err;
 	err = read_pin_defaults(codec);
 	if (err < 0)
-		goto error;
+		return err;
 
 	/* power-up all before initialization */
 	hda_set_power_state(codec, AC_PWRST_D0);
 	codec->core.dev.power.power_state = PMSG_ON;
 
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hda_codec_device_init);
+
+int snd_hda_codec_assign_card(struct hda_codec *codec)
+{
+	static const struct snd_device_ops dev_ops = {
+		.dev_register = snd_hda_codec_dev_register,
+		.dev_free = snd_hda_codec_dev_free,
+	};
+	char component[31];
+
+	codec->card = codec->bus->card;
 	snd_hda_codec_proc_new(codec);
 
+	snd_hda_sysfs_init(codec);
+
 	snd_hda_create_hwdep(codec);
 
 	sprintf(component, "HDA:%08x,%08x,%08x", codec->core.vendor_id,
 		codec->core.subsystem_id, codec->core.revision_id);
-	snd_component_add(card, component);
-
-	err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);
-	if (err < 0)
-		goto error;
-
-	return 0;
+	snd_component_add(codec->card, component);
 
- error:
-	put_device(hda_codec_dev(codec));
-	return err;
+	return snd_device_new(codec->card, SNDRV_DEV_CODEC, codec, &dev_ops);
 }
-EXPORT_SYMBOL_GPL(snd_hda_codec_device_new);
+EXPORT_SYMBOL_GPL(snd_hda_codec_assign_card);
 
 /**
  * snd_hda_codec_update_widgets - Refresh widget caps and pin defaults
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 80016b7b6849..e68ca57be30e 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -1213,6 +1213,7 @@ EXPORT_SYMBOL_GPL(azx_bus_init);
 int azx_probe_codecs(struct azx *chip, unsigned int max_slots)
 {
 	struct hdac_bus *bus = azx_bus(chip);
+	struct hda_codec *codec;
 	int c, codecs, err;
 
 	codecs = 0;
@@ -1245,10 +1246,10 @@ int azx_probe_codecs(struct azx *chip, unsigned int max_slots)
 	/* Then create codec instances */
 	for (c = 0; c < max_slots; c++) {
 		if ((bus->codec_mask & (1 << c)) & chip->codec_probe_mask) {
-			struct hda_codec *codec;
-			err = snd_hda_codec_new(&chip->bus, chip->card, c, &codec);
+			err = snd_hda_codec_new(&chip->bus, c, &codec);
 			if (err < 0)
 				continue;
+
 			codec->jackpoll_interval = chip->jackpoll_interval;
 			codec->beep_mode = chip->beep_mode;
 			codecs++;
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
index 473efe9ef998..298d9c46d85d 100644
--- a/sound/soc/codecs/hdac_hda.c
+++ b/sound/soc/codecs/hdac_hda.c
@@ -417,18 +417,12 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component)
 		snd_hdac_display_power(hdev->bus,
 				       HDA_CODEC_IDX_CONTROLLER, true);
 
-	ret = snd_hda_codec_device_new(hcodec->bus, component->card->snd_card,
-				       hdev->addr, hcodec);
+	hcodec->bus->card = dapm->card->snd_card;
+	ret = snd_hda_codec_assign_card(hcodec);
 	if (ret < 0) {
 		dev_err(&hdev->dev, "failed to create hda codec %d\n", ret);
 		goto error_no_pm;
 	}
-	/*
-	 * Overwrite type to HDA_DEV_ASOC since it is a ASoC driver
-	 * hda_codec.c will check this flag to determine if unregister
-	 * device is needed.
-	 */
-	hdev->type = HDA_DEV_ASOC;
 
 	/*
 	 * snd_hda_codec_device_new decrements the usage count so call get pm
@@ -436,8 +430,6 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component)
 	 */
 	pm_runtime_get_noresume(&hdev->dev);
 
-	hcodec->bus->card = dapm->card->snd_card;
-
 	ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
 	if (ret < 0) {
 		dev_err(&hdev->dev, "name failed %s\n", hcodec->preset->name);
@@ -574,6 +566,7 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev)
 {
 	struct hdac_ext_link *hlink;
 	struct hdac_hda_priv *hda_pvt;
+	struct hda_codec *hcodec;
 	int ret;
 
 	/* hold the ref while we probe */
@@ -588,6 +581,18 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev)
 	if (!hda_pvt)
 		return -ENOMEM;
 
+	ret = snd_hda_codec_device_init(hcodec->bus, hdev->addr,
+					&hda_pvt->codec);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Overwrite type to HDA_DEV_ASOC since it is a ASoC driver
+	 * hda_codec.c will check this flag to determine if unregister
+	 * device is needed.
+	 */
+	hdev->type = HDA_DEV_ASOC;
+
 	/* ASoC specific initialization */
 	ret = devm_snd_soc_register_component(&hdev->dev,
 					 &hdac_hda_codec, hdac_hda_dais,

Re: [PATCH] ALSA: hda: fix NULL pointer dereference during suspend

[Ranjani Sridharan]

On Thu, 2020-07-30 at 15:07 +0200, Takashi Iwai wrote:
> On Wed, 29 Jul 2020 18:06:25 +0200,
> Takashi Iwai wrote:
> > On Wed, 29 Jul 2020 17:03:22 +0200,
> > Ranjani Sridharan wrote:
> > > On Wed, 2020-07-29 at 09:48 +0200, Takashi Iwai wrote:
> > > > On Wed, 29 Jul 2020 01:10:11 +0200,
> > > > Ranjani Sridharan wrote:
> > > > > When the ASoC card registration fails and the codec component
> > > > > driver
> > > > > never probes, the codec device is not initialized and
> > > > > therefore
> > > > > memory for codec->wcaps is not allocated. This results in a
> > > > > NULL
> > > > > pointer
> > > > > dereference when the codec driver suspend callback is invoked
> > > > > during
> > > > > system suspend. Fix this by returning without performing any
> > > > > actions
> > > > > during codec suspend/resume if the card was not registered
> > > > > successfully.
> > > > > 
> > > > > Reviewed-by: Pierre-Louis Bossart <
> > > > > pierre-louis.bossart@linux.intel.com>
> > > > > Signed-off-by: Ranjani Sridharan <
> > > > > ranjani.sridharan@linux.intel.com
> > > > 
> > > > The code changes look OK to apply, but I still wonder how the
> > > > runtime
> > > > PM gets invoked even if the device is not instantiated
> > > > properly?
> > > Hi Takashi,
> > > 
> > > Its not runtime PM suspend but rather the system PM suspend
> > > callback
> > > that is invoked when the system is suspended that ends up
> > > callling the
> > > the runtime PM callback. So, the sequence is:
> > > hda_codec_pm_suspend()
> > >    -> pm_runtime_force_suspend()
> > >           -> hda_codec_runtime_suspend()
> > 
> > OK, but the problem is still same.  The basic problem is that the
> > hda_codec_driver_probe() is called for the hda_codec object that
> > hasn't been initialized and bypasses to ext_ops.hdev_attach.
> > 
> > So, we can factor out the fundamental part of
> > snd_hda_codec_device_new() that is irrelevant with the card object
> > and
> > call it in hdac_hda_dev_probe().
> 
> I meant something like below (totally untested)
Thanks, Takashi. I can test this out and get back to you by next week.

Thanks,
Ranjani