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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <AB3E8CB7-76CE-4F1D-B47C-57D63E56E6D7@dilger.ca>
Date:   Fri, 18 Aug 2017 10:09:07 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Deepa Dinamani <deepa.kernel@...il.com>
Cc:     Arnd Bergmann <arnd@...db.de>, 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 Aug 18, 2017, at 9:38 AM, Deepa Dinamani <deepa.kernel@...il.com> wrote:
> 
> On Fri, Aug 18, 2017 at 2:31 AM, Arnd Bergmann <arnd@...db.de> wrote:
>> On Fri, Aug 18, 2017 at 3:23 AM, Deepa Dinamani <deepa.kernel@...il.com> wrote:
>>> 
>>>> 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.
> 
> There is some confusion here.
> I was only referring to the current implementation:
> 
> static int recently_deleted(struct super_block *sb, ext4_group_t group, int ino)
> {
> .
> .
> .
>   unsigned long dtime, now;
>   int offset, ret = 0, recentcy = RECENTCY_MIN;
> .
> .
> .
>    offset = (ino % inodes_per_block) * EXT4_INODE_SIZE(sb);
>    raw_inode = (struct ext4_inode *) (bh->b_data + offset);
>    dtime = le32_to_cpu(raw_inode->i_dtime);
>    now = get_seconds();
>    if (buffer_dirty(bh))
>    recentcy += RECENTCY_DIRTY;
> 
>    if (dtime && (dtime < now) && (now < dtime + recentcy))
>         ret = 1;
> .
> .
> .
> }
> 
> In the above implementation, I do not see any problem on a 64 bit machine.
> The only problem is that dtime on disk representation is signed 32 bits only.
> If that were not a problem then this would be fine from time prespective.

The 32-bit dtime is the root of the problem.  There is no plan to extend
the dtime field on disk, because it is used so little (mostly as a boolean
value, and for forensics).

>>> 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.
> 
> Dtime on disk representation again breaks this for certain values in
> 2038 even though everything is unsigned.
> 
> I was just saying that whatever we do here depends on how dtime on
> disk is interpreted.
> 
> Agree that ktime_get_real_seconds() should be used here. But, the way
> we handle new values would rely on this new interpretation of dtime.
> Also, using time64_t variables on stack only matters after this. Once
> the types are corrected, maybe the comparison expression need not
> change at all (after new dtime interpretation is in place).

There will not be a new dtime format on disk, but since the calculation
here only depends on relative times (within a few minutes), then it would
be fine to use only 32-bit timestamps, and truncate off the high bits
from get_seconds()/ktime_get_real_seconds().

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)