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

[sashiko-bot@kernel.org]

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://sashiko.dev/#/patchset/20260914141537.3881890-1-lakshay.piplani@nxp.com?part=2