Re: [PATCH 4/6] rtc: rzn1: make interrupts optional

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch replaces immutable `const struct rtc_class_ops` structures with a single mutable global `struct rtc_class_ops rzn1_rtc_ops`. The driver then dynamically modifies this shared global struct during `probe()`. This introduces two severe issues: 1) A security regression by placing function pointers in writable `.data` memory instead of read-only `.rodata`, making them an easy target for control-flow hijacking exploits. 2) Driver model isolation breakage. If a device is unbound and rebound with different hardware capabilities (e.g., via a DT overlay lacking the alarm IRQ), or if multiple RTC instances exist, the global struct retains stale function pointers from previous probes. For example, a device without an alarm interrupt will inherit `set_alarm` from a prior successful probe, causing the RTC core to mistakenly advertise unsupported features and attempt to program unsupported hardware.
- [Medium] `RTC_FEATURE_UPDATE_INTERRUPT` is erroneously left enabled when the device lacks an alarm interrupt, causing the RTC core to advertise non-existent capabilities. Before this patch, the driver explicitly cleared `RTC_FEATURE_UPDATE_INTERRUPT` if it couldn't acquire the 'pps' interrupt. In this patch, the check for the 'pps' interrupt and the subsequent feature flag clearing logic are nested inside an `else if (irq >= 0)` block that checks for a valid 'alarm' interrupt. If a device has no 'alarm' interrupt, this entire block is bypassed. Consequently, the driver neither requests the 'pps' interrupt nor clears the `RTC_FEATURE_UPDATE_INTERRUPT` flag (which is enabled by default by the RTC core), leaving the device in an inconsistent state.
--

commit 2099c0ea773de6a8c400669f6b3316540360644e
Author: Wolfram Sang <wsa+renesas@sang-engineering.com>

rtc: rzn1: make interrupts optional

This patch makes interrupts optional in the rzn1 rtc driver, disabling
alarm handling if they are absent. It replaces the multiple static rtc_ops
structures with a single non-static one that is adapted during probe().

> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index 1e1676a7f3cc9..583ce12b052d9 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -379,22 +379,9 @@ static int rzn1_rtc_set_offset(struct device *dev, long offset)
>  	return 0;
>  }
>  
> -static const struct rtc_class_ops rzn1_rtc_ops_subu = {
> +static struct rtc_class_ops rzn1_rtc_ops = {

[Severity: High]
Does removing const from this global operations structure introduce a
security regression? Placing function pointers into writable memory makes
them a potential target for control-flow hijacking exploits.

[ ... ]

> @@ -491,9 +475,9 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
>  
>  	if (scmp_val) {
>  		writel(rate - 1, rtc->base + RZN1_RTC_SCMP);
> -		rtc->rtcdev->ops = &rzn1_rtc_ops_scmp;
>  	} else {
> -		rtc->rtcdev->ops = &rzn1_rtc_ops_subu;
> +		rzn1_rtc_ops.read_offset = rzn1_rtc_read_offset;
> +		rzn1_rtc_ops.set_offset = rzn1_rtc_set_offset;

[Severity: High]
Can mutating this shared global structure in rzn1_rtc_probe() lead to state
corruption across unbind/rebind cycles or if multiple instances of the RTC
exist?

If a device is unbound and then rebound with different hardware capabilities,
or if a device lacking an alarm interrupt inherits the set_alarm operations
from a previous probe, the RTC core could mistakenly advertise unsupported
features and attempt to program unsupported hardware.

[ ... ]

> @@ -504,20 +488,30 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&rtc->ctl1_access_lock);
>  
> -	ret = devm_request_irq(dev, irq, rzn1_rtc_alarm_irq, 0, "RZN1 RTC Alarm", rtc);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "RTC alarm interrupt not available\n");
>  
> -	irq = platform_get_irq_byname_optional(pdev, "pps");
> -	if (irq == -EPROBE_DEFER)
> +	irq = platform_get_irq_byname_optional(pdev, "alarm");
> +	if (irq == -EPROBE_DEFER) {
>  		return irq;
> -	if (irq >= 0)
> -		ret = devm_request_irq(dev, irq, rzn1_rtc_1s_irq, 0, "RZN1 RTC 1s", rtc);
> -
> -	if (irq < 0 || ret) {
> -		set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->rtcdev->features);
> -		clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtcdev->features);
> -		dev_warn(dev, "RTC pps interrupt not available. Alarm has only minute accuracy\n");
> +	} else if (irq >= 0) {
> +		ret = devm_request_irq(dev, irq, rzn1_rtc_alarm_irq, 0, "RZN1 RTC Alarm", rtc);

[Severity: Medium]
Will RTC_FEATURE_UPDATE_INTERRUPT mistakenly remain enabled if there is no
alarm interrupt present on the device?

Because the feature flag cleanup and the pps interrupt probing are now nested
inside this else if (irq >= 0) block, a missing alarm interrupt bypasses this
entire logic.

Since the RTC core enables RTC_FEATURE_UPDATE_INTERRUPT by default, failing
to clear it here could leave the device in an inconsistent state and cause
incorrect capabilities to be advertised.

> +		if (ret)
> +			return dev_err_probe(dev, ret, "RTC alarm interrupt not available\n");
> +
> +		irq = platform_get_irq_byname_optional(pdev, "pps");
> +		if (irq == -EPROBE_DEFER)
> +			return irq;
> +		if (irq >= 0)
> +			ret = devm_request_irq(dev, irq, rzn1_rtc_1s_irq, 0, "RZN1 RTC 1s", rtc);
> +
> +		if (irq < 0 || ret) {
> +			set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->rtcdev->features);
> +			clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtcdev->features);
> +			dev_warn(dev, "RTC pps interrupt not available. Alarm has only minute accuracy\n");
> +		}
> +
> +		rzn1_rtc_ops.read_alarm = rzn1_rtc_read_alarm;
> +		rzn1_rtc_ops.set_alarm = rzn1_rtc_set_alarm;
> +		rzn1_rtc_ops.alarm_irq_enable = rzn1_rtc_alarm_irq_enable;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914211826.73727-8-wsa+renesas@sang-engineering.com?part=4