Re: [PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Probe deferral is permanently masked because devm_clk_get errors are converted to -ENODEV.
- [High] The sata_gemini bridge state (sg) can be freed while still in use by consumers like pata_ftide010, leading to a Use-After-Free on driver unbind.
- [High] Unconditional clock disable during port stop can lead to negative enable counts if port start partially fails.
--

commit 41dfc4226f4d13085ec0bbac5ed9062fd893a436
Author: Myeonghun Pak <mhun512@gmail.com>

ata: sata_gemini: unwind clocks on IDE pinctrl errors

gemini_sata_bridge_init() prepares and enables both SATA PCLKs, then
disables them again while keeping the clocks prepared for later bridge
start and stop operations. This patch routes the IDE pinctrl failure
through out_unprep_clk so the clocks prepared by gemini_sata_bridge_init()
are correctly unprepared before probe fails.

> diff --git a/drivers/ata/sata_gemini.c b/drivers/ata/sata_gemini.c
> --- a/drivers/ata/sata_gemini.c
> +++ b/drivers/ata/sata_gemini.c
> @@ -353,7 +353,7 @@ static int gemini_sata_probe(struct platform_device *pdev)
>  	if (sg->ide_pins) {
>  		ret = gemini_setup_ide_pins(dev);
>  		if (ret)
> -			return ret;
> +			goto out_unprep_clk;

[Severity: High]
This is a pre-existing issue, but I noticed that probe deferral is
permanently masked when getting the clocks.

In gemini_sata_bridge_init(), which is called earlier in probe, errors
from devm_clk_get() are overwritten:

drivers/ata/sata_gemini.c:gemini_sata_bridge_init() {
    sg->sata0_pclk = devm_clk_get(dev, "SATA0_PCLK");
    if (IS_ERR(sg->sata0_pclk)) {
        dev_err(dev, "no SATA0 PCLK");
        return -ENODEV;
    }
    ...
}

If devm_clk_get() returns -EPROBE_DEFER because the clock provider isn't
ready, does returning -ENODEV cause the SATA controller to permanently
fail to probe?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a
lifetime issue between sata_gemini and pata_ftide010.

When pata_ftide010 retrieves the bridge state:

drivers/ata/pata_ftide010.c:pata_ftide010_gemini_init() {
    ...
    sg = gemini_sata_bridge_get();
    if (IS_ERR(sg))
        return PTR_ERR(sg);
    ftide->sg = sg;
    ...
}

The bridge is returned from a singleton:

drivers/ata/sata_gemini.c:gemini_sata_bridge_get() {
    if (sg_singleton)
        return sg_singleton;
    ...
}

Since the memory is allocated with devm_kzalloc() in gemini_sata_probe()
and no device link or reference count is taken, can this lead to a
use-after-free if sata_gemini is unbound via sysfs while pata_ftide010
is still active?

[Severity: High]
This is also a pre-existing issue, but the clock disable logic might become
unbalanced during port stop.

In pata_ftide010_gemini_port_stop(), the bridge is unconditionally
stopped:

drivers/ata/pata_ftide010.c:pata_ftide010_gemini_port_stop() {
    ...
    if (ftide->master_to_sata1) {
        dev_info(dev, "SATA1 (master) stop\n");
        gemini_sata_stop_bridge(sg, 1);
    }
    ...
}

Which disables the clock:

drivers/ata/sata_gemini.c:gemini_sata_stop_bridge() {
    ...
    else if (bridge == 1)
        clk_disable(sg->sata1_pclk);
    ...
}

If port start partially fails (e.g., due to a PHY timeout in
gemini_sata_setup_bridge()) and the clock was already disabled in the
error path of gemini_sata_start_bridge(), does this unconditional stop
cause a kernel warning or panic due to negative enable counts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626085848.40199-1-mhun512@gmail.com?part=1