Skip to content

Commit

Permalink
ASoC: mediatek: mt8186: Fix use-after-free in driver remove path
Browse files Browse the repository at this point in the history
[ Upstream commit a93d2af ]

When devm runs function in the "remove" path for a device it runs them
in the reverse order. That means that if you have parts of your driver
that aren't using devm or are using "roll your own" devm w/
devm_add_action_or_reset() you need to keep that in mind.

The mt8186 audio driver didn't quite get this right. Specifically, in
mt8186_init_clock() it called mt8186_audsys_clk_register() and then
went on to call a bunch of other devm function. The caller of
mt8186_init_clock() used devm_add_action_or_reset() to call
mt8186_deinit_clock() but, because of the intervening devm functions,
the order was wrong.

Specifically at probe time, the order was:
1. mt8186_audsys_clk_register()
2. afe_priv->clk = devm_kcalloc(...)
3. afe_priv->clk[i] = devm_clk_get(...)

At remove time, the order (which should have been 3, 2, 1) was:
1. mt8186_audsys_clk_unregister()
3. Free all of afe_priv->clk[i]
2. Free afe_priv->clk

The above seemed to be causing a use-after-free. Luckily, it's easy to
fix this by simply using devm more correctly. Let's move the
devm_add_action_or_reset() to the right place. In addition to fixing
the use-after-free, code inspection shows that this fixes a leak
(missing call to mt8186_audsys_clk_unregister()) that would have
happened if any of the syscon_regmap_lookup_by_phandle() calls in
mt8186_init_clock() had failed.

Fixes: 55b423d ("ASoC: mediatek: mt8186: support audio clock control in platform driver")
Signed-off-by: Douglas Anderson <dianders@chromium.org
Link: https://lore.kernel.org/r/20230511092437.1.I31cceffc8c45bb1af16eb613e197b3df92cdc19e@changeid
Signed-off-by: Mark Brown <broonie@kernel.org
Signed-off-by: Sasha Levin <sashal@kernel.org>
  • Loading branch information
dianders authored and gregkh committed May 24, 2023
1 parent 15b71ad commit dffd9e2
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 34 deletions.
6 changes: 0 additions & 6 deletions sound/soc/mediatek/mt8186/mt8186-afe-clk.c
Expand Up @@ -644,9 +644,3 @@ int mt8186_init_clock(struct mtk_base_afe *afe)

return 0;
}

void mt8186_deinit_clock(void *priv)
{
struct mtk_base_afe *afe = priv;
mt8186_audsys_clk_unregister(afe);
}
1 change: 0 additions & 1 deletion sound/soc/mediatek/mt8186/mt8186-afe-clk.h
Expand Up @@ -81,7 +81,6 @@ enum {
struct mtk_base_afe;
int mt8186_set_audio_int_bus_parent(struct mtk_base_afe *afe, int clk_id);
int mt8186_init_clock(struct mtk_base_afe *afe);
void mt8186_deinit_clock(void *priv);
int mt8186_afe_enable_cgs(struct mtk_base_afe *afe);
void mt8186_afe_disable_cgs(struct mtk_base_afe *afe);
int mt8186_afe_enable_clock(struct mtk_base_afe *afe);
Expand Down
4 changes: 0 additions & 4 deletions sound/soc/mediatek/mt8186/mt8186-afe-pcm.c
Expand Up @@ -2848,10 +2848,6 @@ static int mt8186_afe_pcm_dev_probe(struct platform_device *pdev)
return ret;
}

ret = devm_add_action_or_reset(dev, mt8186_deinit_clock, (void *)afe);
if (ret)
return ret;

/* init memif */
afe->memif_32bit_supported = 0;
afe->memif_size = MT8186_MEMIF_NUM;
Expand Down
46 changes: 24 additions & 22 deletions sound/soc/mediatek/mt8186/mt8186-audsys-clk.c
Expand Up @@ -84,6 +84,29 @@ static const struct afe_gate aud_clks[CLK_AUD_NR_CLK] = {
GATE_AUD2(CLK_AUD_ETDM_OUT1_BCLK, "aud_etdm_out1_bclk", "top_audio", 24),
};

static void mt8186_audsys_clk_unregister(void *data)
{
struct mtk_base_afe *afe = data;
struct mt8186_afe_private *afe_priv = afe->platform_priv;
struct clk *clk;
struct clk_lookup *cl;
int i;

if (!afe_priv)
return;

for (i = 0; i < CLK_AUD_NR_CLK; i++) {
cl = afe_priv->lookup[i];
if (!cl)
continue;

clk = cl->clk;
clk_unregister_gate(clk);

clkdev_drop(cl);
}
}

int mt8186_audsys_clk_register(struct mtk_base_afe *afe)
{
struct mt8186_afe_private *afe_priv = afe->platform_priv;
Expand Down Expand Up @@ -124,27 +147,6 @@ int mt8186_audsys_clk_register(struct mtk_base_afe *afe)
afe_priv->lookup[i] = cl;
}

return 0;
return devm_add_action_or_reset(afe->dev, mt8186_audsys_clk_unregister, afe);
}

void mt8186_audsys_clk_unregister(struct mtk_base_afe *afe)
{
struct mt8186_afe_private *afe_priv = afe->platform_priv;
struct clk *clk;
struct clk_lookup *cl;
int i;

if (!afe_priv)
return;

for (i = 0; i < CLK_AUD_NR_CLK; i++) {
cl = afe_priv->lookup[i];
if (!cl)
continue;

clk = cl->clk;
clk_unregister_gate(clk);

clkdev_drop(cl);
}
}
1 change: 0 additions & 1 deletion sound/soc/mediatek/mt8186/mt8186-audsys-clk.h
Expand Up @@ -10,6 +10,5 @@
#define _MT8186_AUDSYS_CLK_H_

int mt8186_audsys_clk_register(struct mtk_base_afe *afe);
void mt8186_audsys_clk_unregister(struct mtk_base_afe *afe);

#endif

0 comments on commit dffd9e2

Please sign in to comment.