Messages in this thread | | | Date | Mon, 21 Aug 2023 11:35:32 +0800 | From | Chen-Yu Tsai <> | Subject | Re: [PATCH] spmi: mediatek: Fix UAF on device remove |
| |
On Mon, Jul 17, 2023 at 05:39:35PM +0800, Yu-Che Cheng wrote: > The pmif driver data that contains the clocks is allocated along with > spmi_controller. > On device remove, spmi_controller will be freed first, and then devres > , including the clocks, will be cleanup. > This leads to UAF because putting the clocks will access the clocks in > the pmif driver data, which is already freed along with spmi_controller. > > This can be reproduced by enabling DEBUG_TEST_DRIVER_REMOVE and > building the kernel with KASAN. > > Fix the UAF issue by using unmanaged clk_bulk_get() and putting the > clocks before freeing spmi_controller. > > Reported-by: Fei Shao <fshao@chromium.org> > Signed-off-by: Yu-Che Cheng <giver@chromium.org>
Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
Stephen, could you pick this up?
> --- > > drivers/spmi/spmi-mtk-pmif.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/spmi/spmi-mtk-pmif.c b/drivers/spmi/spmi-mtk-pmif.c > index b3c991e1ea40..74b73f9bc222 100644 > --- a/drivers/spmi/spmi-mtk-pmif.c > +++ b/drivers/spmi/spmi-mtk-pmif.c > @@ -465,7 +465,7 @@ static int mtk_spmi_probe(struct platform_device *pdev) > for (i = 0; i < arb->nclks; i++) > arb->clks[i].id = pmif_clock_names[i]; > > - err = devm_clk_bulk_get(&pdev->dev, arb->nclks, arb->clks); > + err = clk_bulk_get(&pdev->dev, arb->nclks, arb->clks); > if (err) { > dev_err(&pdev->dev, "Failed to get clocks: %d\n", err); > goto err_put_ctrl; > @@ -474,7 +474,7 @@ static int mtk_spmi_probe(struct platform_device *pdev) > err = clk_bulk_prepare_enable(arb->nclks, arb->clks); > if (err) { > dev_err(&pdev->dev, "Failed to enable clocks: %d\n", err); > - goto err_put_ctrl; > + goto err_put_clks; > } > > ctrl->cmd = pmif_arb_cmd; > @@ -498,6 +498,8 @@ static int mtk_spmi_probe(struct platform_device *pdev) > > err_domain_remove: > clk_bulk_disable_unprepare(arb->nclks, arb->clks); > +err_put_clks: > + clk_bulk_put(arb->nclks, arb->clks); > err_put_ctrl: > spmi_controller_put(ctrl); > return err; > @@ -509,6 +511,7 @@ static void mtk_spmi_remove(struct platform_device *pdev) > struct pmif *arb = spmi_controller_get_drvdata(ctrl); > > clk_bulk_disable_unprepare(arb->nclks, arb->clks); > + clk_bulk_put(arb->nclks, arb->clks); > spmi_controller_remove(ctrl); > spmi_controller_put(ctrl);
Maybe we need devres versions of spmi_controller_alloc and spmi_controller_add to avoid this in the future? I'm sure drivers put all sorts of stuff in their private data.
ChenYu
> } > -- > 2.41.0.255.g8b1d071c50-goog >
| |