| Message ID | 20260810051504.26912-1-phucduc.bui@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] rtc: m48t59: Propagate errors from optional IRQ lookup | expand |
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);
diff --git a/drivers/rtc/rtc-m48t59.c b/drivers/rtc/rtc-m48t59.c index 4e608bc8bbd3..1150f3f6b008 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; - 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);