| Message ID | 1562989575-33785-3-git-send-email-wen.yang99@zte.com.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2abee12c0ab1924a69993d2c063a39a952e7d836 |
| Headers | show |
| Series | ASoC: samsung: odroid: fix err handling of odroid_audio_probe | expand |
> The cpu_dai variable is still being used after the of_node_put() call, Such an implementation detail is questionable. https://wiki.sei.cmu.edu/confluence/display/c/MEM30-C.+Do+not+access+freed+memory > which may result in double-free: This consequence is also undesirable. https://cwe.mitre.org/data/definitions/415.html Now I wonder if two update steps are really appropriate as a fix instead of using a single update step for the desired correction in this software module. Should a commit (including previous ones) usually be correct by itself? Regards, Markus
> > The cpu_dai variable is still being used after the of_node_put() call, > > Such an implementation detail is questionable. > https://wiki.sei.cmu.edu/confluence/display/c/MEM30-C.+Do+not+access+freed+memory > > > > which may result in double-free: > > This consequence is also undesirable. > https://cwe.mitre.org/data/definitions/415.html > > > Now I wonder if two update steps are really appropriate as a fix > instead of using a single update step for the desired correction > in this software module. > Should a commit (including previous ones) usually be correct by itself? Thanks. These two updates fix two different bugs. One of them is the use-after-free issue introduced by bc3cf17b575a: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=bc3cf17b575a7a97b4af7ddcf86133175da7a582 - ret = snd_soc_of_get_dai_link_codecs(dev, codec, link); + cpu_dai = of_parse_phandle(cpu, "sound-dai", 0); + of_node_put(cpu); + of_node_put(codec); + + ret = snd_soc_of_get_dai_link_codecs(dev, codec, codec_link); if (ret < 0) goto err_put_codec_n; and the other is the double-free issue introduced by d832d2b246c5: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/sound/soc/samsung/odroid.c?id=d832d2b246c516eacb2d0ba53ec17ed59c3cd62b#n318 and n303, n308. So we sent two patches to fix them separately. -- Regards, Wen
> These two updates fix two different bugs. I can follow this view to some degree. > and the other is the double-free issue This programming error affects also the use of data structures which became invalid. https://cwe.mitre.org/data/definitions/415.html#oc_415_Notes > So we sent two patches to fix them separately. You would like to fix something according to two variables (of the data type “device_node *”) in the same function implementation. Please combine these corrections in an update step under a topic like “ASoC: samsung: odroid: Fix handling of device node references in odroid_audio_probe()”. (The previous update step would contain still a known programming mistake otherwise, wouldn't it?) Regards, Markus
On Sat, 13 Jul 2019 at 05:48, Wen Yang <wen.yang99@zte.com.cn> wrote: > > The cpu_dai variable is still being used after the of_node_put() call, > which may result in double-free: > > of_node_put(cpu_dai); ---> released here > > ret = devm_snd_soc_register_card(dev, card); > if (ret < 0) { > ... > goto err_put_clk_i2s; --> jump to err_put_clk_i2s > ... > > err_put_clk_i2s: > clk_put(priv->clk_i2s_bus); > err_put_sclk: > clk_put(priv->sclk_i2s); > err_put_cpu_dai: > of_node_put(cpu_dai); --> double-free here > > Fixes: d832d2b246c5 ("ASoC: samsung: odroid: Fix of_node refcount unbalance") > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Sangbeom Kim <sbkim73@samsung.com> > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> > Cc: Liam Girdwood <lgirdwood@gmail.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Jaroslav Kysela <perex@perex.cz> > Cc: Takashi Iwai <tiwai@suse.com> > Cc: alsa-devel@alsa-project.org > Cc: linux-kernel@vger.kernel.org > --- > sound/soc/samsung/odroid.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
diff --git a/sound/soc/samsung/odroid.c b/sound/soc/samsung/odroid.c index 64ebe89..f0f5fa9 100644 --- a/sound/soc/samsung/odroid.c +++ b/sound/soc/samsung/odroid.c @@ -308,7 +308,6 @@ static int odroid_audio_probe(struct platform_device *pdev) ret = PTR_ERR(priv->clk_i2s_bus); goto err_put_sclk; } - of_node_put(cpu_dai); ret = devm_snd_soc_register_card(dev, card); if (ret < 0) { @@ -316,6 +315,7 @@ static int odroid_audio_probe(struct platform_device *pdev) goto err_put_clk_i2s; } + of_node_put(cpu_dai); of_node_put(codec); return 0;
The cpu_dai variable is still being used after the of_node_put() call, which may result in double-free: of_node_put(cpu_dai); ---> released here ret = devm_snd_soc_register_card(dev, card); if (ret < 0) { ... goto err_put_clk_i2s; --> jump to err_put_clk_i2s ... err_put_clk_i2s: clk_put(priv->clk_i2s_bus); err_put_sclk: clk_put(priv->sclk_i2s); err_put_cpu_dai: of_node_put(cpu_dai); --> double-free here Fixes: d832d2b246c5 ("ASoC: samsung: odroid: Fix of_node refcount unbalance") Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> Cc: Krzysztof Kozlowski <krzk@kernel.org> Cc: Sangbeom Kim <sbkim73@samsung.com> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> Cc: Liam Girdwood <lgirdwood@gmail.com> Cc: Mark Brown <broonie@kernel.org> Cc: Jaroslav Kysela <perex@perex.cz> Cc: Takashi Iwai <tiwai@suse.com> Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org --- sound/soc/samsung/odroid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)