In particular seeing zero in eft->month is problematic, as it results
in -1 (converted to unsigned int, i.e. yielding 0xffffffff) getting
passed to rtc_year_days(), where the value gets used as an array index
(normally resulting in a crash). This was observed with the driver
enabled on x86 on some Fujitsu system (with possibly not up to date
firmware, but anyway).
Perhaps efi_read_alarm() should not fail if neither enabled nor pending
are set, but the returned time is invalid?
Reported-by: Raymund Will <[email protected]>
Signed-off-by: Jan Beulich <[email protected]>
---
drivers/rtc/rtc-efi.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
--- 3.15-rc8/drivers/rtc/rtc-efi.c
+++ 3.15-rc8-EFI-RTC-check-time/drivers/rtc/rtc-efi.c
@@ -17,6 +17,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/stringify.h>
#include <linux/time.h>
#include <linux/platform_device.h>
#include <linux/rtc.h>
@@ -48,8 +49,8 @@ compute_wday(efi_time_t *eft)
int y;
int ndays = 0;
- if (eft->year < 1998) {
- pr_err("EFI year < 1998, invalid date\n");
+ if (eft->year < EFI_RTC_EPOCH) {
+ pr_err("EFI year < " __stringify(EFI_RTC_EPOCH) ", invalid date\n");
return -1;
}
@@ -78,19 +79,36 @@ convert_to_efi_time(struct rtc_time *wti
eft->timezone = EFI_UNSPECIFIED_TIMEZONE;
}
-static void
+static bool
convert_from_efi_time(efi_time_t *eft, struct rtc_time *wtime)
{
memset(wtime, 0, sizeof(*wtime));
+
+ if (eft->second >= 60)
+ return false;
wtime->tm_sec = eft->second;
+
+ if (eft->minute >= 60)
+ return false;
wtime->tm_min = eft->minute;
+
+ if (eft->hour >= 24)
+ return false;
wtime->tm_hour = eft->hour;
+
+ if (!eft->day || eft->day > 31)
+ return false;
wtime->tm_mday = eft->day;
+
+ if (!eft->month || eft->month > 12)
+ return false;
wtime->tm_mon = eft->month - 1;
wtime->tm_year = eft->year - 1900;
/* day of the week [0-6], Sunday=0 */
wtime->tm_wday = compute_wday(eft);
+ if (wtime->tm_wday < 0)
+ return false;
/* day in the year [1-365]*/
wtime->tm_yday = compute_yday(eft);
@@ -106,6 +124,8 @@ convert_from_efi_time(efi_time_t *eft, s
default:
wtime->tm_isdst = -1;
}
+
+ return true;
}
static int efi_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
@@ -122,7 +142,8 @@ static int efi_read_alarm(struct device
if (status != EFI_SUCCESS)
return -EINVAL;
- convert_from_efi_time(&eft, &wkalrm->time);
+ if (!convert_from_efi_time(&eft, &wkalrm->time))
+ return -EIO;
return rtc_valid_tm(&wkalrm->time);
}
@@ -163,7 +184,8 @@ static int efi_read_time(struct device *
return -EINVAL;
}
- convert_from_efi_time(&eft, tm);
+ if (!convert_from_efi_time(&eft, tm))
+ return -EIO;
return rtc_valid_tm(tm);
}
On Mon, Jun 02, 2014 at 03:09:16PM +0100, Jan Beulich wrote:
> In particular seeing zero in eft->month is problematic, as it results
> in -1 (converted to unsigned int, i.e. yielding 0xffffffff) getting
> passed to rtc_year_days(), where the value gets used as an array index
> (normally resulting in a crash). This was observed with the driver
> enabled on x86 on some Fujitsu system (with possibly not up to date
> firmware, but anyway).
>
> Perhaps efi_read_alarm() should not fail if neither enabled nor pending
> are set, but the returned time is invalid?
>
> Reported-by: Raymund Will <[email protected]>
> Signed-off-by: Jan Beulich <[email protected]>
Acked-by: Lee, Chun-Yi <[email protected]>
Thanks a lot!
Joey Lee
> ---
> drivers/rtc/rtc-efi.c | 32 +++++++++++++++++++++++++++-----
> 1 file changed, 27 insertions(+), 5 deletions(-)
>
> --- 3.15-rc8/drivers/rtc/rtc-efi.c
> +++ 3.15-rc8-EFI-RTC-check-time/drivers/rtc/rtc-efi.c
> @@ -17,6 +17,7 @@
>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/stringify.h>
> #include <linux/time.h>
> #include <linux/platform_device.h>
> #include <linux/rtc.h>
> @@ -48,8 +49,8 @@ compute_wday(efi_time_t *eft)
> int y;
> int ndays = 0;
>
> - if (eft->year < 1998) {
> - pr_err("EFI year < 1998, invalid date\n");
> + if (eft->year < EFI_RTC_EPOCH) {
> + pr_err("EFI year < " __stringify(EFI_RTC_EPOCH) ", invalid date\n");
> return -1;
> }
>
> @@ -78,19 +79,36 @@ convert_to_efi_time(struct rtc_time *wti
> eft->timezone = EFI_UNSPECIFIED_TIMEZONE;
> }
>
> -static void
> +static bool
> convert_from_efi_time(efi_time_t *eft, struct rtc_time *wtime)
> {
> memset(wtime, 0, sizeof(*wtime));
> +
> + if (eft->second >= 60)
> + return false;
> wtime->tm_sec = eft->second;
> +
> + if (eft->minute >= 60)
> + return false;
> wtime->tm_min = eft->minute;
> +
> + if (eft->hour >= 24)
> + return false;
> wtime->tm_hour = eft->hour;
> +
> + if (!eft->day || eft->day > 31)
> + return false;
> wtime->tm_mday = eft->day;
> +
> + if (!eft->month || eft->month > 12)
> + return false;
> wtime->tm_mon = eft->month - 1;
> wtime->tm_year = eft->year - 1900;
>
> /* day of the week [0-6], Sunday=0 */
> wtime->tm_wday = compute_wday(eft);
> + if (wtime->tm_wday < 0)
> + return false;
>
> /* day in the year [1-365]*/
> wtime->tm_yday = compute_yday(eft);
> @@ -106,6 +124,8 @@ convert_from_efi_time(efi_time_t *eft, s
> default:
> wtime->tm_isdst = -1;
> }
> +
> + return true;
> }
>
> static int efi_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> @@ -122,7 +142,8 @@ static int efi_read_alarm(struct device
> if (status != EFI_SUCCESS)
> return -EINVAL;
>
> - convert_from_efi_time(&eft, &wkalrm->time);
> + if (!convert_from_efi_time(&eft, &wkalrm->time))
> + return -EIO;
>
> return rtc_valid_tm(&wkalrm->time);
> }
> @@ -163,7 +184,8 @@ static int efi_read_time(struct device *
> return -EINVAL;
> }
>
> - convert_from_efi_time(&eft, tm);
> + if (!convert_from_efi_time(&eft, tm))
> + return -EIO;
>
> return rtc_valid_tm(tm);
> }
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/