linux-ext4 - Re: Y2038 bug in ext4 recently

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a2mu9SGdZhWX59OhSK-fokSEpjKsK4_HC1OC-XxZU8eTw@mail.gmail.com>
Date:   Fri, 18 Aug 2017 11:31:25 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Deepa Dinamani <deepa.kernel@...il.com>
Cc:     Andreas Dilger <adilger@...ger.ca>,
        "Theodore Ts'o" <tytso@....edu>, Wang Shilong <wshilong@....com>,
        Wang Shilong <wangshilong1991@...il.com>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        Shuichi Ihara <sihara@....com>, Li Xi <lixi@....com>,
        Jan Kara <jack@...e.cz>
Subject: Re: Y2038 bug in ext4 recently_deleted() function

On Fri, Aug 18, 2017 at 3:23 AM, Deepa Dinamani <deepa.kernel@...il.com> wrote:
>> Strange, I never even knew recently_deleted() existed, even though it was
>> added to the tree 4 years ago yesterday.  It looks like this is only used
>> with the no-journal code, which I don't really interact with.
>>
>> One thing I did notice when looking at it is that there is a Y2038 bug in
>> recently_deleted(), as it is comparing 32-bit i_dtime directly with 64-bit
>> get_seconds().
>
> I don't think dtime has widened on the disk layout for ext4 according
> to https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout. So I am
> not sure how fixing the internal implementation would be useful until
> we do that. Is there a plan for that?
>
> As far as get_seconds() is concerned, get_seconds() returns unsigned
> long which is 64 bits on a 64 bit arch and 32 bit on a 32 bit arch.
> Since dtime variable is declared as unsigned long in this function,
> same holds for the size of this variable.
>
> There is no y2038 problem on a 64 bit machine.

I think what Andreas was saying is that it's actually the opposite:
on a 32-bit machine, the code will work correctly for 32-bit unsigned
long values as long as 'dtime' and 'now' are in the same epoch,
e.g. both are before 2106 or both are after.

On 64-bit systems it's always wrong after 2106.

> So moving to the case of a 32 bit machine:
>
> get_seconds() can return values until year 2106. And, recentcy at max
> can only be 35. Analyzing the current line:
>
> if (dtime && (dtime < now) && (now < dtime + recentcy))
>
> The above equation should work fine at least until 35 seconds before
> y2038 deadline.

Since it's all unsigned arithmetic, it should be fine until 2106.
However, we should get rid of get_seconds() long before then
and use ktime_get_real_seconds() instead, as most other users
of get_seconds() are (more) broken.

Looking at the two suggested approaches:

>>        u32 now, dtime;
>>
>>        /* assume dtime is within the past 30 years, see time_after() */
>>        now = get_seconds();
>>        if (dtime && (dtime - now < 0) && (dtime + recentcy - now < 0))
>>                ret = 1;

* As 'dtime' and 'now' are both unsigned, subtracting them will also result
  in an unsigned value that is never less than zero, so it won't work.
  Adding a cast to 's32' would fix that the same way that time_after() does.

* please use ktime_get_real_seconds() instead of get_seconds(), so we
   don't have to replace it later.

* The comment should say '68 years', not 30.

> or use i_ctime_extra to implicitly extend i_dtime beyond 2038, something like:
>
>        /* assume dtime epoch same as ctime, see EXT4_INODE_GET_XTIME() */
>        dtime = le32_to_cpu(raw_inode->i_dtime);
>        if (EXT4_INODE_SIZE(sb) > EXT4_GOOD_OLD_INODE_SIZE &&
>            offsetof(typeof(*raw_inode), i_ctime_extra) + 4 <=
>            EXT4_GOOD_OLD_INODE_SIZE + le32_to_cpu(raw_inode->i_extra_isize))
>                dtime += (long)(le32_to_cpu(raw_inode->i_ctime_extra) &
>                                EXT4_EPOCH_MASK) << 32;

* This is slightly incorrect when we are close to the epoch boundary, as i_ctime
  and i_dtime might end up being in different epochs. I would not go there.

* If we were to pick this approach, a cast to 'long' is obviously wrong on
  32-bit systems, better use 'u64' or 'time64_t'.

     Arnd