[edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime

[edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime

[Marcin Wojtas]

Because the Armada RTC uses a 32-bit counter for seconds,
the maximum time span is just over 136 years.
Time is stored in Unix Epoch format, so it starts in 1970,
Therefore it can not exceed the year 2106.

The issue emerged during ACS test case, which does not pass
Unix Epoch-relative time and caused EfiTimeToEpoch to assert.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
index a811fd368e..40ab01ed41 100644
--- a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
+++ b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
@@ -179,6 +179,16 @@ LibSetWakeupTime (
 {
   UINTN       WakeupSeconds;
 
+  //
+  // Because the Armada RTC uses a 32-bit counter for seconds,
+  // the maximum time span is just over 136 years.
+  // Time is stored in Unix Epoch format, so it starts in 1970,
+  // Therefore it can not exceed the year 2106.
+  //
+  if ((Time->Year < 1970) || (Time->Year >= 2106)) {
+    return EFI_UNSUPPORTED;
+  }
+
   // Convert time to raw seconds
   WakeupSeconds = EfiTimeToEpoch (Time);
   if (WakeupSeconds > MAX_UINT32) {
-- 
2.29.0

Re: [edk2-devel] [edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime

[Laszlo Ersek]

Hi Marcin,

On 12/21/20 17:37, Marcin Wojtas wrote:
> Because the Armada RTC uses a 32-bit counter for seconds,
> the maximum time span is just over 136 years.
> Time is stored in Unix Epoch format, so it starts in 1970,
> Therefore it can not exceed the year 2106.
> 
> The issue emerged during ACS test case, which does not pass
> Unix Epoch-relative time and caused EfiTimeToEpoch to assert.
> 
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
> index a811fd368e..40ab01ed41 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
> @@ -179,6 +179,16 @@ LibSetWakeupTime (
>  {
>    UINTN       WakeupSeconds;
>  
> +  //
> +  // Because the Armada RTC uses a 32-bit counter for seconds,
> +  // the maximum time span is just over 136 years.
> +  // Time is stored in Unix Epoch format, so it starts in 1970,
> +  // Therefore it can not exceed the year 2106.
> +  //
> +  if ((Time->Year < 1970) || (Time->Year >= 2106)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
>    // Convert time to raw seconds
>    WakeupSeconds = EfiTimeToEpoch (Time);
>    if (WakeupSeconds > MAX_UINT32) {
> 

please see:

- edk2-platforms commit fbdfe8c4100d ("Silicon/Marvell/RealTimeClockLib:
make EpochSeconds, WakeupSeconds UINTN", 2020-12-21) -- part of this is
already visible in the context above,

- edk2 commit c06635ea3f4b ("EmbeddedPkg/TimeBaseLib: remove useless
truncation to 32-bit", 2020-12-21).

If you advance the edk2 submodule reference in edk2-platforms to or past
c06635ea3f4b, then the "Time->Year >= 2106" condition will be covered
already, by the (WakeupSeconds > MAX_UINT32) check.

Preventing ASSERT (JulianDate >= EPOCH_JULIAN_DATE) from firing in
EfiGetEpochDays() still makes sense, so the (Time->Year < 1970) check
can still be added usefully, I think.

Thanks
Laszlo

Re: [edk2-devel] [edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime

[Laszlo Ersek]

On 12/21/20 21:09, Laszlo Ersek wrote:
> Hi Marcin,
> 
> On 12/21/20 17:37, Marcin Wojtas wrote:
>> Because the Armada RTC uses a 32-bit counter for seconds,
>> the maximum time span is just over 136 years.
>> Time is stored in Unix Epoch format, so it starts in 1970,
>> Therefore it can not exceed the year 2106.
>>
>> The issue emerged during ACS test case, which does not pass
>> Unix Epoch-relative time and caused EfiTimeToEpoch to assert.
>>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
>> index a811fd368e..40ab01ed41 100644
>> --- a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
>> +++ b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
>> @@ -179,6 +179,16 @@ LibSetWakeupTime (
>>  {
>>    UINTN       WakeupSeconds;
>>  
>> +  //
>> +  // Because the Armada RTC uses a 32-bit counter for seconds,
>> +  // the maximum time span is just over 136 years.
>> +  // Time is stored in Unix Epoch format, so it starts in 1970,
>> +  // Therefore it can not exceed the year 2106.
>> +  //
>> +  if ((Time->Year < 1970) || (Time->Year >= 2106)) {
>> +    return EFI_UNSUPPORTED;
>> +  }
>> +
>>    // Convert time to raw seconds
>>    WakeupSeconds = EfiTimeToEpoch (Time);
>>    if (WakeupSeconds > MAX_UINT32) {
>>
> 
> please see:
> 
> - edk2-platforms commit fbdfe8c4100d ("Silicon/Marvell/RealTimeClockLib:
> make EpochSeconds, WakeupSeconds UINTN", 2020-12-21) -- part of this is
> already visible in the context above,
> 
> - edk2 commit c06635ea3f4b ("EmbeddedPkg/TimeBaseLib: remove useless
> truncation to 32-bit", 2020-12-21).
> 
> If you advance the edk2 submodule reference in edk2-platforms to or past
> c06635ea3f4b, then the "Time->Year >= 2106" condition will be covered
> already, by the (WakeupSeconds > MAX_UINT32) check.

(Oops, sorry, I forgot that edk2-platforms consumes edk2 *only* through PACKAGES_PATH, and never through a submodule; so please adjust the above submodule statement accordingly.)

BTW, is this library instance ever built for 32-bit ARM? Because in that case (== UINTN meaning UINT32), checking Year against 2106 makes sense as well. While build-testing the above-noted patches, I tried to build them for 32-bit ARM too, but that arch did not seem applicable: while the following command works:

  build \
    -a AARCH64 \
    -b NOOPT \
    -p Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc \
    -t GCC5 \
    -m EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf

the same with "-a ARM" fails, with the following error message:

edk2-platforms/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc(...): error 4000: Instance of library class [ArmSoftFloatLib] is not found
        in [edk2/CryptoPkg/Library/OpensslLib/OpensslLib.inf] [ARM]
        consumed by module [edk2/SignedCapsulePkg/Universal/SystemFirmwareUpdate/SystemFirmwareReportDxe.inf]

which implies that "Armada80x0McBin.dsc" is never actually built for ARM.

... Hm, the following platforms:

- Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
- Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc

do build the library for ARM. So this patch seems justified after all, as-posted (i.e., including the year 2106 check).

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> 
> Preventing ASSERT (JulianDate >= EPOCH_JULIAN_DATE) from firing in
> EfiGetEpochDays() still makes sense, so the (Time->Year < 1970) check
> can still be added usefully, I think.
> 
> Thanks
> Laszlo
> 

Re: [edk2-devel] [edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime

[Ard Biesheuvel]

On 12/21/20 9:32 PM, Laszlo Ersek wrote:
> On 12/21/20 21:09, Laszlo Ersek wrote:
>> Hi Marcin,
>>
>> On 12/21/20 17:37, Marcin Wojtas wrote:
>>> Because the Armada RTC uses a 32-bit counter for seconds,
>>> the maximum time span is just over 136 years.
>>> Time is stored in Unix Epoch format, so it starts in 1970,
>>> Therefore it can not exceed the year 2106.
>>>
>>> The issue emerged during ACS test case, which does not pass
>>> Unix Epoch-relative time and caused EfiTimeToEpoch to assert.
>>>
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> ---
>>>  Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
>>> index a811fd368e..40ab01ed41 100644
>>> --- a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
>>> +++ b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
>>> @@ -179,6 +179,16 @@ LibSetWakeupTime (
>>>  {
>>>    UINTN       WakeupSeconds;
>>>  
>>> +  //
>>> +  // Because the Armada RTC uses a 32-bit counter for seconds,
>>> +  // the maximum time span is just over 136 years.
>>> +  // Time is stored in Unix Epoch format, so it starts in 1970,
>>> +  // Therefore it can not exceed the year 2106.
>>> +  //
>>> +  if ((Time->Year < 1970) || (Time->Year >= 2106)) {
>>> +    return EFI_UNSUPPORTED;
>>> +  }
>>> +
>>>    // Convert time to raw seconds
>>>    WakeupSeconds = EfiTimeToEpoch (Time);
>>>    if (WakeupSeconds > MAX_UINT32) {
>>>
>>
>> please see:
>>
>> - edk2-platforms commit fbdfe8c4100d ("Silicon/Marvell/RealTimeClockLib:
>> make EpochSeconds, WakeupSeconds UINTN", 2020-12-21) -- part of this is
>> already visible in the context above,
>>
>> - edk2 commit c06635ea3f4b ("EmbeddedPkg/TimeBaseLib: remove useless
>> truncation to 32-bit", 2020-12-21).
>>
>> If you advance the edk2 submodule reference in edk2-platforms to or past
>> c06635ea3f4b, then the "Time->Year >= 2106" condition will be covered
>> already, by the (WakeupSeconds > MAX_UINT32) check.
> 
> (Oops, sorry, I forgot that edk2-platforms consumes edk2 *only* through PACKAGES_PATH, and never through a submodule; so please adjust the above submodule statement accordingly.)
> 
> BTW, is this library instance ever built for 32-bit ARM? Because in that case (== UINTN meaning UINT32), checking Year against 2106 makes sense as well. While build-testing the above-noted patches, I tried to build them for 32-bit ARM too, but that arch did not seem applicable: while the following command works:
> 
>   build \
>     -a AARCH64 \
>     -b NOOPT \
>     -p Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc \
>     -t GCC5 \
>     -m EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
> 
> the same with "-a ARM" fails, with the following error message:
> 
> edk2-platforms/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc(...): error 4000: Instance of library class [ArmSoftFloatLib] is not found
>         in [edk2/CryptoPkg/Library/OpensslLib/OpensslLib.inf] [ARM]
>         consumed by module [edk2/SignedCapsulePkg/Universal/SystemFirmwareUpdate/SystemFirmwareReportDxe.inf]
> 
> which implies that "Armada80x0McBin.dsc" is never actually built for ARM.
> 
> ... Hm, the following platforms:
> 
> - Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> - Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> 
> do build the library for ARM. So this patch seems justified after all, as-posted (i.e., including the year 2106 check).
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 

Pushed as 94e9fba43d7e..794979b1ee14

Thanks!