[PATCH] ASoC: hdac: make SOF HDA codec driver probe deterministic

[PATCH] ASoC: hdac: make SOF HDA codec driver probe deterministic

[Kai Vehmanen]

To provide backward compatibility to older systems, the SOF HDA driver
allows user to specify which HDMI codec driver to use at runtime via
kernel parameter. This mechanism has a subtle flaw in that it assumes
the codec drivers not to be loaded when the SOF PCI driver is loaded.

The problem is rooted in use of the hdev->type field.
snd_hdac_ext_bus_device_init() initializes this field to HDA_DEV_ASOC.
This signals the HDA core that ASoC drivers should be considered in
driver matching (hda_bus_match()). The SOF and SST drivers continue by
overriding this field to HDA_DEV_LEGACY and proceeding to load driver
modules with request_module(). Correct drivers will get loaded and
attached.

If however the codec drivers are already loaded when
snd_hdac_ext_bus_device_init() is called, the matching will not work as
expected as device type is still set to HDA_DEV_ASOC. Specifically if
hdac-hdmi is attached when machine driver is configured to use hdac-hda,
this leads to out-of-bounds memory access in
hda_dsp_hdmi_build_controls().

Fix the issue by adding codec type as a parameter to
snd_hdac_ext_bus_device_init() and ensuring type is set correctly from
the start.

BugLink: https://github.com/thesofproject/linux/issues/2311
Fixes: 139c7febad1a ("ASoC: SOF: Intel: add support for snd-hda-codec-hdmi")
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/hdaudio_ext.h     |  2 +-
 sound/hda/ext/hdac_ext_bus.c    |  5 +++--
 sound/soc/intel/skylake/skl.c   |  4 ++--
 sound/soc/sof/intel/hda-codec.c | 17 ++++++++---------
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index ef88b20c7b0a..7abf74c1c474 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -10,7 +10,7 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 
 void snd_hdac_ext_bus_exit(struct hdac_bus *bus);
 int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
-						struct hdac_device *hdev);
+				struct hdac_device *hdev, int type);
 void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev);
 void snd_hdac_ext_bus_device_remove(struct hdac_bus *bus);
 
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
index d0a604c939df..765c40a6ccba 100644
--- a/sound/hda/ext/hdac_ext_bus.c
+++ b/sound/hda/ext/hdac_ext_bus.c
@@ -70,11 +70,12 @@ static void default_release(struct device *dev)
  * @bus: hdac bus to attach to
  * @addr: codec address
  * @hdev: hdac device to init
+ * @type: codec type (HDAC_DEV_*) to use for this device
  *
  * Returns zero for success or a negative error code.
  */
 int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
-					struct hdac_device *hdev)
+				 struct hdac_device *hdev, int type)
 {
 	char name[15];
 	int ret;
@@ -88,7 +89,7 @@ int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
 		dev_err(bus->dev, "device init failed for hdac device\n");
 		return ret;
 	}
-	hdev->type = HDA_DEV_ASOC;
+	hdev->type = type;
 	hdev->dev.release = default_release;
 
 	ret = snd_hdac_device_register(hdev);
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index ea00cf61d1a8..8b993722f74e 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -721,7 +721,7 @@ static int probe_codec(struct hdac_bus *bus, int addr)
 	hda_codec->codec.bus = skl_to_hbus(skl);
 	hdev = &hda_codec->codec.core;
 
-	err = snd_hdac_ext_bus_device_init(bus, addr, hdev);
+	err = snd_hdac_ext_bus_device_init(bus, addr, hdev, HDA_DEV_ASOC);
 	if (err < 0)
 		return err;
 
@@ -736,7 +736,7 @@ static int probe_codec(struct hdac_bus *bus, int addr)
 	if (!hdev)
 		return -ENOMEM;
 
-	return snd_hdac_ext_bus_device_init(bus, addr, hdev);
+	return snd_hdac_ext_bus_device_init(bus, addr, hdev, HDA_DEV_ASOC);
 #endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */
 }
 
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index f6ba3b593e1f..6875fa570c2c 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -117,6 +117,7 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
 	struct hdac_hda_priv *hda_priv;
 	struct hda_codec *codec;
+	int type = HDA_DEV_LEGACY;
 #endif
 	struct hda_bus *hbus = sof_to_hbus(sdev);
 	struct hdac_device *hdev;
@@ -143,7 +144,11 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 	hdev = &hda_priv->codec.core;
 	codec = &hda_priv->codec;
 
-	ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev);
+	/* only probe ASoC codec drivers for HDAC-HDMI */
+	if (!hda_codec_use_common_hdmi && (resp & 0xFFFF0000) == IDISP_VID_INTEL)
+		type = HDA_DEV_ASOC;
+
+	ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev, type);
 	if (ret < 0)
 		return ret;
 
@@ -161,13 +166,7 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 	else
 		codec->probe_id = 0;
 
-	/*
-	 * if common HDMI codec driver is not used, codec load
-	 * is skipped here and hdac_hdmi is used instead
-	 */
-	if (hda_codec_use_common_hdmi ||
-	    (resp & 0xFFFF0000) != IDISP_VID_INTEL) {
-		hdev->type = HDA_DEV_LEGACY;
+	if (type == HDA_DEV_LEGACY) {
 		ret = hda_codec_load_module(codec);
 		/*
 		 * handle ret==0 (no driver bound) as an error, but pass
@@ -188,7 +187,7 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 	if (!hdev)
 		return -ENOMEM;
 
-	ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev);
+	ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev, HDA_DEV_ASOC);
 
 	return ret;
 #endif
-- 
2.27.0

Re: [PATCH] ASoC: hdac: make SOF HDA codec driver probe deterministic

[Mark Brown]

On Mon, 21 Sep 2020 13:08:41 +0300, Kai Vehmanen wrote:
> To provide backward compatibility to older systems, the SOF HDA driver
> allows user to specify which HDMI codec driver to use at runtime via
> kernel parameter. This mechanism has a subtle flaw in that it assumes
> the codec drivers not to be loaded when the SOF PCI driver is loaded.
> 
> The problem is rooted in use of the hdev->type field.
> snd_hdac_ext_bus_device_init() initializes this field to HDA_DEV_ASOC.
> This signals the HDA core that ASoC drivers should be considered in
> driver matching (hda_bus_match()). The SOF and SST drivers continue by
> overriding this field to HDA_DEV_LEGACY and proceeding to load driver
> modules with request_module(). Correct drivers will get loaded and
> attached.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: hdac: make SOF HDA codec driver probe deterministic
      commit: 163cd1059a85d225b811ddb4192fabd1553f77f1

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark