Re: [PATCH] fat: Allow out-of-range FAT modification timestamps

[Daniel Kiper]

On Wed, Aug 25, 2021 at 08:51:09AM +0200, Heinrich Schuchardt wrote:
> On 8/16/21 4:59 PM, Tomasz Kramkowski via Grub-devel wrote:
> > 20def1a3c introduced support for file modification times to allow
> > comparison of file ages on EFI systems. This patch used
> > grub_datetime2unixtime which uses a 32 bit unix timestamp and as a
> > result did not allow the full range of times that FAT timestamps do.
> >
> > In some situations a file with a timestamp of 1970-01-01 gets
> > transferred to a FAT partition, the timestamp ends up as 2098-01-01
> > because of FAT's use of the 1980-01-01 DOS epoch and lack of negative
> > timestamps.
> >
> > Since 2098 is after 2038, this date cannot fit in a 32 bit timestamp.
> >
> > Ideally grub should use 64 bit timestamps but I have not investigated
> > what kind of work would be required to support this.
>
> Field mtime of struct grub_dirhook_info is already 64bit. See commit
> 81f1962393f4 ("fs: Use 64-bit type for filesystem timestamp"). Function
> grub_datetime2unixtime needs to be fixed: The 2037 check should be
> removed and the code has to be adjusted to correctly treat year 2100.
>
> grub_unixtime2datetime already handles 64bit timestamps but ignores that
> year 2100 is not a leap year:
>
> grub_unixtime2datetime(4107585600) = 2100-02-29 12:00:00
>
> >
> > This fixes bug #60565.
> >
> > Reported-by: Naïm Favier <n+grub@monade.li>
> > Tested-by: Naïm Favier <n+grub@monade.li>
> > Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
> > ---
> >   grub-core/fs/fat.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/grub-core/fs/fat.c b/grub-core/fs/fat.c
> > index dd82e4ee3..34589d7db 100644
> > --- a/grub-core/fs/fat.c
> > +++ b/grub-core/fs/fat.c
> > @@ -1020,16 +1020,17 @@ grub_fat_dir (grub_device_t device, const char 
> > *path, grub_fs_dir_hook_t hook,
> >         info.mtimeset = grub_exfat_timestamp (grub_le_to_cpu32 
> > (ctxt.entry.type_specific.file.m_time),
> >                                         
> > ctxt.entry.type_specific.file.m_time_tenth,
> >                                         &info.mtime);
> > +      if (info.mtimeset == 0)
> > +   grub_dprintf("exfat", "invalid modification timestamp for %s\n", path);
>
> According to the commit message there is nothing invalid about the
> modification timestamp. It is a GRUB deficiency that it cannot handle
> the same date range as FAT and exFAT.
>
> So the message could be:
> "GRUB cannot handle the modification timestamp for %s".
>
> >   #else
> >         if (ctxt.dir.attr & GRUB_FAT_ATTR_VOLUME_ID)
> >     continue;
> >         info.mtimeset = grub_fat_timestamp (grub_le_to_cpu16 
> > (ctxt.dir.w_time),
> >                                       grub_le_to_cpu16 (ctxt.dir.w_date),
> >                                       &info.mtime);
> > -#endif
> >         if (info.mtimeset == 0)
> > -   grub_error (GRUB_ERR_OUT_OF_RANGE,
> > -               "invalid modification timestamp for %s", path);
> > +   grub_dprintf("fat", "invalid modification timestamp for %s\n", path);
>
> I suggest to use grub_error() for both messages.

Tomasz, I agree with Heinrich. I am happy to take updated patch.
Could you make it?

Daniel