[PATCH v3 36/36] mmc: meson-mx-sdio: Use devm_mmc_alloc_host() helper

[Binbin Zhou]

Hi Martin:

Thanks for your testing and reply.

On Wed, Jun 4, 2025 at 4:49 AM Martin Blumenstingl
<martin.blumenstingl at googlemail.com> wrote:
>
> Hello,
>
> On Tue, Jun 3, 2025 at 2:28 PM Binbin Zhou <zhoubinbin at loongson.cn> wrote:
> >
> > Use new function devm_mmc_alloc_host() to simplify the code.
> >
> > Cc: Neil Armstrong <neil.armstrong at linaro.org>
> > Cc: Kevin Hilman <khilman at baylibre.com>
> > Cc: Jerome Brunet <jbrunet at baylibre.com>
> > Cc: Martin Blumenstingl <martin.blumenstingl at googlemail.com>
> > Cc: linux-amlogic at lists.infradead.org
> > Reviewed-by: Huacai Chen <chenhuacai at loongson.cn>
> > Signed-off-by: Binbin Zhou <zhoubinbin at 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


-- 
Thanks.
Binbin