Re: [PATCH v3 09/13] i3c: dw-i3c-master: Add a quirk to skip clock and reset

[sashiko-bot@kernel.org]

Sashiko AI review found 5 potential issue(s):
- [Critical] The driver fails to disable hardware interrupts before destroying the workqueue during removal, leading to a potential Use-After-Free.
- [High] The patch alters the initialization order of `pclk` and `core_rst`, causing the core reset to be deasserted before the peripheral clock is enabled.
- [High] Casting the return value of `device_get_match_data()` to `unsigned long` causes pointer addresses to be interpreted as quirk flags on ACPI PRP0001 systems, which can lead to random hardware initialization failures.
- [High] A crafted `clock-frequency` device property can cause an integer overflow leading to a divide-by-zero kernel panic on 32-bit systems.
- [High] Combining `devm_clk_get_enabled()` with Runtime PM clock management causes a clock refcount underflow if the device is suspended during driver removal.
--

commit 62e84914f9fc0efd829919bb31559f49b5cb8b00
Author: Akhil R <akhilrajeev@nvidia.com>

i3c: dw-i3c-master: Add a quirk to skip clock and reset

Some ACPI-enumerated devices like Tegra410 do not have clock and reset
resources exposed via the clk/reset frameworks. This patch adds match data
to skip acquiring these controls during probe and reads the core clock rate
from a device property instead.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index edd42daf75539b..b90756ade2db6d 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -556,13 +557,26 @@ static void dw_i3c_master_set_intr_regs(struct dw_i3c_master *master)
>  	writel(IBI_REQ_REJECT_ALL, master->regs + IBI_MR_REQ_REJECT);
>  }
>  
> +static unsigned long dw_i3c_master_get_core_rate(struct dw_i3c_master *master)
> +{
> +	unsigned int core_rate_prop;
> +
> +	if (!(master->quirks & DW_I3C_ACPI_SKIP_CLK_RST))
> +		return clk_get_rate(master->core_clk);
> +
> +	if (device_property_read_u32(master->dev, "clock-frequency", &core_rate_prop))
> +		return 0;
> +
> +	return core_rate_prop;
> +}

Could reading a large clock-frequency property cause an integer overflow?

If core_rate is read as a very large value from the property (e.g., 
3400000000), it seems core_period in dw_i3c_clk_cfg() could evaluate to 0 
due to the addition in the DIV_ROUND_UP() macro wrapping around on 32-bit 
systems. 

Would the subsequent DIV_ROUND_UP(I3C_BUS_THIGH_MAX_NS, core_period) then 
trigger a divide-by-zero panic?

[ ... ]
> @@ -1580,19 +1594,33 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  	if (IS_ERR(master->regs))
>  		return PTR_ERR(master->regs);
>  
> -	master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> -	if (IS_ERR(master->core_clk))
> -		return PTR_ERR(master->core_clk);
> +	if (has_acpi_companion(&pdev->dev)) {
> +		quirks = (unsigned long)device_get_match_data(&pdev->dev);

Can this pointer cast lead to random hardware initialization failures?

If an ACPI device uses the PRP0001 mechanism, device_get_match_data()
might return a pointer to struct dw_i3c_drvdata instead of an integer.

If the pointer address happens to have BIT(3) set, could the driver
inadvertently skip acquiring core_clk and core_rst, leading to an external
abort when reading hardware registers on an unclocked device?

> +	} else if (pdev->dev.of_node) {
> +		drvdata = device_get_match_data(&pdev->dev);
> +		if (drvdata)
> +			quirks = drvdata->flags;
> +	}
> +	master->quirks = quirks;
> +
> +	if (master->quirks & DW_I3C_ACPI_SKIP_CLK_RST) {
> +		master->core_clk = NULL;
> +		master->core_rst = NULL;
> +	} else {
> +		master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);

Is there a risk of a clock refcount underflow here during device removal?

The clocks are dynamically disabled in dw_i3c_master_runtime_suspend().
If the device is already suspended when dw_i3c_common_remove() calls
pm_runtime_disable() without forcefully resuming first, would the
devres cleanup unconditionally call clk_disable_unprepare() again?

> +		if (IS_ERR(master->core_clk))
> +			return PTR_ERR(master->core_clk);
> +
> +		master->core_rst = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev,
> +											"core_rst");
> +		if (IS_ERR(master->core_rst))
> +			return PTR_ERR(master->core_rst);
> +	}
>  
>  	master->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");

Does this change alter the initialization order between the peripheral clock
and the core reset?

In the original code, pclk was enabled before core_rst was acquired and
deasserted. By moving core_rst into the else block above pclk, the reset
appears to be deasserted while pclk is still disabled.

Since hardware IPs typically require the peripheral bus clock to be running
for reset signals to properly synchronize and clear APB registers, could
this inverted sequence cause bus hangs or initialization failures?

[ ... ]

Additionally, is there a potential use-after-free regarding hj_work during
driver removal?

In dw_i3c_common_remove(), cancel_work_sync(&master->hj_work) is called
followed by i3c_master_unregister(), which destroys master->base.wq.

Since the IRQ was requested with devm_request_irq(), it remains active
until devres runs after remove() returns. If a Hot-Join interrupt fires
after the workqueue is destroyed, would the handler execute and call
queue_work(), dereferencing the freed pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260423085718.70762-1-akhilrajeev@nvidia.com?part=9