Re: [PATCH] spmi: mediatek: Fix UAF on device remove

On Mon, Aug 21, 2023 at 11:35 AM Chen-Yu Tsai [off-list ref] wrote:

On Mon, Jul 17, 2023 at 05:39:35PM +0800, Yu-Che Cheng wrote:

quoted

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 <redacted>
Signed-off-by: Yu-Che Cheng <redacted>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

Stephen, could you pick this up?

quoted

---

 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.

Sounds good, I think I can send a follow-up series for that.

Regards,
Fei

ChenYu

quoted

 }
--
2.41.0.255.g8b1d071c50-goog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel