Re: [PATCH] rtc: efi: restore alarm support with runtime capability probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Uninitialized kernel stack memory (struct padding) is leaked to EFI firmware in efi_set_alarm and efi_set_time.
- [High] efi_read_alarm processes uninitialized stack memory and returns spurious errors when the alarm is disabled.
- [High] wkalrm->enabled is passed to firmware without being normalized to a strict boolean, risking unpredictable firmware behavior.
- [Low] Incorrect format specifier for signed timezone value in procfs output.

Pre-existing issues:
- [High] Lack of year bounds checking allows an extremely large tm_year to be truncated, programming the RTC with an unintended valid year.
--

commit f48bd3a58905ce7e489d515621c84d688e125eec
Author: Johnny.Fan <Johnny.Fan@cixtech.com>

rtc: efi: restore alarm support with runtime capability probe

This commit restores EFI RTC alarm support by probing for the capability
during initialization via GetWakeupTime(). It avoids breaking systems
without a working wakeup capability by clearing RTC_FEATURE_ALARM if the
probe fails, ensuring the alarm functions correctly for platforms that
rely on EFI for shared RTC access.

> diff --git a/drivers/rtc/rtc-efi.c b/drivers/rtc/rtc-efi.c
> index b4f44999ef0f4..708cba43435e0 100644
> --- a/drivers/rtc/rtc-efi.c
> +++ b/drivers/rtc/rtc-efi.c
> @@ -112,6 +112,34 @@ convert_from_efi_time(efi_time_t *eft, struct rtc_time *wtime)
>  	return true;
>  }
>  
> +static int efi_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> +{
> +	efi_time_t eft;
> +	efi_status_t status;
> +
> +	status = efi.get_wakeup_time((efi_bool_t *)&wkalrm->enabled,
> +				     (efi_bool_t *)&wkalrm->pending, &eft);
> +	if (status != EFI_SUCCESS)
> +		return -EINVAL;
> +
> +	if (!convert_from_efi_time(&eft, &wkalrm->time))
> +		return -EIO;

[Severity: High]
Does this code handle disabled alarms correctly? 

According to the UEFI specification, if the alarm is not enabled, the
contents of the Time parameter returned by GetWakeupTime are undefined. Since
eft is allocated on the stack uninitialized, it may contain random stack
garbage if the firmware leaves the buffer unmodified. 

Calling convert_from_efi_time() with this stack garbage will fail its strict
bounds checks (like eft->second >= 60) and invariably return -EIO instead of
returning the disabled alarm state.

> +
> +	return rtc_valid_tm(&wkalrm->time);
> +}
> +
> +static int efi_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> +{
> +	efi_time_t eft;
> +	efi_status_t status;
> +
> +	convert_to_efi_time(&wkalrm->time, &eft);
> +
> +	status = efi.set_wakeup_time((efi_bool_t)wkalrm->enabled, &eft);

[Severity: High]
Does this code leak uninitialized stack memory to the EFI firmware?

convert_to_efi_time() initializes specific fields but fails to zero out the
explicit pad1 and pad2 fields in efi_time_t. The UEFI specification mandates
that Pad1 and Pad2 must be zero, and passing stack garbage in these fields can
cause the firmware to reject valid parameters with EFI_INVALID_PARAMETER.

[Severity: High]
Does this code safely pass the enabled flag to the firmware? 

The wkalrm->enabled field is directly populated from userspace as an
unsigned char (e.g., 255) and cast to efi_bool_t without normalization. The
UEFI specification explicitly defines BOOLEAN as containing exactly 0 for FALSE
or 1 for TRUE. 

Passing un-normalized values can cause unpredictable behavior in buggy UEFI
firmware implementations.

> +
> +	return status == EFI_SUCCESS ? 0 : -EINVAL;
> +}

[ ... ]

> @@ -168,6 +204,25 @@ static int efi_procfs(struct device *dev, struct seq_file *seq)
>  		/* XXX fixme: convert to string? */
>  		seq_printf(seq, "Timezone\t: %u\n", eft.timezone);
>  
> +	if (test_bit(RTC_FEATURE_ALARM, rtc->features)) {
> +		seq_printf(seq,
> +			   "Alarm Time\t: %u:%u:%u.%09u\n"
> +			   "Alarm Date\t: %u-%u-%u\n"
> +			   "Alarm Daylight\t: %u\n"
> +			   "Enabled\t\t: %s\n"
> +			   "Pending\t\t: %s\n",
> +			   alm.hour, alm.minute, alm.second, alm.nanosecond,
> +			   alm.year, alm.month, alm.day,
> +			   alm.daylight,
> +			   enabled == 1 ? "yes" : "no",
> +			   pending == 1 ? "yes" : "no");
> +
> +		if (alm.timezone == EFI_UNSPECIFIED_TIMEZONE)
> +			seq_puts(seq, "Alarm Timezone\t: unspecified\n");
> +		else
> +			seq_printf(seq, "Alarm Timezone\t: %u\n", alm.timezone);

[Severity: Low]
Does this use the correct format specifier for alm.timezone? 

alm.timezone is an s16 variable, so using the %u format specifier will cause
a negative timezone value (e.g., -300) to be formatted as a large positive
unsigned integer in the procfs output.

> +	}
> +

[ ... ]

> @@ -203,7 +261,23 @@ static int __init efi_rtc_probe(struct platform_device *dev)
>  	platform_set_drvdata(dev, rtc);
>  
>  	rtc->ops = &efi_rtc_ops;
> -	clear_bit(RTC_FEATURE_ALARM, rtc->features);
> +	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);

[Severity: High]
This isn't a bug introduced by this patch, but does this probe function need to
initialize rtc->range_max?

Without setting rtc->range_max = 9999 - 1900, the RTC core's internal bounds
checking is disabled. If userspace provides a maliciously large year, the
calculation wtime->tm_year + 1900 in convert_to_efi_time() overflows the 16-bit
unsigned integer eft->year, silently truncating it into a valid range and
programming the hardware RTC with an unintended year. 

This issue affects efi_set_time and propagates to the new efi_set_alarm
functionality.

> +
> +	/*
> +	 * The EFI_RT_SUPPORTED_WAKEUP_SERVICES bit defaults to enabled
> +	 * and only gets cleared when the RT_PROP table explicitly says
> +	 * wakeup is unsupported. Many platforms lack an RT_PROP table

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828031519.2749354-1-johnny.fan@cixtech.com?part=1