Re: [EXT] Re: [PATCH v10 2/2] rtc: Add NXP PCF85053 driver support

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

Hi Lakshay,

On Mon, 2026-09-14 at 14:51 +0000, Lakshay Piplani wrote:
> Hi Alexandre,
> 
> I wanted to get your view on the two new Sashiko findings reported on the PCF85053 v10 driver patch.
> 
> There were no driver code changes between v9 and v10, and the exact same implementation was reviewed cleanly in v9. I also went through both new findings again.
> 
> For the devres/UAF concern, my understanding is that the RTC object is still alive while the managed IRQ is being released, and RTC core synchronizes pending irqwork before the rtc_device is finally freed, so I don't see an actual use-after-free there.
> 
> For the alarm IRQ race, I understand the interleaving Sashiko is suggesting, but I am not sure whether additional locking is actually required by the RTC core or whether the existing implementation is acceptable.

I've got similar Sashiko pre-existing findings for PCF85063:

https://lore.kernel.org/all/20260807131504.5C6A41F000E9@smtp.kernel.org/
https://lore.kernel.org/all/20260820113906.9BD991F00A3A@smtp.kernel.org/

and ended up dropping the AIE manipulation in the handler:

https://patchwork.kernel.org/project/linux-rtc/patch/20260828164445.3907839-3-alexander.sverdlin@siemens.com/

Does it make sense from your PoV?

> Since these are new findings on unchanged code, I did not want to add more synchronization or teardown changes without confirming that they are necessary.
> 
> Could you please take a look and let me know your opinion?
> 
> Thanks
> Lakshay
> 
> 
> NXP Confidential
> > -----Original Message-----
> > From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> > Sent: Monday, September 14, 2026 8:05 PM
> > To: Lakshay Piplani <lakshay.piplani@nxp.com>
> > Cc: linux-rtc@vger.kernel.org; Alexandre Belloni
> > <alexandre.belloni@bootlin.com>; conor+dt@kernel.org;
> > devicetree@vger.kernel.org; robh@kernel.org
> > Subject: [EXT] Re: [PATCH v10 2/2] rtc: Add NXP PCF85053 driver support
> > 
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report
> > this email' button
> > 
> > 
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to
> > consider:
> > - [High] Possible use-after-free of `rtc` object in `rtc->irqwork` due to
> > unbalanced devres teardown order.
> > - [High] Missing mutual exclusion between the threaded IRQ handler and alarm
> > configuration leads to dropped alarms.
> > --
> > 
> > commit 41cf346ef49a0ce7f8953c6b31dac43a5fae5017
> > Author: Lakshay Piplani <lakshay.piplani@nxp.com>
> > Subject: rtc: Add NXP PCF85053 driver support
> > 
> > This commit introduces support for the NXP PCF85053 I2C RTC. It includes
> > timer and calendar functionality, alarm management, and clock output control.
> > 
> > diff --git a/drivers/rtc/rtc-pcf85053.c b/drivers/rtc/rtc-pcf85053.c
> > --- a/drivers/rtc/rtc-pcf85053.c
> > +++ b/drivers/rtc/rtc-pcf85053.c
> > 
> > [ ... ]
> > 
> > > +static irqreturn_t pcf85053_irq(int irq, void *dev_id) {
> > > +     struct device *dev = dev_id;
> > > +     struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> > > +     unsigned int st;
> > > +     int err;
> > > +
> > > +     err = regmap_read(pcf85053->regmap, PCF85053_REG_ST, &st);
> > > +     if (err || !(st & PCF85053_BIT_AF))
> > > +             return IRQ_NONE;
> > > +
> > > +     /*
> > > +      * The alarm matches every day; disable AIE to make it one-shot. Mask
> > > +      * the interrupt before clearing AF so there is no window where AF is
> > > +      * cleared but AIE is still able to re-assert the line.
> > > +      */
> > > +     err = pcf85053_set_aie(pcf85053->regmap, false);
> > 
> > [Severity: High]
> > Could this threaded IRQ handler race with pcf85053_rtc_set_alarm() and drop
> > a newly programmed alarm?
> > 
> > If the IRQ thread reads the status register and sees the alarm flag AF is set, but
> > is then preempted by a userspace process programming a new alarm via
> > pcf85053_rtc_set_alarm():
> > 
> > Userspace in pcf85053_rtc_set_alarm() would:
> > - configure the new time
> > - clear AF
> > - enable AIE
> > 
> > When the IRQ thread resumes here, it calls pcf85053_set_aie(..., false).
> > Would this overwrite the newly enabled AIE bit and silently disable the new
> > alarm interrupt?
> > 
> > [ ... ]
> > 
> > > +static int pcf85053_probe(struct i2c_client *client) {
> > > +     const struct pcf85053_config *config;
> > > +     struct device *dev = &client->dev;
> > > +     const char *iface = NULL;
> > > +     struct pcf85053 *pcf85053;
> > > +     int err;
> > 
> > [ ... ]
> > 
> > > +     pcf85053->rtc = devm_rtc_allocate_device(dev);
> > > +     if (IS_ERR(pcf85053->rtc))
> > > +             return PTR_ERR(pcf85053->rtc);
> > 
> > [ ... ]
> > 
> > > +     if (config->has_alarms && pcf85053->is_primary && client->irq > 0) {
> > > +             /*
> > > +              * ALRT is shared by the alarm (AIE), oscillator-fail (OFIE) and
> > > +              * RTC-clear (CIE) sources. This driver only services the alarm,
> > > +              * so disable the other two; otherwise ALRT could stay asserted
> > > +              * with AF=0 and the handler could not clear it.
> > > +              */
> > > +             err = regmap_update_bits(pcf85053->regmap,
> > PCF85053_REG_CTRL,
> > > +                                      PCF85053_BIT_OFIE | PCF85053_BIT_CIE, 0);
> > > +             if (err)
> > > +                     return err;
> > > +
> > > +             err = devm_request_threaded_irq(dev, client->irq,
> > > +                                             NULL, pcf85053_irq,
> > > +                                             IRQF_ONESHOT,
> > > +                                             "pcf85053", dev);
> > 
> > [Severity: High]
> > Is there a potential use-after-free during device unbind due to the devres
> > allocation order here?
> > 
> > Since devres cleans up in reverse order of creation:
> > 1. devm_rtc_register_device() unregisters the RTC.
> > 2. devm_request_threaded_irq() frees the IRQ.
> > 3. devm_rtc_allocate_device() frees the RTC structure.
> > 
> > If a hardware alarm interrupt fires exactly in the window after the RTC is
> > unregistered but before the IRQ is freed, the handler pcf85053_irq() will
> > execute and call rtc_update_irq().
> > 
> > This unconditionally queues rtc->irqwork, but the rtc object will be freed
> > momentarily when devm_rtc_allocate_device() unrolls. Can this result in a use-
> > after-free when the scheduled work runs on the freed rtc object?
> > 
> > > +             if (err)
> > > +                     return dev_err_probe(dev, err,
> > > +                                          "unable to request IRQ %d\n",
> > > +                                          client->irq);
> > > +
> > > +             set_bit(RTC_FEATURE_ALARM, pcf85053->rtc->features);
> > > +             err = devm_device_init_wakeup(dev);
> > > +             if (err)
> > > +                     return dev_err_probe(dev, err,
> > > +                                          "failed to initialize wakeup\n");
> > > +             err = devm_pm_set_wake_irq(dev, client->irq);
> > > +             if (err)
> > > +                     return dev_err_probe(dev, err,
> > > +                                          "failed to set wake IRQ\n");
> > > +     }
> > > +
> > > +#ifdef CONFIG_COMMON_CLK
> > > +     err = pcf85053_clkout_register_clk(pcf85053);
> > > +     if (err)
> > > +             return err;
> > > +#endif
> > > +
> > > +     return devm_rtc_register_device(pcf85053->rtc);
> > > +}
> > 
> > --
> > Sashiko AI
> > review · https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2
> > Fsashiko.dev%2F%23%2Fpatchset%2F20260914141537.3881890-1-
> > lakshay.piplani%40nxp.com%3Fpart%3D2&data=05%7C02%7Clakshay.piplani
> > %40nxp.com%7C2059c4a928154fa82d8108df126d5080%7C686ea1d3bc2b4c
> > 6fa92cd99c5c301635%7C0%7C0%7C639249932851733938%7CUnknown%7C
> > TWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJX
> > aW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=zXFx
> > KJ50N%2BbMSYPUDqrtMwPVpGR47yJcyv%2B8PAn%2F%2B2M%3D&reserved
> > =0

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com