ASoC: SOF: Intel: hda: Fix UAF when reloading module

[bardliao]

hda_generic_machine_select() appends -idisp to the tplg filename by allocating a new string with devm_kasprintf(), then stores the string right back into the global variable snd_soc_acpi_intel_hda_machines. When the module is unloaded, this memory is freed, resulting in a global variable pointing to freed memory. Reloading the modules then triggers a use-after-free:

BUG: KFENCE: use-after-free read in string+0x48/0xe0

Use-after-free read at 0x00000000967e0109 (in kfence-#99):
string+0x48/0xe0
vsnprintf+0x329/0x6e0
devm_kvasprintf+0x54/0xb0
devm_kasprintf+0x58/0x80
hda_machine_select.cold+0x198/0x17a2 [snd_sof_intel_hda_generic]
sof_probe_work+0x7f/0x600 [snd_sof]
process_one_work+0x17b/0x330
worker_thread+0x2ce/0x3f0
kthread+0xcf/0x100
ret_from_fork+0x31/0x50
ret_from_fork_asm+0x1a/0x30

kfence-#99: 0x00000000198a940f-0x00000000ace47d9d, size=64, cache=kmalloc-64

allocated by task 333 on cpu 8 at 17.798069s (130.453553s ago):
devm_kmalloc+0x52/0x120
devm_kvasprintf+0x66/0xb0
devm_kasprintf+0x58/0x80
hda_machine_select.cold+0x198/0x17a2 [snd_sof_intel_hda_generic]
sof_probe_work+0x7f/0x600 [snd_sof]
process_one_work+0x17b/0x330
worker_thread+0x2ce/0x3f0
kthread+0xcf/0x100
ret_from_fork+0x31/0x50
ret_from_fork_asm+0x1a/0x30

freed by task 1543 on cpu 4 at 141.586686s (6.665010s ago):
release_nodes+0x43/0xb0
devres_release_all+0x90/0xf0
device_unbind_cleanup+0xe/0x70
device_release_driver_internal+0x1c1/0x200
driver_detach+0x48/0x90
bus_remove_driver+0x6d/0xf0
pci_unregister_driver+0x42/0xb0
__do_sys_delete_module+0x1d1/0x310
do_syscall_64+0x82/0x190
entry_SYSCALL_64_after_hwframe+0x76/0x7e

Fix it by saving the filename in pdata->tplg_filename instead, just like every other code path that appends to the tplg filename.

Fixes: 5458411 ("ASoC: SOF: Intel: hda: refactoring topology name fixup for HDA mach")

[Copilot]

Pull Request Overview

This PR fixes a use-after-free bug by updating the assignment of the topology filename so that the memory remains valid when the module is reloaded.

• Update assignment in hda_generic_machine_select to store tplg_filename in pdata->tplg_filename.
• Aligns the filename management with other code paths to prevent freeing memory still in use.

[Copilot]

This change correctly stores the topology filename in pdata, avoiding the previous use-after-free issue when the global variable was freed during module unload.

Suggested change

	pdata->tplg_filename = tplg_filename;
	}
	pdata->tplg_filename = kstrdup(tplg_filename, GFP_KERNEL);
	if (!pdata->tplg_filename) {
	dev_err(sdev->dev, "Failed to allocate memory for tplg_filename\n");
	return;
	}

[bardliao]

@tavianator May I know how should I reproduce the issue? I am not sure if I did it right, but I can't reproduce the issue if I run sudo modprobe -r snd-sof-intel-hda-generic and sudo modprobe snd_sof_intel_hda_generic to reload the module.
And IMHO, the fix will break the existing tplg name fixup mechanism. Please see below in https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/intel/hda.c#L1337

		if (!sof_pdata->tplg_filename) {
			/* remove file extension if it exists */
			tplg_filename = remove_file_ext(mach->sof_tplg_filename);
			if (!tplg_filename)
				return NULL;

			sof_pdata->tplg_filename = tplg_filename;
			tplg_fixup = true;
		}

With your change, pdata->tplg_filename is set and as a result, we won't do tplg fixup for other components.

[bardliao]

@tavianator Can you check if below change helps?

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index b34e5fdf10f1..783cc6396517 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -1333,6 +1333,7 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev)
                                return NULL;

                        sof_pdata->tplg_filename = tplg_filename;
+                       mach->sof_tplg_filename = NULL;
                        tplg_fixup = true;
                }

[tavianator]

@tavianator May I know how should I reproduce the issue? I am not sure if I did it right, but I can't reproduce the issue if I run sudo modprobe -r snd-sof-intel-hda-generic and sudo modprobe snd_sof_intel_hda_generic to reload the module.

I triggered it by unloading and reloading snd_sof_pci_intel_tgl. Original report here: https://lore.kernel.org/linux-sound/CABg4E-nVdZa9oOi3a6FYQ60Wc93aq_8CpO2J4nZxVv94o98quw@mail.gmail.com/T/

# dmesg -t | grep tplg
sof-audio-pci-intel-tgl 0000:00:1f.3:  Topology file: intel/sof-tplg/sof-hda-generic-idisp-4ch.tplg
# modprobe -r snd_sof_pci_intel_tgl
# modprobe snd_sof_pci_intel_tgl
# dmesg -t | grep tplg
sof-audio-pci-intel-tgl 0000:00:1f.3:  Topology file: intel/sof-tplg/sof-hda-generic-idisp-4ch.tplg
sof-audio-pci-intel-tgl 0000:00:1f.3:  Topology file: intel/sof-tplg/sof-hda-generic-idisp-idisp-4ch.tplg
# modprobe -r snd_sof_pci_intel_tgl
# modprobe snd_sof_pci_intel_tgl
# dmesg -t | grep tplg
sof-audio-pci-intel-tgl 0000:00:1f.3:  Topology file: intel/sof-tplg/sof-hda-generic-idisp-4ch.tplg
sof-audio-pci-intel-tgl 0000:00:1f.3:  Topology file: intel/sof-tplg/sof-hda-generic-idisp-idisp-4ch.tplg
sof-audio-pci-intel-tgl 0000:00:1f.3:  Topology file: intel/sof-tplg/\xa0\x11\xaf\x0e\x8d\x8e\xff\xff\xb0\x11\xaf\x0e\x8d\x8e\xff\xff\xb0\x11\xaf\x0e\x8d\x8e\xff\xff-idisp-4ch.tplg

And IMHO, the fix will break the existing tplg name fixup mechanism. Please see below in https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/intel/hda.c#L1337

Right, I see that now. I was trying to copy how the fixups work in hda_machine_select(), but I missed that by setting pdata->tplg_filename in hda_generic_machine_select(), it will keep tplg_fixup = false in hda_machine_select().

@tavianator Can you check if below change helps?

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index b34e5fdf10f1..783cc6396517 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -1333,6 +1333,7 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev)
                                return NULL;

                        sof_pdata->tplg_filename = tplg_filename;
+                       mach->sof_tplg_filename = NULL;
                        tplg_fixup = true;
                }

I suspect that will crash on reloading because hda_mach->sof_tplg_filename is not checked for NULL here:

linux/sound/soc/sof/intel/hda.c

Lines 1072 to 1081 in 0f593a6

	if (tplg_fixup &&
	codec_num == 1 && HDA_IDISP_CODEC(bus->codec_mask)) {
	tplg_filename = devm_kasprintf(sdev->dev, GFP_KERNEL,
	"%s-idisp",
	hda_mach->sof_tplg_filename);
	if (!tplg_filename)
	return;
	hda_mach->sof_tplg_filename = tplg_filename;
	}

[bardliao]

@ujfalusi has a better solution that use a local copy. It may be more efficient to discuss here. :)

[tavianator]

@ujfalusi has a better solution that use a local copy. It may be more efficient to discuss here. :)

Oh I saw that, latest patch here: #5407 (comment)

Should I be submitting patches here instead of the list?

[bardliao]

@ujfalusi has a better solution that use a local copy. It may be more efficient to discuss here. :)

Oh I saw that, latest patch here: #5407 (comment)

Should I be submitting patches here instead of the list?

I saw Peter has acked to your patch. Since you are aligned, I think we are good.