linux-ext4 - Re: [PATCH v1 5/10] ext4: Add DX_HASH

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: <0765A511-D5E2-4F00-8F2F-F5A8E76C0B03@dilger.ca>
Date:	Tue, 23 Sep 2014 21:12:10 +0200
From:	Andreas Dilger <adilger@...ger.ca>
To:	George Spelvin <linux@...izon.com>
Cc:	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
	"tytso@....edu" <tytso@....edu>
Subject: Re: [PATCH v1 5/10] ext4: Add DX_HASH_SIPHASH24 support

On Sep 23, 2014, at 12:16, "George Spelvin" <linux@...izon.com> wrote:
> 
> This uses SipHash-2-4 for directory hashing.
> 
> Signed-off-by: George Spelvin <linux@...izon.com>
> ---
> The actual feature.  Pretty simple, no?

Simple, except that it introduces a serious bug in the code. It duplicates
the previous on-disk encoding of:
  DX_HASH_LEGACY | DX_HASH_UNSIGNED_DELTA = 3

with:
  DX_HASH_SIPHASH24    3

and that will mean old filesystems would be considered corrupted. 

Due to the changes of your previous patch, it is not at all clear at the
definition of DX_HASH_* that the values 3, 4, 5 are already used.

> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index cda8dd9c..13420bd4 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1737,6 +1737,7 @@ static inline __le16 ext4_rec_len_to_disk(unsigned len, unsigned blocksize)
> #define DX_HASH_LEGACY        0
> #define DX_HASH_HALF_MD4    1
> #define DX_HASH_TEA        2
> +#define DX_HASH_SIPHASH24    3

This should still explicitly #define the DX_HASH_LEGACY_UNSIGNED
values so that it is clear the values are already in use, otherwise we
expose ourselves to this bug repeatedly in the future. 

Cheers, Andreas
> /*
>  * For historical reasons, the first three hash algorithms
> @@ -1744,7 +1745,7 @@ static inline __le16 ext4_rec_len_to_disk(unsigned len, unsigned blocksize)
>  * use only, define some extra values outside the range of
>  * what's allowed on disk.
>  */
> -#define DX_HASH_UNSIGNED_DELTA    3
> +#define DX_HASH_UNSIGNED_DELTA    4
> 
> #define DX_HASH_LEGACY_UNSIGNED   (DX_HASH_LEGACY   + DX_HASH_UNSIGNED_DELTA)
> #define DX_HASH_HALF_MD4_UNSIGNED (DX_HASH_HALF_MD4 + DX_HASH_UNSIGNED_DELTA)
> diff --git a/fs/ext4/hash.c b/fs/ext4/hash.c
> index 5186e09a..584639f4 100644
> --- a/fs/ext4/hash.c
> +++ b/fs/ext4/hash.c
> @@ -140,6 +140,7 @@ int ext4fs_dirhash(const char *name, int len, struct dx_hash_info *hinfo)
> {
>    __u32    hash;
>    __u32    minor_hash = 0;
> +    __u64    hash64;
>    int    i;
>    __u32    in[8], buf[4];
>    void    (*str2hashbuf)(const char *, int, __u32 *, int) =
> @@ -192,6 +193,11 @@ int ext4fs_dirhash(const char *name, int len, struct dx_hash_info *hinfo)
>        hash = buf[0];
>        minor_hash = buf[1];
>        break;
> +    case DX_HASH_SIPHASH24:
> +        hash64 = siphash24(name, len, hinfo->seed);
> +        hash = (__u32)hash64;
> +        minor_hash = hash64 >> 32;
> +        break;
>    default:
>        hinfo->hash = 0;
>        return -1;
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 9e6eced1..922d6b48 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -685,7 +685,8 @@ dx_probe(const struct qstr *d_name, struct inode *dir,
>        goto fail;
>    }
>    root = (struct dx_root *) bh->b_data;
> -    if (root->info.hash_version != DX_HASH_TEA &&
> +    if (root->info.hash_version != DX_HASH_SIPHASH24 &&
> +        root->info.hash_version != DX_HASH_TEA &&
>        root->info.hash_version != DX_HASH_HALF_MD4 &&
>        root->info.hash_version != DX_HASH_LEGACY) {
>        ext4_warning(dir->i_sb, "Unrecognised inode hash code %d",
> -- 
> 2.1.0
> 
> --
> 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