* [edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime @ 2020-12-21 16:37 Marcin Wojtas 2020-12-21 20:09 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Marcin Wojtas @ 2020-12-21 16:37 UTC (permalink / raw) To: devel; +Cc: leif, ard.biesheuvel, mw, jaz, kostap 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime 2020-12-21 16:37 [edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime Marcin Wojtas @ 2020-12-21 20:09 ` Laszlo Ersek 2020-12-21 20:32 ` Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2020-12-21 20:09 UTC (permalink / raw) To: devel, mw; +Cc: leif, ard.biesheuvel, jaz, kostap 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime 2020-12-21 20:09 ` [edk2-devel] " Laszlo Ersek @ 2020-12-21 20:32 ` Laszlo Ersek 2021-01-04 17:14 ` Ard Biesheuvel 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2020-12-21 20:32 UTC (permalink / raw) To: devel, mw; +Cc: leif, ard.biesheuvel, jaz, kostap 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime 2020-12-21 20:32 ` Laszlo Ersek @ 2021-01-04 17:14 ` Ard Biesheuvel 0 siblings, 0 replies; 4+ messages in thread From: Ard Biesheuvel @ 2021-01-04 17:14 UTC (permalink / raw) To: Laszlo Ersek, devel, mw; +Cc: leif, jaz, kostap 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! ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-04 17:14 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-21 16:37 [edk2-platforms PATCH] Marvell/RealTimeClockLib: Allow only Unix Epoch in LibSetWakeupTime Marcin Wojtas 2020-12-21 20:09 ` [edk2-devel] " Laszlo Ersek 2020-12-21 20:32 ` Laszlo Ersek 2021-01-04 17:14 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox