Re: [PATCH 1/2] rtc: m48t59: Propagate errors from optional IRQ lookup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Improper use of devm_kzalloc for pdev->dev.platform_data leads to Use-After-Free on deferred probe.
- [High] The shared IRQ handler is registered before its required context (drvdata, spinlock) is initialized, risking a NULL pointer dereference.
--

--- Patch [1]: [PATCH 1/2] rtc: m48t59: Propagate errors from optional IRQ lookup ---
commit 9d48ac67a0a6bc0aac19eba1f8852bc6afa6e0da
Author: bui duc phuc <phucduc.bui@gmail.com>

rtc: m48t59: Propagate errors from optional IRQ lookup

This commit updates m48t59_rtc_probe() to propagate error codes other than
-ENXIO from platform_get_irq_optional(). Previously, these errors were ignored
and the driver fell back to polling mode.

> diff --git a/drivers/rtc/rtc-m48t59.c b/drivers/rtc/rtc-m48t59.c
> index 4e608bc8bbd36..1150f3f6b0084 100644
> --- a/drivers/rtc/rtc-m48t59.c
> +++ b/drivers/rtc/rtc-m48t59.c
> @@ -404,15 +404,17 @@ static int m48t59_rtc_probe(struct platform_device *pdev)
>  	 * the mode without IRQ.
>  	 */
>  	m48t59->irq = platform_get_irq_optional(pdev, 0);
> -	if (m48t59->irq <= 0)
> -		m48t59->irq = NO_IRQ;
> +	if (m48t59->irq < 0 && m48t59->irq != -ENXIO)
> +		return m48t59->irq;

[Severity: High]
This is a pre-existing issue, but this new error return path exposes it: if
this returns -EPROBE_DEFER, will it lead to a use-after-free on the next probe
attempt?

Earlier in m48t59_rtc_probe(), if the device is memory-mapped and platform
data is not provided, pdata is allocated with devm_kzalloc() and assigned to
the device:

drivers/rtc/rtc-m48t59.c:m48t59_rtc_probe() {
	...
	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
	if (!pdata)
		return -ENOMEM;
	/* Ensure we only kmalloc platform data once */
	pdev->dev.platform_data = pdata;
	...
}

When the probe fails and returns -EPROBE_DEFER here, devres will free the
pdata allocation. However, pdev->dev.platform_data is not cleared. On the next
probe attempt, dev_get_platdata() will return this dangling pointer, bypassing
reallocation and causing the driver to dereference freed memory.

>  
> -	if (m48t59->irq != NO_IRQ) {
> +	if (m48t59->irq > 0) {
>  		ret = devm_request_irq(&pdev->dev, m48t59->irq,
>  				m48t59_rtc_interrupt, IRQF_SHARED,
>  				"rtc-m48t59", &pdev->dev);
>  		if (ret)
>  			return ret;

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying the IRQ
setup, is it safe to request a shared interrupt here before the driver data
and spinlocks are initialized?

If another device shares this interrupt line and triggers an interrupt
immediately after devm_request_irq(), the handler m48t59_rtc_interrupt() will
execute before m48t59_rtc_probe() has finished setting up:

drivers/rtc/rtc-m48t59.c:m48t59_rtc_probe() {
	...
	spin_lock_init(&m48t59->lock);
	platform_set_drvdata(pdev, m48t59);
	...
}

Because platform_set_drvdata() hasn't been called yet, dev_get_drvdata() in
the interrupt handler will return null, resulting in a null pointer
dereference when it tries to lock it:

drivers/rtc/rtc-m48t59.c:m48t59_rtc_interrupt() {
	struct m48t59_private *m48t59 = dev_get_drvdata(dev);
	...
	spin_lock(&m48t59->lock);
	...
}

Could this cause a crash on platforms with a shared interrupt?

> +	} else {
> +		m48t59->irq = NO_IRQ;
>  	}
>  
>  	m48t59->rtc = devm_rtc_allocate_device(&pdev->dev);

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