linux-kernel - Broken child-node lookup in sound/soc/mediatek/mt8173

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20171113113252.GT11226@localhost>
Date:   Mon, 13 Nov 2017 12:32:52 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Mark Brown <broonie@...nel.org>, PC Liao <pc.liao@...iatek.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        alsa-devel@...a-project.org, linux-mediatek@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Broken child-node lookup in sound/soc/mediatek/mt8173

Hi,

I'm trying to fix up incorrect usage of of_find_node_by_name() to lookup
child nodes, and found the following code in
sound/soc/mediatek/mt8173/mt8173-rt5650.c:

static int mt8173_rt5650_dev_probe(struct platform_device *pdev)
{
	...

	platform_node = of_parse_phandle(pdev->dev.of_node,
					 "mediatek,platform", 0);
	...

	if (of_find_node_by_name(platform_node, "codec-capture")) {
		np = of_get_child_by_name(pdev->dev.of_node, "codec-capture");
		if (!np) {
			dev_err(&pdev->dev,
				"%s: Can't find codec-capture DT node\n",
				__func__);
			return -EINVAL;
		}
		ret = snd_soc_of_get_dai_name(np, &codec_capture_dai);
		if (ret < 0) {
			dev_err(&pdev->dev,
				"%s codec_capture_dai name fail %d\n",
				__func__, ret);
			return ret;
		}
		mt8173_rt5650_codecs[1].dai_name = codec_capture_dai;
	}

added by commit d349caeb0510 ("ASoC: mediatek: Add second I2S on
mt8173-rt5650 machine driver").

First of all the "codec-capture" node is indeed documented as a child
node of the sound node, so the tree-wide depth-first search from the
platform_node looks entirely bogus.

Note that of_find_node_by_name() also drops a reference to its first
argument, in this case the sound node, which could end up being
prematurely freed. 

And then the reference to any returned codec-capture node (from either
lookup) is never dropped.

And since support for this second codec was added retrospectively and is
documented as optional, that -EINVAL in case the node is missing looks
broken too.

I figured I better just report this one to the author of the patch and
the maintainers to be straightened out.

Thanks,
Johan