Re: [PATCH v2 2/2] rtc: qcom: rtc-glink: Add new driver to support alarms on Glymur & Hamoa boards

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] rtc: qcom-glink: permanent -EAGAIN wedge and races in qcom_rtc_glink_request()
- [High] rtc: qcom-glink: UAF and ERR_PTR dereference on qcom_rtc_glink_probe() failure
- [High] rtc: qcom-glink: u32 wrap-around in qcom_rtc_glink_get_alarm() and set_alarm()
- [High] rtc: qcom-glink: indefinite probe deferral in qcom_rtc_glink_probe() with PDR
- [Medium] rtc: qcom-glink: ignored status in qcom_rtc_glink_set_timer_value()
- [Medium] rtc: qcom-glink: unbalanced device_init_wakeup() on probe error and remove
- [Medium] rtc: qcom-glink: returning -EINVAL instead of -ETIME in qcom_rtc_glink_set_alarm()
- [Medium] rtc: qcom-glink: premature client unregister in qcom_rtc_glink_remove()
- [Medium] rtc: qcom-glink: wakeup race from deferring rtc_update_irq() to workqueue
--

commit 3cad1ec82d81d9d80a30f2fc1c26ef5d59d6afbe
Author: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>

rtc: qcom: rtc-glink: Add new driver to support alarms on Glymur & Hamoa boards

This commit adds the rtc-qcom-glink auxiliary driver under pmic_glink to
support RTC time read/write operations and alarm notifications relayed by
remote firmware on Qualcomm Glymur and Hamoa platforms.

Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-defined-devices

> diff --git a/drivers/rtc/rtc-qcom-glink.c b/drivers/rtc/rtc-qcom-glink.c
> new file mode 100644
> index 0000000000000..b1394b08e28d2
> --- /dev/null
> +++ b/drivers/rtc/rtc-qcom-glink.c

[ ... ]

> +static int qcom_rtc_glink_request(struct qcom_rtc_glink *rtc_glink,
> +				  void *data, size_t len)
> +{
> +	unsigned long flags;
> +	bool timed_out = false;
> +	int ret;
> +
> +	/*
> +	 * Reinit the completion before request_pending becomes visible to
> +	 * qcom_rtc_glink_pdr_notify(), so a concurrent SSR down-transition
> +	 * can't complete() a stale completion that gets wiped out by
> +	 * reinit_completion() right after.
> +	 */
> +	reinit_completion(&rtc_glink->ack);
> +	rtc_glink->error = 0;
> +	rtc_glink->resp_valid = false;
> +
> +	spin_lock_irqsave(&rtc_glink->state_lock, flags);
> +	if (!rtc_glink->service_up || rtc_glink->timed_out) {
> +		spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +		return !rtc_glink->service_up ? -ECONNRESET : -EAGAIN;
> +	}
> +	rtc_glink->request_pending = true;
> +	spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +
> +	ret = pmic_glink_send(rtc_glink->client, data, len);
> +	if (!ret && !wait_for_completion_timeout(&rtc_glink->ack, HZ)) {
> +		ret = -ETIMEDOUT;
> +		timed_out = true;
> +	}
> +
> +	spin_lock_irqsave(&rtc_glink->state_lock, flags);
> +	rtc_glink->request_pending = false;
> +	rtc_glink->timed_out |= timed_out;
> +	spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +
> +	return ret < 0 ? ret : rtc_glink->error;
> +}

[Severity: High]
As reported by Sashiko on v1 [1], can a timeout in qcom_rtc_glink_request()
permanently wedge the driver into returning -EAGAIN for all subsequent RTC
operations?

When wait_for_completion_timeout() expires, qcom_rtc_glink_request() sets
rtc_glink->timed_out = true, and every later call to qcom_rtc_glink_request()
immediately returns -EAGAIN while rtc_glink->timed_out remains set.
rtc_glink->timed_out is only cleared in qcom_rtc_glink_callback() when a
response arrives while !rtc_glink->request_pending, or in
qcom_rtc_glink_pdr_notify() on a down-transition. If a request times out
because the message was dropped by firmware and no late reply or SSR
down-transition ever occurs (noting that Glymur sets pdr_available = false),
won't rtc_glink->timed_out stay true forever?

In addition, neither qcom_rtc_glink_callback() nor qcom_rtc_glink_pdr_notify()
clears rtc_glink->request_pending under state_lock. If the response or a PDR
down-notification arrives on another CPU right after
wait_for_completion_timeout() returns 0 and before qcom_rtc_glink_request()
re-acquires state_lock:

CPU0                                  CPU1
qcom_rtc_glink_request()
  wait_for_completion_timeout() -> 0
  timed_out = true;
                                      qcom_rtc_glink_callback()
                                        spin_lock_irqsave(&state_lock)
                                        pending = rtc_glink->request_pending;
                                        /* pending is true, consumes reply */
                                        spin_unlock_irqrestore(&state_lock)
  spin_lock_irqsave(&state_lock)
  rtc_glink->request_pending = false;
  rtc_glink->timed_out |= timed_out;
  spin_unlock_irqrestore(&state_lock)

Because CPU1 already consumed the response while request_pending was still
true, no further response will arrive to clear rtc_glink->timed_out. The same
happens if qcom_rtc_glink_pdr_notify() runs with !up in that window and clears
rtc_glink->timed_out before CPU0 sets rtc_glink->timed_out |= true.

Also, in the early exit path of qcom_rtc_glink_request():

	if (!rtc_glink->service_up || rtc_glink->timed_out) {
		spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
		return !rtc_glink->service_up ? -ECONNRESET : -EAGAIN;
	}

rtc_glink->service_up is re-read after dropping state_lock. If service_up was
false under the lock and qcom_rtc_glink_pdr_notify() sets service_up = true
before the return expression is evaluated, qcom_rtc_glink_request() returns
-EAGAIN instead of -ECONNRESET, causing qcom_rtc_glink_probe() to fail with
-ENODEV instead of -EPROBE_DEFER.

> +
> +static int qcom_rtc_glink_set_timer_value(struct qcom_rtc_glink *rtc_glink,
> +					  u32 timer_id, u32 timer_value)
> +{
> +	struct qcom_rtc_glink_set_timer_req msg = {
> +		.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> +		.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> +		.hdr.opcode = cpu_to_le32(RTC_GLINK_SET_TIMER_VALUE),
> +		.timer_id = cpu_to_le32(timer_id),
> +		.timer_value = cpu_to_le32(timer_value),
> +	};
> +
> +	dev_dbg(rtc_glink->dev,
> +		"TX opcode=0x%x timer_id=%u timer_value=%u\n",
> +		RTC_GLINK_SET_TIMER_VALUE, timer_id, timer_value);
> +	return qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> +}

[Severity: Medium]
Does qcom_rtc_glink_set_timer_value() ignore firmware failure statuses
returned in struct qcom_rtc_glink_timer_resp?

The comment above struct qcom_rtc_glink_timer_resp notes that for
RTC_GLINK_SET_TIMER_VALUE, resp->value holds the pass/fail status. However, in
qcom_rtc_glink_callback(), case RTC_GLINK_GET_TIMER_VALUE falls through to
case RTC_GLINK_SET_TIMER_VALUE and unconditionally sets rtc_glink->error = 0:

	case RTC_GLINK_GET_TIMER_VALUE:
		fallthrough;
	case RTC_GLINK_SET_TIMER_VALUE: {
		const struct qcom_rtc_glink_timer_resp *resp = data;
		...
		rtc_glink->resp_value = le32_to_cpu(resp->value);
		rtc_glink->resp_valid = true;
		rtc_glink->error = 0;

And qcom_rtc_glink_set_timer_value() returns qcom_rtc_glink_request() directly
without inspecting rtc_glink->resp_value or verifying that the response opcode
matched the pending request. Should RTC_GLINK_SET_TIMER_VALUE check
le32_to_cpu(resp->value) and set rtc_glink->error = -EIO when non-zero, similar
to RTC_GLINK_SET_REAL_TIME?

[ ... ]

> +static int qcom_rtc_glink_set_alarm(struct qcom_rtc_glink *rtc_glink,
> +				    struct rtc_wkalrm *alarm)
> +{
> +	time64_t alarm_t = rtc_tm_to_time64(&alarm->time);
> +	struct rtc_time now_tm;
> +	time64_t now_real, secs_until_alarm;
> +	u32 fw_current, fw_alarm;
> +	int ret;
> +
> +	ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
> +	if (ret)
> +		return ret;
> +
> +	now_real = rtc_tm_to_time64(&now_tm);
> +	secs_until_alarm = alarm_t - now_real;
> +	dev_dbg(rtc_glink->dev, "set_alarm: now_real=%lld alarm_t=%lld delta=%lld\n",
> +		now_real, alarm_t, secs_until_alarm);
> +
> +	if (secs_until_alarm < 0)
> +		return -EINVAL;

[Severity: Medium]
Should qcom_rtc_glink_set_alarm() return -ETIME instead of -EINVAL when the
alarm time has already passed?

In drivers/rtc/interface.c, __rtc_set_alarm() reads the current time, checks
scheduled <= now, and then calls rtc->ops->set_alarm(). Because
qcom_rtc_glink_set_alarm() performs another GLINK round-trip via
qcom_rtc_glink_get_time(), the RTC second can tick past alarm_t during that
IPC call.

The callers of __rtc_set_alarm() in drivers/rtc/interface.c
(rtc_timer_enqueue(), rtc_timer_remove(), and rtc_timer_do_work()) check
specifically for -ETIME to expire the timer immediately via rtc->irqwork.
Returning -EINVAL causes rtc_timer_do_work() to drop the timer after retries
without firing it, leaves the next timer unscheduled in rtc_timer_remove(),
and fails 1-second UIE timers in rtc_timer_enqueue(). Also, if
secs_until_alarm == 0 when alarm->enabled is true, programming fw_alarm =
fw_current + 0 sets an alarm timestamp that expires before the remaining GLINK
requests finish.

> +	if (secs_until_alarm > U32_MAX)
> +		return -ERANGE;
> +
> +	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_TIME, &fw_current);
> +	if (ret)
> +		return ret;
> +	if (fw_current == U32_MAX)
> +		fw_current = 0;
> +	fw_alarm = fw_current + (u32)secs_until_alarm;
> +	dev_dbg(rtc_glink->dev, "set_alarm: fw_current=%u fw_alarm=%u\n", fw_current, fw_alarm);
> +
> +	ret = qcom_rtc_glink_set_alarm_en(rtc_glink, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_rtc_glink_set_timer_value(rtc_glink, QCOM_RTC_GLINK_ALARM_TIME, fw_alarm);
> +	if (ret)
> +		return ret;
> +
> +	return qcom_rtc_glink_set_alarm_en(rtc_glink, alarm->enabled);
> +}
> +
> +static int qcom_rtc_glink_get_alarm(struct qcom_rtc_glink *rtc_glink,
> +				    struct rtc_wkalrm *alarm)
> +{
> +	u32 fw_alarm_time, fw_current, alarm_en;
> +	struct rtc_time now_tm;
> +	time64_t now_real, secs_until_alarm, alarm_real;
> +	int ret;
> +
> +	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_ALARM_TIME, &fw_alarm_time);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_TIME, &fw_current);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_ALARM_ENABLE, &alarm_en);
> +	if (ret)
> +		return ret;
> +	if (fw_current == U32_MAX)
> +		fw_current = 0;
> +
> +	ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
> +	if (ret)
> +		return ret;
> +
> +	now_real = rtc_tm_to_time64(&now_tm);
> +	secs_until_alarm = (u32)(fw_alarm_time - fw_current);
> +	alarm_real = now_real + secs_until_alarm;
> +	dev_dbg(rtc_glink->dev, "get_alarm: now_real=%lld fw_current=%u fw_alarm=%u delta=%lld\n",
> +		now_real, fw_current, fw_alarm_time, secs_until_alarm);
> +
> +	rtc_time64_to_tm(alarm_real, &alarm->time);
> +	alarm->enabled = !!alarm_en;
> +	return 0;
> +}

[Severity: High]
What happens in qcom_rtc_glink_get_alarm() when fw_alarm_time is less than
fw_current, such as at boot when no alarm has been programmed (fw_alarm_time
is 0) or after an alarm has already expired?

Because secs_until_alarm is computed as:

	secs_until_alarm = (u32)(fw_alarm_time - fw_current);
	alarm_real = now_real + secs_until_alarm;

any negative delta underflows in 32-bit unsigned arithmetic and wraps to a
timestamp roughly 136 years in the future (exceeding rtc->range_max). During
devm_rtc_register_device(), __devm_rtc_register_device() calls
__rtc_read_alarm() and rtc_initialize_alarm(), which relies on comparing the
alarm time against now to ignore already-expired alarms. Wrapping expired
alarms into the future defeats that check and enqueues stale alarms into
rtc->timerqueue.

Conversely, in qcom_rtc_glink_set_alarm(), fw_alarm = fw_current +
(u32)secs_until_alarm can overflow 32 bits or land on the U32_MAX sentinel
when secs_until_alarm >= U32_MAX - fw_current.

> +
> +static void qcom_rtc_glink_alarm_work(struct work_struct *work)
> +{
> +	struct qcom_rtc_glink *rtc_glink =
> +		container_of(work, struct qcom_rtc_glink, alarm_work);
> +
> +	/*
> +	 * The glink client is registered, and can receive an unsolicited
> +	 * ALARM_EXPIRED, before the RTC device exists - e.g. if the board
> +	 * booted because of a previously armed alarm.
> +	 */
> +	if (!rtc_glink->rtc)
> +		return;
> +
> +	rtc_lock(rtc_glink->rtc);
> +	rtc_update_irq(rtc_glink->rtc, 1, RTC_IRQF | RTC_AF);
> +	rtc_unlock(rtc_glink->rtc);
> +}
> +
> +static void qcom_rtc_glink_callback(const void *data, size_t len, void *priv)
> +{
> +	struct qcom_rtc_glink *rtc_glink = priv;
> +	const struct pmic_glink_hdr *hdr = data;
> +	unsigned long flags;
> +	bool pending;
> +
> +	if (len < sizeof(*hdr))
> +		return;
> +
> +	dev_dbg(rtc_glink->dev,
> +		"RX opcode=0x%x type=0x%x owner=0x%x len=%zu\n",
> +		le32_to_cpu(hdr->opcode), le32_to_cpu(hdr->type),
> +		le32_to_cpu(hdr->owner), len);
> +
> +	if (le32_to_cpu(hdr->opcode) == RTC_GLINK_ALARM_EXPIRED) {
> +		schedule_work(&rtc_glink->alarm_work);
> +		return;
> +	}

[Severity: Medium]
Can deferring rtc_update_irq() to qcom_rtc_glink_alarm_work() race with system
suspend when RTC_GLINK_ALARM_EXPIRED arrives during suspend entry or
suspend-to-idle?

The GLINK RX callback runs from the IPCC interrupt handler
(qcom_ipcc_irq_fn() -> qcom_glink_smem_intr() -> qcom_glink_native_rx() ->
pmic_glink_rpmsg_callback() -> qcom_rtc_glink_callback()), and none of the
underlying IPCC, GLINK, or pmic_glink layers activate a wakeup source. When
qcom_rtc_glink_callback() only calls schedule_work(&rtc_glink->alarm_work) and
returns from the interrupt handler, pm_stay_awake(rtc->dev.parent) inside
rtc_update_irq() is not called until alarm_work gets scheduled and acquires
rtc_lock(rtc_glink->rtc) (which is also held by rtc_suspend() during
dpm_suspend()).

Since rtc_update_irq() is safe to call from hardirq context, checks
IS_ERR_OR_NULL(rtc), calls pm_stay_awake(rtc->dev.parent) immediately, and
schedules rtc->irqwork (which takes rtc->ops_lock itself), could
qcom_rtc_glink_callback() call rtc_update_irq() directly instead of bouncing
through alarm_work?

[ ... ]

> +static int qcom_rtc_glink_probe(struct auxiliary_device *adev,
> +				const struct auxiliary_device_id *id)
> +{

[ ... ]

> +	pmic_glink_client_register(rtc_glink->client);
> +
> +	ret = qcom_rtc_glink_get_time(rtc_glink, &tm);
> +	if (ret) {
> +		pmic_glink_client_unregister(rtc_glink->client);
> +		if (ret == -ECONNRESET)
> +			return -EPROBE_DEFER;
> +		return dev_err_probe(dev, -ENODEV,
> +				     "RTC GLINK firmware is not available\n");
> +	}

[Severity: High]
On PDR-enabled platforms such as Hamoa (X1E80100), can returning -EPROBE_DEFER
here when ret == -ECONNRESET leave the driver permanently on the deferred
probe list?

In drivers/soc/qcom/pmic_glink.c, pmic_glink_probe() registers the rtc-glink
auxiliary device before pdr_add_lookup() completes and before __pmic_glink is
set, so pg->client_state is initially 0 and qcom_rtc_glink_get_time() returns
-ECONNRESET. On Hamoa (PMIC_RTR_ADSP_APPS), pg->pdr_available is true, so
pg->pdr_state only transitions to SERVREG_SERVICE_STATE_UP asynchronously in
pmic_glink_pdr_callback() from a QMI worker thread. Because
pmic_glink_pdr_callback() only notifies registered clients and does not bind a
device, it does not trigger driver_deferred_probe_trigger(). If
pmic_glink_pdr_callback() runs after device probing has settled, what will
re-trigger probing of qcom_rtc_glink_probe()?

> +
> +	device_init_wakeup(dev, true);

[Severity: Medium]
Does calling unmanaged device_init_wakeup(dev, true) here leak the wakeup
source on probe failure or when the driver is unbound in
qcom_rtc_glink_remove()?

Neither the error paths below nor qcom_rtc_glink_remove() call
device_init_wakeup(dev, false). Should this use devm_device_init_wakeup(dev)
instead?

> +
> +	rtc_glink->rtc = devm_rtc_allocate_device(dev);
> +	if (IS_ERR(rtc_glink->rtc)) {
> +		pmic_glink_client_unregister(rtc_glink->client);
> +		return PTR_ERR(rtc_glink->rtc);
> +	}
> +
> +	rtc_glink->rtc->ops = &qcom_rtc_glink_rtc_ops;
> +	rtc_glink->rtc->range_min = 0;
> +	rtc_glink->rtc->range_max = U32_MAX;
> +
> +	return devm_rtc_register_device(rtc_glink->rtc);
> +}

[Severity: High]
As reported by Sashiko on v1 [2], can a probe failure after
pmic_glink_client_register() cause a use-after-free or an ERR_PTR dereference
in qcom_rtc_glink_alarm_work()?

Once pmic_glink_client_register() has been called, qcom_rtc_glink_callback()
can receive an unsolicited RTC_GLINK_ALARM_EXPIRED message and schedule
rtc_glink->alarm_work:

1. None of the error paths after pmic_glink_client_register()
   (qcom_rtc_glink_get_time(), devm_rtc_allocate_device(), or
   devm_rtc_register_device()) cancel rtc_glink->alarm_work before devres
   frees rtc_glink.

2. If devm_rtc_allocate_device() fails, rtc_glink->rtc is assigned the ERR_PTR
   before the IS_ERR() check and before pmic_glink_client_unregister() is
   called. Because qcom_rtc_glink_alarm_work() only checks if
   (!rtc_glink->rtc), a concurrent work execution will pass that check and
   dereference the ERR_PTR in rtc_lock(rtc_glink->rtc).

3. If devm_rtc_register_device() fails, pmic_glink_client_unregister() is not
   called at all. Because devm_pmic_glink_client_alloc() was registered before
   devm_rtc_allocate_device(), devres releases rtc_glink->rtc before removing
   the GLINK client, leaving rtc_glink->rtc as a non-NULL dangling pointer
   while callbacks and alarm_work can still run.

> +
> +static void qcom_rtc_glink_remove(struct auxiliary_device *adev)
> +{
> +	struct qcom_rtc_glink *rtc_glink = dev_get_drvdata(&adev->dev);
> +
> +	pmic_glink_client_unregister(rtc_glink->client);
> +	cancel_work_sync(&rtc_glink->alarm_work);
> +}

[Severity: Medium]
In qcom_rtc_glink_remove(), pmic_glink_client_unregister() is called before
devres unregisters rtc_glink->rtc, and without clearing rtc_glink->service_up.

If an RTC operation or rtc->irqwork (scheduled by qcom_rtc_glink_alarm_work()
via rtc_update_irq()) runs while the RTC device is still registered, won't it
see rtc_glink->service_up == true, send a GLINK request whose response can
never arrive, block for HZ until timeout, and cause rtc_timer_do_work() to
bail out on -ETIMEDOUT without calling pm_relax(rtc->dev.parent)?

[1] https://lore.kernel.org/r/20260806210934.485441F000E9@smtp.kernel.org
[2] https://lore.kernel.org/r/20260806210934.485441F000E9@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-rtc-glink-cleanup-v2-0-895eb6e8d890@qti.qualcomm.com?part=2