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

[Takashi Iwai <tiwai@suse.de>]

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,