Re: [RFC PATCH 07/14] rtc: add support for x86 macs in macsmc-rtc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] rtc: macsmc: uninitialized u64 clkl and wrong mask in macsmc_rtc_probe()
- [High] rtc: macsmc: positive return on ACPI write failure in macsmc_rtc_set_time()
- [Medium] rtc: macsmc: shift underflow when clkr is zero in macsmc_rtc_probe()
- [Low] rtc: macsmc: missing kernel-parameters.txt entry for force_clkm
--

commit a291b39ce81e3a636ffdc1d72c951eb114ad04ea
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

rtc: add support for x86 macs in macsmc-rtc

This commit adds support for Intel T2 Macs to the macsmc-rtc driver. On these
machines, the driver reads the clock rate and RTC offset from the SMC rather
than NVMEM, and optionally reads the clock via the asynchronous CLKL SMC key.

> diff --git a/drivers/rtc/rtc-macsmc.c b/drivers/rtc/rtc-macsmc.c
> index 8fe8830669569..37395c64cbb5e 100644
> --- a/drivers/rtc/rtc-macsmc.c
> +++ b/drivers/rtc/rtc-macsmc.c
[ ... ]
> @@ -20,13 +23,54 @@
>  /* 32768 Hz clock */
>  #define RTC_SEC_SHIFT 15
>  
> +static bool force_clkm;
> +module_param(force_clkm, bool, 0644);
> +MODULE_PARM_DESC(force_clkm, "Force CLKM Key, instead of trying to use CLKL");

[Severity: Low]
Should the new rtc_macsmc.force_clkm module parameter also be documented in
Documentation/admin-guide/kernel-parameters.txt?

[ ... ]
> @@ -66,14 +121,20 @@ static int macsmc_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	u64 ctr = 0, off = 0;
>  	int ret;
>  
> -	ret = apple_smc_read(rtc->smc, SMC_KEY(CLKM), &ctr, RTC_BYTES);
> +	if (rtc->async_rtc && !force_clkm)
> +		ret = macsmc_rtc_read_clkl(rtc, &ctr);
> +	else
> +		ret = apple_smc_read(rtc->smc, SMC_KEY(CLKM), &ctr, RTC_BYTES);
>  	if (ret < 0)
>  		return ret;
>  	if (ret != RTC_BYTES)
>  		return -EIO;
>  
>  	/* This sets the offset such that the set second begins now */
> -	off = (rtc_tm_to_time64(tm) << RTC_SEC_SHIFT) - ctr;
> +	off = (rtc_tm_to_time64(tm) << rtc->shift) - ctr;
> +	if (rtc->smc->is_acpi)
> +		return (apple_smc_write(rtc->smc, SMC_KEY(CLKO), &off, RTC_BYTES) != RTC_BYTES);

[Severity: High]
What happens when apple_smc_write() fails in macsmc_rtc_set_time()?

Because struct rtc_class_ops.set_time is expected to return 0 on success and
a negative error code on failure, returning the boolean comparison result
evaluates to 1 when apple_smc_write() returns a negative errno or a short
write.

In the rtc_dev_ioctl() -> rtc_set_time() -> macsmc_rtc_set_time() call chain:

drivers/rtc/interface.c:rtc_set_time() {
    ...
	else if (rtc->ops->set_time)
		err = rtc->ops->set_time(rtc->dev.parent, tm);
    ...
	return err;
}

a return value of 1 is either overwritten by 0 if update IRQs are enabled or
returned to userspace from the RTC_SET_TIME ioctl as a non-negative value,
causing a failed hardware clock update to be reported as success. Should this
propagate negative error codes from apple_smc_write() and return -EIO on a
size mismatch?

[ ... ]
> @@ -102,18 +165,40 @@ static int macsmc_rtc_probe(struct platform_device *pdev)
[ ... ]
> +	if (rtc->smc->is_acpi) {
> +		ret = apple_smc_read_u32(rtc->smc, SMC_KEY(CLKR), &clkr);
> +		if (ret < 0 || clkr == 0xFFFFFFFF)
> +			rtc->shift = RTC_SEC_SHIFT;
> +		else
> +			rtc->shift = ffs(clkr) - 1;

[Severity: Medium]
Can clkr be 0 if the SMC returns a zero value for the CLKR key in
macsmc_rtc_probe()?

If apple_smc_read_u32() succeeds with clkr == 0, ffs(0) returns 0 and
ffs(clkr) - 1 underflows the uint8_t field rtc->shift to 255. That causes
subsequent shifts by rtc->shift in macsmc_rtc_probe(), macsmc_rtc_get_time(),
and macsmc_rtc_set_time() to shift 64-bit integers by 255 or 271 bits:

	rtc->rtc_dev->range_min = S64_MIN >> (rtc->shift + (64 - RTC_BITS));
	rtc->rtc_dev->range_max = S64_MAX >> (rtc->shift + (64 - RTC_BITS));

Also, if apple_smc_read_u32() fails after partially modifying clkr, the
dev_info() call below only checks clkr != 0xFFFFFFFF and will log the
corrupted clkr value instead of 32768 even though rtc->shift fell back to
RTC_SEC_SHIFT. Would it make sense to guard against !clkr as well?

> +
> +		if (apple_smc_key_exists(rtc->smc, SMC_KEY(CLKL))) {
> +			u64 clkl;
> +
> +			ret = apple_smc_read(rtc->smc, SMC_KEY(CLKL), &clkl, RTC_BYTES);
> +			if (ret == RTC_BYTES && clkl != 0xFFFFFFFFFFFFFFFFULL)
> +				rtc->async_rtc = true;
> +		}

[Severity: High]
Does this read uninitialized stack memory and check against the wrong sentinel
mask in macsmc_rtc_probe()?

Here clkl is an uninitialized 8-byte u64 on the stack, but apple_smc_read()
only writes RTC_BYTES (6 bytes) into &clkl, leaving the upper 2 bytes
uninitialized when evaluating clkl != 0xFFFFFFFFFFFFFFFFULL.

In addition, because CLKL is a 48-bit (6-byte) value, its all-ones invalid
sentinel is 0xffffffffffffULL, as checked in macsmc_rtc_read_clkl():

static int macsmc_rtc_read_clkl(struct macsmc_rtc *rtc, u64 *ctr)
{
	u64 request = 0xffffffffffffULL;
	u64 response = 0;
	...
				if (response != 0xffffffffffffULL) {
					*ctr = response;
					return RTC_BYTES;
				}

Even if clkl is zero-initialized to 0, reading 6 bytes of 0xff into it
produces 0x0000ffffffffffffULL, which will never equal 0xFFFFFFFFFFFFFFFFULL
and will erroneously set rtc->async_rtc = true. Should clkl be initialized to
0 and compared against 0xffffffffffffULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029085504.3547-1-atharvatiwarilinuxdev@gmail.com?part=7