Re: [PATCH v6 5/5] ASoC: airoha: Add machine driver for Airoha AN7581

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Global static structures (`an7581_wm8960_card` and `an7581_wm8960_dai_links`) are modified during probe without protection, leading to state corruption and potential Use-After-Free.
- [Medium] OF node references acquired via `of_parse_phandle()` are leaked on the successful probe path and never released on driver unbind.
- [Low] Duplicate platform components are defined and bound to each DAI link.
--

commit 7051186115da5c8ed0db19bf2c3b4aac46840edd
Author: Christian Marangi [off-list ref]

ASoC: airoha: Add machine driver for Airoha AN7581
    
Add support for the machine driver for Airoha AN7581 paired with the WM8960
i2c codec. This driver creates the sound card for the AFE driver.

[ ... ]

+SND_SOC_DAILINK_DEFS(playback,
+		     DAILINK_COMP_ARRAY(COMP_CPU("DL1")),
+		     DAILINK_COMP_ARRAY(COMP_DUMMY()),
+		     DAILINK_COMP_ARRAY(COMP_EMPTY(), COMP_EMPTY()));

[Severity: Low]
Are two platform components necessary for this DAI link?

The code defines two empty platform components here, and later assigns the
exact same device node to both of them.

[ ... ]

+static int an7581_wm8960_machine_probe(struct platform_device *pdev)
+{
+	struct device_node *platform_dai_node, *codec_dai_node;
+	struct snd_soc_card *card = &an7581_wm8960_card;
+	struct device_node *of_platform, *codec;
+	struct snd_soc_dai_link *dai_link;
+	int i, d, ret;
+
+	card->dev = &pdev->dev;

[Severity: High]
Does this code modify a global static structure during probe?

The an7581_wm8960_card structure is statically defined, but its dev pointer
is overwritten with a per-device pointer here.

If the driver is unbound and rebound, or if multiple instances of this device
exist, could this lead to state corruption or a use-after-free when
device-managed memory is released?

+
+	of_platform = of_get_child_by_name(pdev->dev.of_node, "platform");
+
+	if (of_platform) {
+		platform_dai_node = of_parse_phandle(of_platform, "sound-dai", 0);
+		of_node_put(of_platform);

[ ... ]

+	for_each_card_prelinks(card, i, dai_link) {
+		struct snd_soc_dai_link_component *platform;
+
+		dai_link->num_platforms = 2;
+		for_each_link_platforms(dai_link, d, platform) {
+			if (platform->name)
+				continue;
+			platform->of_node = platform_dai_node;

[Severity: Low]
Does this code assign the exact same platform_dai_node to both platform
components?

Since dai_link->num_platforms is 2 and we have two empty platform components
defined, this will bind the same component twice.

+		}
+	}
+
+	codec = of_get_child_by_name(pdev->dev.of_node, "codec");
+
+	if (codec) {
+		codec_dai_node = of_parse_phandle(codec, "sound-dai", 0);

[ ... ]

+	for_each_card_prelinks(card, i, dai_link) {
+		if (dai_link->codecs->name)
+			continue;
+		dai_link->codecs->of_node = codec_dai_node;

[Severity: High]
Does this code modify the globally shared an7581_wm8960_dai_links array?

Writing a per-device codec_dai_node to a global structure could cause issues
if the device is unbound, leading to dangling pointers for future probes.

+	}
+
+	ret = snd_soc_of_parse_audio_routing(card, "audio-routing");

[Severity: High]
Does this introduce a use-after-free risk?

The snd_soc_of_parse_audio_routing function allocates device-managed memory
for the routes and stores the pointer in the global static
card->of_dapm_routes.

If the device is unbound, this memory will be freed, but the global structure
will still hold a dangling pointer.

+	if (ret) {
+		dev_err(&pdev->dev, "Failed to parse audio-routing: %d\n", ret);
+		goto err_of_node_put;
+	}
+
+	ret = devm_snd_soc_register_card(&pdev->dev, card);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "%s snd_soc_register_card fail\n", __func__);
+		goto err_of_node_put;
+	}
+
+	return 0;

[Severity: Medium]
Will this successful return path leak the platform_dai_node and
codec_dai_node references?

The references acquired via of_parse_phandle() earlier in the function are
not released on success.

Since there is no .remove callback or device-managed cleanup action registered
to release them upon driver unbind, could these device node references be
permanently lost?

+
+err_of_node_put:
+	of_node_put(platform_dai_node);
+	of_node_put(codec_dai_node);
+	return ret;
+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908090448.3332-1-ansuelsmth@gmail.com?part=5