Skip to contentSkip to conversation

rtc: m48t59: Propagate errors from optional IRQ lookup

PATCH New [email protected] 2 participants 4 messages Aug 10 Real time clock (RTC)

lore
Share & tools
lore

[email protected]

author
#0 lore raw

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

From: bui duc phuc <[email protected]>

platform_get_irq_optional() returns a positive IRQ number on success or a negative error code on failure. For an optional IRQ, -ENXIO indicates that no optional IRQ is available, while other errors should be propagated.

Propagate all error codes returned by platform_get_irq_optional() other than -ENXIO, so that failures are properly reported to the caller. Preserve NO_IRQ for the case where no optional IRQ is available.

Signed-off-by bui duc phuc <[email protected]>
---
 drivers/rtc/rtc-m48t59.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/rtc/rtc-m48t59.c +5 -3
index 4e608bc8bbd3..1150f3f6b008 100644
@@ -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;
 
-	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;
+	} else {
+		m48t59->irq = NO_IRQ;
 	}
 
 	m48t59->rtc = devm_rtc_allocate_device(&pdev->dev);
Signature · 1 line
2.43.0
author
#1 lore raw

PATCH 2/2 rtc: cmos: Propagate errors from optional IRQ lookup

From: bui duc phuc <[email protected]>

platform_get_irq_optional() returns a positive IRQ number on success or a negative error code on failure. For an optional IRQ, -ENXIO indicates that no optional IRQ is available, while other errors should be propagated.

Propagate all error codes returned by platform_get_irq_optional() other than -ENXIO, while preserving -1 for the case where no optional IRQ is available.

Signed-off-by bui duc phuc <[email protected]>
---
 drivers/rtc/rtc-cmos.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/rtc/rtc-cmos.c +3 -1
index fa04ece151b8..0a2c7383c8cd 100644
@@ -1436,7 +1436,9 @@ static int __init cmos_platform_probe(struct platform_device *pdev)
 	else
 		resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq_optional(pdev, 0);
-	if (irq < 0) {
+	if (irq < 0 && irq != -ENXIO)
+		return irq;
+	if (irq == -ENXIO) {
 		irq = -1;
 #ifdef CONFIG_X86
 		/*
Signature · 1 line
2.43.0
AI review
#2 lore raw

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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Propagating -EPROBE_DEFER from platform_get_irq_optional() causes a permanent device probe failure…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Propagating -EPROBE_DEFER from platform_get_irq_optional() causes a permanent device probe failure because rtc-cmos is registered using platform_driver_probe(), which explicitly forbids deferred probing.
- [High] Propagating parsing errors like -EINVAL bypasses the x86 legacy IRQ fallback, breaking RTC probing on systems with malformed ACPI tables.
--

--- Patch [2]: [PATCH 2/2] rtc: cmos: Propagate errors from optional IRQ lookup --- commit bf54cacd6dce9cc731cd51fdbcbf8b84af0b1e7f
Author: bui duc phuc <[email protected]>

rtc: cmos: Propagate errors from optional IRQ lookup

The commit updates the RTC CMOS driver to propagate error codes returned by platform_get_irq_optional() other than -ENXIO, while preserving -1 for the case where no optional IRQ is available.

drivers/rtc/rtc-cmos.c static int __init cmos_platform_probe(struct platform_device *pdev) · line 1440

Quoted 11 lines
drivers/rtc/rtc-cmos.c +2 -1
index fa04ece151b8b..0a2c7383c8cd4 100644
@@ -1436,7 +1436,9 @@ static int __init cmos_platform_probe(struct platform_device *pdev)
 	else
 		resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq_optional(pdev, 0);
-	if (irq < 0) {
+	if (irq < 0 && irq != -ENXIO)
+		return irq;

[Severity: High]
Does propagating -EPROBE_DEFER from platform_get_irq_optional() cause a permanent device probe failure?

Since rtc-cmos is registered using platform_driver_probe(), which explicitly forbids deferred probing, returning -EPROBE_DEFER here might cause the driver core to convert it to -ENXIO and permanently fail the probe instead of proceeding without an optional IRQ.

[Severity: High]
Does propagating parsing errors like -EINVAL bypass the x86 legacy IRQ fallback?

On x86 systems with malformed ACPI tables (e.g., specifying IRQ 0, which platform_get_irq_optional() rejects with -EINVAL), propagating the error bypasses the fallback logic below that successfully hardcodes the IRQ to RTC_IRQ if legacy PIC IRQs are supported, potentially breaking RTC probing on those systems.

Quoted 4 lines
drivers/rtc/rtc-cmos.c +1 -0
+	if (irq == -ENXIO) {
 		irq = -1;
 #ifdef CONFIG_X86
 		/*
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
AI review
#3 lore raw
In reply to [email protected]
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

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 <[email protected]>

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.

drivers/rtc/rtc-m48t59.c static int m48t59_rtc_probe(struct platform_device *pdev) · line 408

Quoted 12 lines
drivers/rtc/rtc-m48t59.c +2 -2
index 4e608bc8bbd36..1150f3f6b0084 100644
@@ -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.

drivers/rtc/rtc-m48t59.c static int m48t59_rtc_probe(struct platform_device *pdev) · line 415

Quoted 8 lines
drivers/rtc/rtc-m48t59.c +1 -1
-	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?

Quoted 5 lines
drivers/rtc/rtc-m48t59.c +2 -0
+	} else {
+		m48t59->irq = NO_IRQ;
 	}
 
 	m48t59->rtc = devm_rtc_allocate_device(&pdev->dev);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1