| Message ID | 68e752931552e95667191c2641076a1bfaea3dc6.1748933789.git.zhoubinbin@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,01/36] mmc: alcor: Use devm_mmc_alloc_host() helper | expand |
Hello, On Tue, Jun 3, 2025 at 2:28 PM Binbin Zhou <zhoubinbin@loongson.cn> wrote: > > Use new function devm_mmc_alloc_host() to simplify the code. > > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Kevin Hilman <khilman@baylibre.com> > Cc: Jerome Brunet <jbrunet@baylibre.com> > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Cc: linux-amlogic@lists.infradead.org > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > --- > drivers/mmc/host/meson-mx-sdio.c | 20 ++++++++------------ > 1 file changed, 8 insertions(+), 12 deletions(-) > > diff --git a/drivers/mmc/host/meson-mx-sdio.c b/drivers/mmc/host/meson-mx-sdio.c > index e0ae5a0c9670..b6cb475f1a5f 100644 > --- a/drivers/mmc/host/meson-mx-sdio.c > +++ b/drivers/mmc/host/meson-mx-sdio.c > @@ -640,7 +640,7 @@ static int meson_mx_mmc_probe(struct platform_device *pdev) > else if (IS_ERR(slot_pdev)) > return PTR_ERR(slot_pdev); > > - mmc = mmc_alloc_host(sizeof(*host), &slot_pdev->dev); > + mmc = devm_mmc_alloc_host(&slot_pdev->dev, sizeof(*host)); This change is fine at runtime (on my Odroid-C1 board) but it can lead to a use-after-free issue. meson_mx_mmc_register_clks() devm_ registers two clocks and uses host->controller_dev as device. This leads to the fact that during driver removal struct meson_mx_mmc_host is free'd before host->controller_dev, which means the clocks are also free'd after struct meson_mx_mmc_host is already gone. The problem here: the clocks need the struct clk_divider and struct clk_fixed_factor from struct meson_mx_mmc_host. I don't understand why I'm not seeing any problems with this patch at runtime - maybe what I'm describing is just a theoretical issue (because it would only it if something would access the clocks between freeing struct meson_mx_mmc_host and removal of the clocks a few milliseconds later). What are your thoughts on this? If we're concerned about the potential UAF then I can refactor meson_mx_mmc_register_clks() first and then apply this patch afterwards. Best regards, Martin
Hi Martin: Thanks for your testing and reply. On Wed, Jun 4, 2025 at 4:49 AM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hello, > > On Tue, Jun 3, 2025 at 2:28 PM Binbin Zhou <zhoubinbin@loongson.cn> wrote: > > > > Use new function devm_mmc_alloc_host() to simplify the code. > > > > Cc: Neil Armstrong <neil.armstrong@linaro.org> > > Cc: Kevin Hilman <khilman@baylibre.com> > > Cc: Jerome Brunet <jbrunet@baylibre.com> > > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > Cc: linux-amlogic@lists.infradead.org > > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn> > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > > --- > > drivers/mmc/host/meson-mx-sdio.c | 20 ++++++++------------ > > 1 file changed, 8 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/mmc/host/meson-mx-sdio.c b/drivers/mmc/host/meson-mx-sdio.c > > index e0ae5a0c9670..b6cb475f1a5f 100644 > > --- a/drivers/mmc/host/meson-mx-sdio.c > > +++ b/drivers/mmc/host/meson-mx-sdio.c > > @@ -640,7 +640,7 @@ static int meson_mx_mmc_probe(struct platform_device *pdev) > > else if (IS_ERR(slot_pdev)) > > return PTR_ERR(slot_pdev); > > > > - mmc = mmc_alloc_host(sizeof(*host), &slot_pdev->dev); > > + mmc = devm_mmc_alloc_host(&slot_pdev->dev, sizeof(*host)); > This change is fine at runtime (on my Odroid-C1 board) but it can lead > to a use-after-free issue. > meson_mx_mmc_register_clks() devm_ registers two clocks and uses > host->controller_dev as device. > This leads to the fact that during driver removal struct > meson_mx_mmc_host is free'd before host->controller_dev, which means > the clocks are also free'd after struct meson_mx_mmc_host is already > gone. The problem here: the clocks need the struct clk_divider and > struct clk_fixed_factor from struct meson_mx_mmc_host. Sorry, I have a slightly different opinion, which may not necessarily be correct. meson_mx_mmc_host is kzalloc as `extra` in mmc_alloc_host()[1], but in fact, the entire host is released in mmc_host_classdev_release()[2], so I don't think it will affect the use of host->controller_dev. [1]: https://elixir.bootlin.com/linux/v6.15-rc1/source/drivers/mmc/core/host.c#L523 [2]: https://elixir.bootlin.com/linux/v6.15-rc1/source/drivers/mmc/core/host.c#L78 > > I don't understand why I'm not seeing any problems with this patch at > runtime - maybe what I'm describing is just a theoretical issue > (because it would only it if something would access the clocks between > freeing struct meson_mx_mmc_host and removal of the clocks a few > milliseconds later). > > What are your thoughts on this? > If we're concerned about the potential UAF then I can refactor > meson_mx_mmc_register_clks() first and then apply this patch > afterwards. > > > Best regards, > Martin
Hi Binbin, On Wed, Jun 4, 2025 at 5:02 AM Binbin Zhou <zhoubb.aaron@gmail.com> wrote: [...] > > > - mmc = mmc_alloc_host(sizeof(*host), &slot_pdev->dev); > > > + mmc = devm_mmc_alloc_host(&slot_pdev->dev, sizeof(*host)); > > This change is fine at runtime (on my Odroid-C1 board) but it can lead > > to a use-after-free issue. > > meson_mx_mmc_register_clks() devm_ registers two clocks and uses > > host->controller_dev as device. > > This leads to the fact that during driver removal struct > > meson_mx_mmc_host is free'd before host->controller_dev, which means > > the clocks are also free'd after struct meson_mx_mmc_host is already > > gone. The problem here: the clocks need the struct clk_divider and > > struct clk_fixed_factor from struct meson_mx_mmc_host. > > Sorry, I have a slightly different opinion, which may not necessarily > be correct. > > meson_mx_mmc_host is kzalloc as `extra` in mmc_alloc_host()[1], but in > fact, the entire host is released in mmc_host_classdev_release()[2], > so I don't think it will affect the use of host->controller_dev. It's well hidden, so it's tricky to spot: we have two "struct device" in the meson-mx-sdio driver: 1. the "controller_dev" which is belongs to the struct platform_device of the driver/device-tree node [0] 2. a second struct platform_device and thus a second struct device whose of_node belongs to the "slot" (which is a child node of the controller) [1] Clock controller initialization is the same for all slots, so I chose to link it with the controller_device. We want to parse the mmc slot properties (bus-width, cd-gpios, cap-*, ...) from the slot node in device-tree, so we create a second platform_device for the slot (child) node. This design was chosen because the controller supports multiple (up to 3) slots while the mmc core in Linux doesn't. However, it was important for me to at least be prepared in terms of device-tree bindings in case Linux would ever support controllers with multiple slots. So the lifecycle during probe is: a) controller_device is instantiated by device-tree node with compatible = "amlogic,meson8-sdio" b) slot device is instantiated using the "mmc-slot" compatible and using controller_device as parent device c) devm_mmc_alloc_host() is called d) clock providers are devm_* registered and attached to controller_device e) the meson-mx-sdio driver acts as clock consumer for the clock it's providing However, if we want to remove the driver the order is: - remove the device attached to the "mmc-slot" compatible - before that happens struct mmc is freed (as it's parent is being removed) which also frees struct meson_mx_mmc_host - (struct clk_divider and struct clk_fixed_factor are linked with the clocks which are still registered <- here we have a problem because those two structs are part of struct meson_mx_mmc_host which has already been freed) - controller_device is freed - before that happens the provided clocks are freed (as their parent is being removed) I take your questions as feedback on how to make the code easier to read/understand. This is very welcome, because it's been 8 years since I introduced the driver and I also have to think twice about some of its details. Let me know if things are more clear now - or if you still have any doubts/questions. Best regards, Martin [0] https://elixir.bootlin.com/linux/v6.15-rc1/source/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.yaml#L83 [1] https://elixir.bootlin.com/linux/v6.15-rc1/source/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.yaml#L92
diff --git a/drivers/mmc/host/meson-mx-sdio.c b/drivers/mmc/host/meson-mx-sdio.c index e0ae5a0c9670..b6cb475f1a5f 100644 --- a/drivers/mmc/host/meson-mx-sdio.c +++ b/drivers/mmc/host/meson-mx-sdio.c @@ -640,7 +640,7 @@ static int meson_mx_mmc_probe(struct platform_device *pdev) else if (IS_ERR(slot_pdev)) return PTR_ERR(slot_pdev); - mmc = mmc_alloc_host(sizeof(*host), &slot_pdev->dev); + mmc = devm_mmc_alloc_host(&slot_pdev->dev, sizeof(*host)); if (!mmc) { ret = -ENOMEM; goto error_unregister_slot_pdev; @@ -658,13 +658,13 @@ static int meson_mx_mmc_probe(struct platform_device *pdev) host->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(host->base)) { ret = PTR_ERR(host->base); - goto error_free_mmc; + goto error_unregister_slot_pdev; } irq = platform_get_irq(pdev, 0); if (irq < 0) { ret = irq; - goto error_free_mmc; + goto error_unregister_slot_pdev; } ret = devm_request_threaded_irq(host->controller_dev, irq, @@ -672,28 +672,28 @@ static int meson_mx_mmc_probe(struct platform_device *pdev) meson_mx_mmc_irq_thread, IRQF_ONESHOT, NULL, host); if (ret) - goto error_free_mmc; + goto error_unregister_slot_pdev; host->core_clk = devm_clk_get(host->controller_dev, "core"); if (IS_ERR(host->core_clk)) { ret = PTR_ERR(host->core_clk); - goto error_free_mmc; + goto error_unregister_slot_pdev; } host->parent_clk = devm_clk_get(host->controller_dev, "clkin"); if (IS_ERR(host->parent_clk)) { ret = PTR_ERR(host->parent_clk); - goto error_free_mmc; + goto error_unregister_slot_pdev; } ret = meson_mx_mmc_register_clks(host); if (ret) - goto error_free_mmc; + goto error_unregister_slot_pdev; ret = clk_prepare_enable(host->core_clk); if (ret) { dev_err(host->controller_dev, "Failed to enable core clock\n"); - goto error_free_mmc; + goto error_unregister_slot_pdev; } ret = clk_prepare_enable(host->cfg_div_clk); @@ -721,8 +721,6 @@ static int meson_mx_mmc_probe(struct platform_device *pdev) clk_disable_unprepare(host->cfg_div_clk); error_disable_core_clk: clk_disable_unprepare(host->core_clk); -error_free_mmc: - mmc_free_host(mmc); error_unregister_slot_pdev: of_platform_device_destroy(&slot_pdev->dev, NULL); return ret; @@ -741,8 +739,6 @@ static void meson_mx_mmc_remove(struct platform_device *pdev) clk_disable_unprepare(host->cfg_div_clk); clk_disable_unprepare(host->core_clk); - - mmc_free_host(host->mmc); } static const struct of_device_id meson_mx_mmc_of_match[] = {