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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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!