| Message ID | 20230530072514.22001-2-trevor.wu@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: mediatek: fix use-after-free in driver remove path | expand |
Hi, On Tue, May 30, 2023 at 12:25 AM Trevor Wu <trevor.wu@mediatek.com> wrote: > > diff --git a/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c > index be1c53bf4729..05d6f9d78e10 100644 > --- a/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c > +++ b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c > @@ -138,6 +138,29 @@ static const struct afe_gate aud_clks[CLK_AUD_NR_CLK] = { > GATE_AUD6(CLK_AUD_GASRC11, "aud_gasrc11", "top_asm_h", 11), > }; > > +static void mt8188_audsys_clk_unregister(void *data) > +{ > + struct mtk_base_afe *afe = (struct mtk_base_afe *)data; The above cast is unnecessary since the compiler lets you assign from a "void *" to another pointer without a cast. Unnecessary casts are considered harmful because they suspend the compiler's ability to do type checking. Other than that, this looks good. Sorry for not noticing that the same problem affected more than just the driver I fixed previously. Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Wed, 2023-05-31 at 16:47 -0700, Doug Anderson wrote: > > you have verified the sender or the content. > Hi, > > On Tue, May 30, 2023 at 12:25 AM Trevor Wu <trevor.wu@mediatek.com> > wrote: > > > > diff --git a/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c > b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c > > index be1c53bf4729..05d6f9d78e10 100644 > > --- a/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c > > +++ b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c > > @@ -138,6 +138,29 @@ static const struct afe_gate > aud_clks[CLK_AUD_NR_CLK] = { > > GATE_AUD6(CLK_AUD_GASRC11, "aud_gasrc11", "top_asm_h", 11), > > }; > > > > +static void mt8188_audsys_clk_unregister(void *data) > > +{ > > + struct mtk_base_afe *afe = (struct mtk_base_afe *)data; > > The above cast is unnecessary since the compiler lets you assign from > a "void *" to another pointer without a cast. Unnecessary casts are > considered harmful because they suspend the compiler's ability to do > type checking. Other than that, this looks good. Sorry for not > noticing that the same problem affected more than just the driver I > fixed previously. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Got it. I will remove the unnecessary cast in V2. Most importantly, thank you for bringing this issue to our attention. Thanks, Trevor
diff --git a/sound/soc/mediatek/mt8188/mt8188-afe-clk.c b/sound/soc/mediatek/mt8188/mt8188-afe-clk.c index 4c24d0b9e90d..e69c1bb2cb23 100644 --- a/sound/soc/mediatek/mt8188/mt8188-afe-clk.c +++ b/sound/soc/mediatek/mt8188/mt8188-afe-clk.c @@ -436,13 +436,6 @@ int mt8188_afe_init_clock(struct mtk_base_afe *afe) return 0; } -void mt8188_afe_deinit_clock(void *priv) -{ - struct mtk_base_afe *afe = priv; - - mt8188_audsys_clk_unregister(afe); -} - int mt8188_afe_enable_clk(struct mtk_base_afe *afe, struct clk *clk) { int ret; diff --git a/sound/soc/mediatek/mt8188/mt8188-afe-clk.h b/sound/soc/mediatek/mt8188/mt8188-afe-clk.h index 904505d10841..ec53c171c170 100644 --- a/sound/soc/mediatek/mt8188/mt8188-afe-clk.h +++ b/sound/soc/mediatek/mt8188/mt8188-afe-clk.h @@ -111,7 +111,6 @@ int mt8188_afe_get_default_mclk_source_by_rate(int rate); int mt8188_get_apll_by_rate(struct mtk_base_afe *afe, int rate); int mt8188_get_apll_by_name(struct mtk_base_afe *afe, const char *name); int mt8188_afe_init_clock(struct mtk_base_afe *afe); -void mt8188_afe_deinit_clock(void *priv); int mt8188_afe_enable_clk(struct mtk_base_afe *afe, struct clk *clk); void mt8188_afe_disable_clk(struct mtk_base_afe *afe, struct clk *clk); int mt8188_afe_set_clk_rate(struct mtk_base_afe *afe, struct clk *clk, diff --git a/sound/soc/mediatek/mt8188/mt8188-afe-pcm.c b/sound/soc/mediatek/mt8188/mt8188-afe-pcm.c index c3fd32764da0..6a24b339444b 100644 --- a/sound/soc/mediatek/mt8188/mt8188-afe-pcm.c +++ b/sound/soc/mediatek/mt8188/mt8188-afe-pcm.c @@ -3256,10 +3256,6 @@ static int mt8188_afe_pcm_dev_probe(struct platform_device *pdev) if (ret) return dev_err_probe(dev, ret, "init clock error"); - ret = devm_add_action_or_reset(dev, mt8188_afe_deinit_clock, (void *)afe); - if (ret) - return ret; - spin_lock_init(&afe_priv->afe_ctrl_lock); mutex_init(&afe->irq_alloc_lock); diff --git a/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c index be1c53bf4729..05d6f9d78e10 100644 --- a/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c +++ b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c @@ -138,6 +138,29 @@ static const struct afe_gate aud_clks[CLK_AUD_NR_CLK] = { GATE_AUD6(CLK_AUD_GASRC11, "aud_gasrc11", "top_asm_h", 11), }; +static void mt8188_audsys_clk_unregister(void *data) +{ + struct mtk_base_afe *afe = (struct mtk_base_afe *)data; + struct mt8188_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 mt8188_audsys_clk_register(struct mtk_base_afe *afe) { struct mt8188_afe_private *afe_priv = afe->platform_priv; @@ -179,27 +202,5 @@ int mt8188_audsys_clk_register(struct mtk_base_afe *afe) afe_priv->lookup[i] = cl; } - return 0; -} - -void mt8188_audsys_clk_unregister(struct mtk_base_afe *afe) -{ - struct mt8188_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); - } + return devm_add_action_or_reset(afe->dev, mt8188_audsys_clk_unregister, afe); } diff --git a/sound/soc/mediatek/mt8188/mt8188-audsys-clk.h b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.h index 6c5f463ad7e4..45b0948c4a06 100644 --- a/sound/soc/mediatek/mt8188/mt8188-audsys-clk.h +++ b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.h @@ -10,6 +10,5 @@ #define _MT8188_AUDSYS_CLK_H_ int mt8188_audsys_clk_register(struct mtk_base_afe *afe); -void mt8188_audsys_clk_unregister(struct mtk_base_afe *afe); #endif
During mt8188_afe_init_clock(), mt8188_audsys_clk_register() was called followed by several other devm functions. The caller of mt8188_afe_init_clock() utilized devm_add_action_or_reset() to call mt8188_afe_deinit_clock(). However, the order was incorrect, causing a use-after-free issue during remove time. At probe time, the order of calls was: 1. mt8188_audsys_clk_register 2. afe_priv->clk = devm_kcalloc 3. afe_priv->clk[i] = devm_clk_get At remove time, the order of calls was: 1. mt8188_audsys_clk_unregister 3. free afe_priv->clk[i] 2. free afe_priv->clk To resolve the problem, it's necessary to move devm_add_action_or_reset() to the appropriate position so that the remove order can be 3->2->1. Fixes: f6b026479b13 ("ASoC: mediatek: mt8188: support audio clock control") Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> --- sound/soc/mediatek/mt8188/mt8188-afe-clk.c | 7 --- sound/soc/mediatek/mt8188/mt8188-afe-clk.h | 1 - sound/soc/mediatek/mt8188/mt8188-afe-pcm.c | 4 -- sound/soc/mediatek/mt8188/mt8188-audsys-clk.c | 47 ++++++++++--------- sound/soc/mediatek/mt8188/mt8188-audsys-clk.h | 1 - 5 files changed, 24 insertions(+), 36 deletions(-)