[PATCH 0/1] EmbeddedPkg: Fix multiple Virtual RTC issues

[PATCH 0/1] EmbeddedPkg: Fix multiple Virtual RTC issues

[Pete Batard]

This patch fixes multiple issues in VirtualRealTimeClockLib.c:

1. Use of two separate variables (ElapsedSeconds and EpochSeconds) in
   LibGetTime(), where only a single one should have been used.

2. Possible underflow while sustracting TZ/DST offsets in LibSetTime()
   if the user sets a date that is very close to start of epoch.

3. Non substraction of elpased seconds since reset, which would lead to
   the following behaviour in UEFI shell (assuming for this example that
   exactly 5 minutes have elapsed since platform reset):

     Shell> time 12:00:00
     Shell> time
     12:05:01 (LOCAL)

   In other words, setting and immediately reading back the time would
   result in the returned value being offset by the time since reset.

   This last behaviour has been observed (and confirmed fixed) using an
   RPi3 platform.

Regards,

/Pete

Pete Batard (1):
  EmbeddedPkg: Fix multiple Virtual RTC issues

 .../VirtualRealTimeClockLib.c                 | 34 ++++++++++++++-----
 1 file changed, 25 insertions(+), 9 deletions(-)

-- 
2.17.0.windows.1

[PATCH 1/1] EmbeddedPkg: Fix multiple Virtual RTC issues

[Pete Batard]

LibGetTime():
- Two variables were used for the epoch, where only one should have been.
- Also harmonize variable name to match the one used in LibSetTime.
LiBSetTime():
- Address possible underflows if time is set to start of epoch.
- Ensure that time being read does actually match time that was manually
  set (plus the time elapsed since), by subtracting number of seconds
  since reset.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard <pete@akeo.ie>
---
 EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c | 34 ++++++++++++++------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
index 4c354730d02b..5559106b696f 100644
--- a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
+++ b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
@@ -54,13 +54,12 @@ LibGetTime (
   )
 {
   EFI_STATUS  Status;
-  UINT32      EpochSeconds;
   INT16       TimeZone;
   UINT8       Daylight;
   UINT64      Freq;
   UINT64      Counter;
   UINT64      Remainder;
-  UINTN       ElapsedSeconds;
+  UINTN       EpochSeconds;
   UINTN       Size;
 
   if (Time == NULL) {
@@ -75,13 +74,13 @@ LibGetTime (
 
   // Get the epoch time from non-volatile storage
   Size = sizeof (UINTN);
-  ElapsedSeconds = 0;
+  EpochSeconds = 0;
   Status = EfiGetVariable (
              (CHAR16 *)mEpochVariableName,
              &gEfiCallerIdGuid,
              NULL,
              &Size,
-             (VOID *)&ElapsedSeconds
+             (VOID *)&EpochSeconds
              );
   // Fall back to compilation-time epoch if not set
   if (EFI_ERROR (Status)) {
@@ -93,7 +92,7 @@ LibGetTime (
     // If you are attempting to use this library on such an environment, please
     // contact the edk2 mailing list, so we can try to add support for it.
     //
-    ElapsedSeconds = BUILD_EPOCH;
+    EpochSeconds = BUILD_EPOCH;
     DEBUG ((
       DEBUG_INFO,
       "LibGetTime: %s non volatile variable was not found - Using compilation time epoch.\n",
@@ -101,7 +100,7 @@ LibGetTime (
       ));
   }
   Counter = GetPerformanceCounter ();
-  ElapsedSeconds += DivU64x64Remainder (Counter, Freq, &Remainder);
+  EpochSeconds += DivU64x64Remainder (Counter, Freq, &Remainder);
 
   // Get the current time zone information from non-volatile storage
   Size = sizeof (TimeZone);
@@ -204,7 +203,7 @@ LibGetTime (
     }
   }
 
-  EpochToEfiTime (ElapsedSeconds, Time);
+  EpochToEfiTime (EpochSeconds, Time);
 
   // Because we use the performance counter, we can fill the Nanosecond attribute
   // provided that the remainder doesn't overflow 64-bit during multiplication.
@@ -240,6 +239,9 @@ LibSetTime (
   )
 {
   EFI_STATUS  Status;
+  UINT64      Freq;
+  UINT64      Counter;
+  UINT64      Remainder;
   UINTN       EpochSeconds;
 
   if (!IsTimeValid (Time)) {
@@ -249,16 +251,30 @@ LibSetTime (
   EpochSeconds = EfiTimeToEpoch (Time);
 
   // Adjust for the correct time zone, i.e. convert to UTC time zone
-  if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
+  if ((Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE)
+      && (EpochSeconds > Time->TimeZone * SEC_PER_MIN)) {
     EpochSeconds -= Time->TimeZone * SEC_PER_MIN;
   }
 
   // Adjust for the correct period
-  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
+  if (((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT)
+      && (EpochSeconds > SEC_PER_HOUR)) {
     // Convert to un-adjusted time, i.e. fall back one hour
     EpochSeconds -= SEC_PER_HOUR;
   }
 
+  // Use the performance counter to substract the number of seconds
+  // since platform reset. Without this, setting time from the shell
+  // and immediately reading it back would result in a forward time
+  // offset, of the duration during which the platform has been up.
+  Freq = GetPerformanceCounterProperties (NULL, NULL);
+  if (Freq != 0) {
+    Counter = GetPerformanceCounter ();
+    if (EpochSeconds > DivU64x64Remainder (Counter, Freq, &Remainder)) {
+      EpochSeconds -= DivU64x64Remainder (Counter, Freq, &Remainder);
+    }
+  }
+
   // Save the current time zone information into non-volatile storage
   Status = EfiSetVariable (
              (CHAR16 *)mTimeZoneVariableName,
-- 
2.17.0.windows.1

Re: [PATCH 1/1] EmbeddedPkg: Fix multiple Virtual RTC issues

[Ard Biesheuvel]

On Tue, 26 Feb 2019 at 00:52, Pete Batard <pete@akeo.ie> wrote:
>
> LibGetTime():
> - Two variables were used for the epoch, where only one should have been.
> - Also harmonize variable name to match the one used in LibSetTime.
> LiBSetTime():
> - Address possible underflows if time is set to start of epoch.
> - Ensure that time being read does actually match time that was manually
>   set (plus the time elapsed since), by subtracting number of seconds
>   since reset.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pete Batard <pete@akeo.ie>

Thanks Pete

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Pushed as 9ab4ec518882..1342d7679e10
> ---
>  EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c | 34 ++++++++++++++------
>  1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
> index 4c354730d02b..5559106b696f 100644
> --- a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
> +++ b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
> @@ -54,13 +54,12 @@ LibGetTime (
>    )
>  {
>    EFI_STATUS  Status;
> -  UINT32      EpochSeconds;
>    INT16       TimeZone;
>    UINT8       Daylight;
>    UINT64      Freq;
>    UINT64      Counter;
>    UINT64      Remainder;
> -  UINTN       ElapsedSeconds;
> +  UINTN       EpochSeconds;
>    UINTN       Size;
>
>    if (Time == NULL) {
> @@ -75,13 +74,13 @@ LibGetTime (
>
>    // Get the epoch time from non-volatile storage
>    Size = sizeof (UINTN);
> -  ElapsedSeconds = 0;
> +  EpochSeconds = 0;
>    Status = EfiGetVariable (
>               (CHAR16 *)mEpochVariableName,
>               &gEfiCallerIdGuid,
>               NULL,
>               &Size,
> -             (VOID *)&ElapsedSeconds
> +             (VOID *)&EpochSeconds
>               );
>    // Fall back to compilation-time epoch if not set
>    if (EFI_ERROR (Status)) {
> @@ -93,7 +92,7 @@ LibGetTime (
>      // If you are attempting to use this library on such an environment, please
>      // contact the edk2 mailing list, so we can try to add support for it.
>      //
> -    ElapsedSeconds = BUILD_EPOCH;
> +    EpochSeconds = BUILD_EPOCH;
>      DEBUG ((
>        DEBUG_INFO,
>        "LibGetTime: %s non volatile variable was not found - Using compilation time epoch.\n",
> @@ -101,7 +100,7 @@ LibGetTime (
>        ));
>    }
>    Counter = GetPerformanceCounter ();
> -  ElapsedSeconds += DivU64x64Remainder (Counter, Freq, &Remainder);
> +  EpochSeconds += DivU64x64Remainder (Counter, Freq, &Remainder);
>
>    // Get the current time zone information from non-volatile storage
>    Size = sizeof (TimeZone);
> @@ -204,7 +203,7 @@ LibGetTime (
>      }
>    }
>
> -  EpochToEfiTime (ElapsedSeconds, Time);
> +  EpochToEfiTime (EpochSeconds, Time);
>
>    // Because we use the performance counter, we can fill the Nanosecond attribute
>    // provided that the remainder doesn't overflow 64-bit during multiplication.
> @@ -240,6 +239,9 @@ LibSetTime (
>    )
>  {
>    EFI_STATUS  Status;
> +  UINT64      Freq;
> +  UINT64      Counter;
> +  UINT64      Remainder;
>    UINTN       EpochSeconds;
>
>    if (!IsTimeValid (Time)) {
> @@ -249,16 +251,30 @@ LibSetTime (
>    EpochSeconds = EfiTimeToEpoch (Time);
>
>    // Adjust for the correct time zone, i.e. convert to UTC time zone
> -  if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
> +  if ((Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE)
> +      && (EpochSeconds > Time->TimeZone * SEC_PER_MIN)) {
>      EpochSeconds -= Time->TimeZone * SEC_PER_MIN;
>    }
>
>    // Adjust for the correct period
> -  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
> +  if (((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT)
> +      && (EpochSeconds > SEC_PER_HOUR)) {
>      // Convert to un-adjusted time, i.e. fall back one hour
>      EpochSeconds -= SEC_PER_HOUR;
>    }
>
> +  // Use the performance counter to substract the number of seconds
> +  // since platform reset. Without this, setting time from the shell
> +  // and immediately reading it back would result in a forward time
> +  // offset, of the duration during which the platform has been up.
> +  Freq = GetPerformanceCounterProperties (NULL, NULL);
> +  if (Freq != 0) {
> +    Counter = GetPerformanceCounter ();
> +    if (EpochSeconds > DivU64x64Remainder (Counter, Freq, &Remainder)) {
> +      EpochSeconds -= DivU64x64Remainder (Counter, Freq, &Remainder);
> +    }
> +  }
> +
>    // Save the current time zone information into non-volatile storage
>    Status = EfiSetVariable (
>               (CHAR16 *)mTimeZoneVariableName,
> --
> 2.17.0.windows.1
>