| 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
| ||
|
Message-ID: <20151105181424.GD2213@birch.djwong.org>
Date: Thu, 5 Nov 2015 10:14:24 -0800
From: "Darrick J. Wong" <darrick.wong@...cle.com>
To: David Howells <dhowells@...hat.com>
Cc: tytso@....edu, kalpak@...sterfs.com, linux-fsdevel@...r.kernel.org,
linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
arnd@...db.de
Subject: Re: [PATCH] Ext4: Fix extended timestamp encoding and decoding
On Thu, Nov 05, 2015 at 02:49:16PM +0000, David Howells wrote:
> The handling of extended timestamps in Ext4 is broken as can be seen in the
> output of the test program attached below:
>
> time extra bad decode good decode bad encode good encode
> ======== ===== ================= ================= =========== ===========
> ffffffff 0 > ffffffffffffffff ffffffffffffffff > *ffffffff 3 ffffffff 0
> 80000000 0 > ffffffff80000000 ffffffff80000000 > *80000000 3 80000000 0
> 00000000 0 > 0 0 > 00000000 0 00000000 0
> 7fffffff 0 > 7fffffff 7fffffff > 7fffffff 0 7fffffff 0
> 80000000 1 > *ffffffff80000000 80000000 > *80000000 0 80000000 1
> ffffffff 1 > *ffffffffffffffff ffffffff > *ffffffff 0 ffffffff 1
> 00000000 1 > 100000000 100000000 > 00000000 1 00000000 1
> 7fffffff 1 > 17fffffff 17fffffff > 7fffffff 1 7fffffff 1
> 80000000 2 > *ffffffff80000000 180000000 > *80000000 1 80000000 2
> ffffffff 2 > *ffffffffffffffff 1ffffffff > *ffffffff 1 ffffffff 2
> 00000000 2 > 200000000 200000000 > 00000000 2 00000000 2
> 7fffffff 2 > 27fffffff 27fffffff > 7fffffff 2 7fffffff 2
> 80000000 3 > *ffffffff80000000 280000000 > *80000000 2 80000000 3
> ffffffff 3 > *ffffffffffffffff 2ffffffff > *ffffffff 2 ffffffff 3
> 00000000 3 > 300000000 300000000 > 00000000 3 00000000 3
> 7fffffff 3 > 37fffffff 37fffffff > 7fffffff 3 7fffffff 3
>
> The values marked with asterisks are wrong.
>
> The problem is that with a 64-bit time, in ext4_decode_extra_time() the
> epoch value is just OR'd with the sign-extended time - which, if negative,
> has all of the upper 32 bits set anyway. We need to add the epoch instead
> of OR'ing it. In ext4_encode_extra_time(), the reverse operation needs to
> take place as the 32-bit part of the number of seconds needs to be
> subtracted from the 64-bit value before the epoch is shifted down.
There's been a patch to fix this for a very long time:
http://thread.gmane.org/gmane.comp.file-systems.ext4/40978
and
https://bugzilla.kernel.org/show_bug.cgi?id=23732
...but I guess nobody ever developed the e2fsprogs regression tests that Ted
asked for, so none of the patches got merged. Ho hum.
Kernel patch looks ok though.
--D
>
> Since the epoch is presumably unsigned, this has the slightly strange
> effect of, for epochs > 0, putting the 0x80000000-0xffffffff range before
> the 0x00000000-0x7fffffff range.
>
> This affects all kernels from v2.6.23-rc1 onwards.
>
> The test program:
>
> #include <stdio.h>
>
> #define EXT4_FITS_IN_INODE(x, y, z) 1
> #define EXT4_EPOCH_BITS 2
> #define EXT4_EPOCH_MASK ((1 << EXT4_EPOCH_BITS) - 1)
> #define EXT4_NSEC_MASK (~0UL << EXT4_EPOCH_BITS)
>
> #define le32_to_cpu(x) (x)
> #define cpu_to_le32(x) (x)
> typedef unsigned int __le32;
> typedef unsigned int u32;
> typedef signed int s32;
> typedef unsigned long long __u64;
> typedef signed long long s64;
>
> struct timespec {
> long long tv_sec; /* seconds */
> long tv_nsec; /* nanoseconds */
> };
>
> struct ext4_inode_info {
> struct timespec i_crtime;
> };
>
> struct ext4_inode {
> __le32 i_crtime; /* File Creation time */
> __le32 i_crtime_extra; /* extra FileCreationtime (nsec << 2 | epoch) */
> };
>
> /* Incorrect implementation */
> static inline void ext4_decode_extra_time_bad(struct timespec *time, __le32 extra)
> {
> if (sizeof(time->tv_sec) > 4)
> time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK)
> << 32;
> time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS;
> }
>
> static inline __le32 ext4_encode_extra_time_bad(struct timespec *time)
> {
> return cpu_to_le32((sizeof(time->tv_sec) > 4 ?
> (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) |
> ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK));
> }
>
> /* Fixed implementation */
> static inline void ext4_decode_extra_time_good(struct timespec *time, __le32 _extra)
> {
> u32 extra = le32_to_cpu(_extra);
> u32 epoch = extra & EXT4_EPOCH_MASK;
>
> time->tv_sec = (s32)time->tv_sec + ((s64)epoch << 32);
> time->tv_nsec = (extra & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS;
> }
>
> static inline __le32 ext4_encode_extra_time_good(struct timespec *time)
> {
> u32 extra;
> s64 epoch = time->tv_sec - (s32)time->tv_sec;
>
> extra = (epoch >> 32) & EXT4_EPOCH_MASK;
> extra |= (time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK;
> return cpu_to_le32(extra);
> }
>
> #define EXT4_INODE_GET_XTIME_BAD(xtime, inode, raw_inode) \
> do { \
> (inode)->xtime.tv_sec = (signed)le32_to_cpu((raw_inode)->xtime); \
> if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \
> ext4_decode_extra_time_bad(&(inode)->xtime, \
> raw_inode->xtime ## _extra); \
> else \
> (inode)->xtime.tv_nsec = 0; \
> } while (0)
>
> #define EXT4_INODE_SET_XTIME_BAD(xtime, inode, raw_inode) \
> do { \
> (raw_inode)->xtime = cpu_to_le32((inode)->xtime.tv_sec); \
> if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \
> (raw_inode)->xtime ## _extra = \
> ext4_encode_extra_time_bad(&(inode)->xtime); \
> } while (0)
>
> #define EXT4_INODE_GET_XTIME_GOOD(xtime, inode, raw_inode) \
> do { \
> (inode)->xtime.tv_sec = (signed)le32_to_cpu((raw_inode)->xtime); \
> if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \
> ext4_decode_extra_time_good(&(inode)->xtime, \
> raw_inode->xtime ## _extra); \
> else \
> (inode)->xtime.tv_nsec = 0; \
> } while (0)
>
> #define EXT4_INODE_SET_XTIME_GOOD(xtime, inode, raw_inode) \
> do { \
> (raw_inode)->xtime = cpu_to_le32((inode)->xtime.tv_sec); \
> if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \
> (raw_inode)->xtime ## _extra = \
> ext4_encode_extra_time_good(&(inode)->xtime); \
> } while (0)
>
> static const struct test {
> unsigned crtime;
> unsigned extra;
> long long sec;
> int nsec;
> } tests[] = {
> // crtime extra tv_sec tv_nsec
> 0xffffffff, 0x00000000, 0xffffffffffffffff, 0,
> 0x80000000, 0x00000000, 0xffffffff80000000, 0,
> 0x00000000, 0x00000000, 0x0000000000000000, 0,
> 0x7fffffff, 0x00000000, 0x000000007fffffff, 0,
> 0x80000000, 0x00000001, 0x0000000080000000, 0,
> 0xffffffff, 0x00000001, 0x00000000ffffffff, 0,
> 0x00000000, 0x00000001, 0x0000000100000000, 0,
> 0x7fffffff, 0x00000001, 0x000000017fffffff, 0,
> 0x80000000, 0x00000002, 0x0000000180000000, 0,
> 0xffffffff, 0x00000002, 0x00000001ffffffff, 0,
> 0x00000000, 0x00000002, 0x0000000200000000, 0,
> 0x7fffffff, 0x00000002, 0x000000027fffffff, 0,
> 0x80000000, 0x00000003, 0x0000000280000000, 0,
> 0xffffffff, 0x00000003, 0x00000002ffffffff, 0,
> 0x00000000, 0x00000003, 0x0000000300000000, 0,
> 0x7fffffff, 0x00000003, 0x000000037fffffff, 0,
> };
>
> int main()
> {
> struct ext4_inode_info ii_bad, ii_good;
> struct ext4_inode raw, *praw = &raw;
> struct ext4_inode raw_bad, *praw_bad = &raw_bad;
> struct ext4_inode raw_good, *praw_good = &raw_good;
> const struct test *t;
> int i, ret = 0;
>
> printf("time extra bad decode good decode bad encode good encode\n");
> printf("======== ===== ================= ================= =========== ===========\n");
> for (i = 0; i < sizeof(tests) / sizeof(t[0]); i++) {
> t = &tests[i];
> raw.i_crtime = t->crtime;
> raw.i_crtime_extra = t->extra;
> printf("%08x %5d > ", t->crtime, t->extra);
>
> EXT4_INODE_GET_XTIME_BAD(i_crtime, &ii_bad, praw);
> EXT4_INODE_GET_XTIME_GOOD(i_crtime, &ii_good, praw);
> if (ii_bad.i_crtime.tv_sec != t->sec ||
> ii_bad.i_crtime.tv_nsec != t->nsec)
> printf("*");
> else
> printf(" ");
> printf("%16llx", ii_bad.i_crtime.tv_sec);
> printf(" ");
> if (ii_good.i_crtime.tv_sec != t->sec ||
> ii_good.i_crtime.tv_nsec != t->nsec) {
> printf("*");
> ret = 1;
> } else {
> printf(" ");
> }
> printf("%16llx", ii_good.i_crtime.tv_sec);
>
> EXT4_INODE_SET_XTIME_BAD(i_crtime, &ii_good, praw_bad);
> EXT4_INODE_SET_XTIME_GOOD(i_crtime, &ii_good, praw_good);
>
> printf(" > ");
> if (raw_bad.i_crtime != raw.i_crtime ||
> raw_bad.i_crtime_extra != raw.i_crtime_extra)
> printf("*");
> else
> printf(" ");
> printf("%08llx %d", raw_bad.i_crtime, raw_bad.i_crtime_extra);
> printf(" ");
>
> if (raw_good.i_crtime != raw.i_crtime ||
> raw_good.i_crtime_extra != raw.i_crtime_extra) {
> printf("*");
> ret = 1;
> } else {
> printf(" ");
> }
> printf("%08llx %d", raw_good.i_crtime, raw_good.i_crtime_extra);
> printf("\n");
> }
>
> return ret;
> }
>
> Signed-off-by: David Howells <dhowells@...hat.com>
> ---
>
> fs/ext4/ext4.h | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index fd1f28be5296..31efcd78bf51 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -723,19 +723,23 @@ struct move_extent {
> <= (EXT4_GOOD_OLD_INODE_SIZE + \
> (einode)->i_extra_isize)) \
>
> -static inline __le32 ext4_encode_extra_time(struct timespec *time)
> +static inline void ext4_decode_extra_time(struct timespec *time, __le32 _extra)
> {
> - return cpu_to_le32((sizeof(time->tv_sec) > 4 ?
> - (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) |
> - ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK));
> + u32 extra = le32_to_cpu(_extra);
> + u32 epoch = extra & EXT4_EPOCH_MASK;
> +
> + time->tv_sec = (s32)time->tv_sec + ((s64)epoch << 32);
> + time->tv_nsec = (extra & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS;
> }
>
> -static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra)
> +static inline __le32 ext4_encode_extra_time(struct timespec *time)
> {
> - if (sizeof(time->tv_sec) > 4)
> - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK)
> - << 32;
> - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS;
> + u32 extra;
> + s64 epoch = time->tv_sec - (s32)time->tv_sec;
> +
> + extra = (epoch >> 32) & EXT4_EPOCH_MASK;
> + extra |= (time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK;
> + return cpu_to_le32(extra);
> }
>
> #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists