[PATCH] crypto: xxhash - Implement xxhash support

[PATCH] crypto: xxhash - Implement xxhash support

[Nikolay Borisov]

xxhash is currently implemented as a self-contained module in /lib.
This patch enables that module to be used as part of the generic kernel
crypto framework. It adds a simple wrapper to the 64bit version. I've
also added a couple of simplistic test vectors to ensure I haven't
screwed up anything doing the plumbing.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 crypto/Kconfig          |   8 +++
 crypto/Makefile         |   1 +
 crypto/testmgr.c        |   7 +++
 crypto/testmgr.h        |  26 ++++++++++
 crypto/xxhash_generic.c | 112 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 154 insertions(+)
 create mode 100644 crypto/xxhash_generic.c

diff --git a/crypto/Kconfig b/crypto/Kconfig
index bbab6bf33519..c56cc450ffc0 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -665,6 +665,14 @@ config CRYPTO_CRC32_MIPS
 	  instructions, when available.
 
 
+config CRYPTO_XXHASH
+	tristate "xxHash hash algorithm"
+	select CRYPTO_HASH
+	select XXHASH
+	help
+	  xxHash non-cryptographic hash algorithm. Extremely fast, working at
+	  speeds close to RAM limits.
+
 config CRYPTO_CRCT10DIF
 	tristate "CRCT10DIF algorithm"
 	select CRYPTO_HASH
diff --git a/crypto/Makefile b/crypto/Makefile
index fb5bf2a3a666..4dc6c45d026e 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -135,6 +135,7 @@ obj-$(CONFIG_CRYPTO_842) += 842.o
 obj-$(CONFIG_CRYPTO_RNG2) += rng.o
 obj-$(CONFIG_CRYPTO_ANSI_CPRNG) += ansi_cprng.o
 obj-$(CONFIG_CRYPTO_DRBG) += drbg.o
+obj-$(CONFIG_CRYPTO_XXHASH) += xxhash_generic.o
 obj-$(CONFIG_CRYPTO_JITTERENTROPY) += jitterentropy_rng.o
 CFLAGS_jitterentropy.o = -O0
 jitterentropy_rng-y := jitterentropy.o jitterentropy-kcapi.o
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 8386038d67c7..322e906b6b6a 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -3879,6 +3879,13 @@ static const struct alg_test_desc alg_test_descs[] = {
 		.alg = "xts512(paes)",
 		.test = alg_test_null,
 		.fips_allowed = 1,
+	}, {
+		.alg = "xxhash64",
+		.test = alg_test_hash,
+		.fips_allowed = 1,
+		.suite = {
+			.hash = __VECS(xxhash64_tv_template)
+		}
 	}, {
 		.alg = "zlib-deflate",
 		.test = alg_test_comp,
diff --git a/crypto/testmgr.h b/crypto/testmgr.h
index d18a37629f05..8e0a9385ee5d 100644
--- a/crypto/testmgr.h
+++ b/crypto/testmgr.h
@@ -33218,6 +33218,32 @@ static const struct hash_testvec crc32c_tv_template[] = {
 	}
 };
 
+static const struct hash_testvec xxhash64_tv_template[] = {
+	{
+		.psize = 0,
+		.digest = "\x99\xe9\xd8\x51\x37\xdb\x46\xef",
+	},
+	{
+		.plaintext = "abcdefg",
+		.psize = 7,
+		.digest = "\x2d\x82\x02\x29\x0e\x94\x60\x18",
+	},
+	{
+		.plaintext = "0123456789abcdef0123456789abcdef0123456789"
+			     "abcdef0123456789abcdef",
+		.psize = 64,
+		.digest = "\x85\x2f\xfe\x60\x47\xac\xf3\x1a",
+	},
+	{
+		.key = "\xd2\x02\x96\x49\x00\x00\x00\x00",
+		.ksize = 8,
+		.plaintext = "0123456789abcdef0123456789abcdef0123456789"
+			     "abcdef0123456789abcdef",
+		.psize = 64,
+		.digest = "\xab\xea\xc2\x48\x1a\x80\x4e\x7b",
+	},
+};
+
 static const struct comp_testvec lz4_comp_tv_template[] = {
 	{
 		.inlen	= 255,
diff --git a/crypto/xxhash_generic.c b/crypto/xxhash_generic.c
new file mode 100644
index 000000000000..aedaabe55d45
--- /dev/null
+++ b/crypto/xxhash_generic.c
@@ -0,0 +1,112 @@
+#include <crypto/internal/hash.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/xxhash.h>
+
+#define XXHASH_BLOCK_SIZE	1
+#define XXHASH64_DIGEST_SIZE	8
+
+struct xxhash64_crypto_ctx {
+	u64 seed;
+};
+
+struct xxhash64_desc_ctx {
+	struct xxh64_state xxhstate;
+	u64 seed;
+};
+
+static int xxhash64_init(struct shash_desc *desc)
+{
+	struct xxhash64_crypto_ctx *cctx = crypto_shash_ctx(desc->tfm);
+	struct xxhash64_desc_ctx *dctx = shash_desc_ctx(desc);
+
+	dctx->seed = cctx->seed;
+	xxh64_reset(&dctx->xxhstate, dctx->seed);
+
+	return 0;
+}
+
+static int xxhash64_setkey(struct crypto_shash *tfm, const u8 *key,
+			 unsigned int keylen)
+{
+	struct xxhash64_crypto_ctx *ctx = crypto_shash_ctx(tfm);
+
+	if (keylen != sizeof(ctx->seed)) {
+		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+		return -EINVAL;
+	}
+	ctx->seed = *(u64 *)key;
+	return 0;
+}
+
+static int xxhash64_update(struct shash_desc *desc, const u8 *data,
+			 unsigned int length)
+{
+	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
+
+	xxh64_update(&ctx->xxhstate, data, length);
+
+	return 0;
+}
+
+static int xxhash64_final(struct shash_desc *desc, u8 *out)
+{
+	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
+
+	*(u64 *)out = xxh64_digest(&ctx->xxhstate);
+
+	return 0;
+}
+
+static int xxhash64_finup(struct shash_desc *desc, const u8 *data,
+			unsigned int len, u8 *out)
+{
+	xxhash64_update(desc, data, len);
+	xxhash64_final(desc, out);
+
+	return 0;
+}
+
+static int xxhash64_digest(struct shash_desc *desc, const u8 *data,
+			 unsigned int length, u8 *out)
+{
+	return xxhash64_finup(desc, data, length, out);
+}
+
+static struct shash_alg alg = {
+	.digestsize		=	XXHASH64_DIGEST_SIZE,
+	.setkey			= xxhash64_setkey,
+	.init		=	xxhash64_init,
+	.update		=	xxhash64_update,
+	.final		=	xxhash64_final,
+	.finup		=	xxhash64_finup,
+	.digest		=	xxhash64_digest,
+	.descsize		=	sizeof(struct xxhash64_desc_ctx),
+	.base			=	{
+		.cra_name		=	"xxhash64",
+		.cra_driver_name	=	"xxhash64-generic",
+		.cra_priority		=	100,
+		.cra_flags		=	CRYPTO_ALG_OPTIONAL_KEY,
+		.cra_blocksize		=	XXHASH_BLOCK_SIZE,
+		.cra_ctxsize		=	sizeof(struct xxhash64_crypto_ctx),
+		.cra_module		=	THIS_MODULE,
+	}
+};
+
+static int __init xxhash_mod_init(void)
+{
+	return crypto_register_shash(&alg);
+}
+
+static void __exit xxhash_mod_fini(void)
+{
+	crypto_unregister_shash(&alg);
+}
+
+module_init(xxhash_mod_init);
+module_exit(xxhash_mod_fini);
+
+MODULE_AUTHOR("Nikolay Borisov <nborisov@suse.com>");
+MODULE_DESCRIPTION("xxhash  calculations wrapper for lib/xxhash.c");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_CRYPTO("xxhash-generic");
-- 
2.17.1

Re: [PATCH] crypto: xxhash - Implement xxhash support

[Stephan Mueller]

Am Montag, 27. Mai 2019, 16:28:10 CEST schrieb Nikolay Borisov:

Hi Nikolay,

> xxhash is currently implemented as a self-contained module in /lib.
> This patch enables that module to be used as part of the generic kernel
> crypto framework. It adds a simple wrapper to the 64bit version. I've
> also added a couple of simplistic test vectors to ensure I haven't
> screwed up anything doing the plumbing.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  crypto/Kconfig          |   8 +++
>  crypto/Makefile         |   1 +
>  crypto/testmgr.c        |   7 +++
>  crypto/testmgr.h        |  26 ++++++++++
>  crypto/xxhash_generic.c | 112 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 154 insertions(+)
>  create mode 100644 crypto/xxhash_generic.c
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index bbab6bf33519..c56cc450ffc0 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -665,6 +665,14 @@ config CRYPTO_CRC32_MIPS
>  	  instructions, when available.
> 
> 
> +config CRYPTO_XXHASH
> +	tristate "xxHash hash algorithm"
> +	select CRYPTO_HASH
> +	select XXHASH
> +	help
> +	  xxHash non-cryptographic hash algorithm. Extremely fast, working at
> +	  speeds close to RAM limits.
> +
>  config CRYPTO_CRCT10DIF
>  	tristate "CRCT10DIF algorithm"
>  	select CRYPTO_HASH
> diff --git a/crypto/Makefile b/crypto/Makefile
> index fb5bf2a3a666..4dc6c45d026e 100644
> --- a/crypto/Makefile
> +++ b/crypto/Makefile
> @@ -135,6 +135,7 @@ obj-$(CONFIG_CRYPTO_842) += 842.o
>  obj-$(CONFIG_CRYPTO_RNG2) += rng.o
>  obj-$(CONFIG_CRYPTO_ANSI_CPRNG) += ansi_cprng.o
>  obj-$(CONFIG_CRYPTO_DRBG) += drbg.o
> +obj-$(CONFIG_CRYPTO_XXHASH) += xxhash_generic.o

Cosmetic question: why is the hash object added inbetween the block of RNG 
objects?

>  obj-$(CONFIG_CRYPTO_JITTERENTROPY) += jitterentropy_rng.o
>  CFLAGS_jitterentropy.o = -O0
>  jitterentropy_rng-y := jitterentropy.o jitterentropy-kcapi.o
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 8386038d67c7..322e906b6b6a 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -3879,6 +3879,13 @@ static const struct alg_test_desc alg_test_descs[] =
> { .alg = "xts512(paes)",
>  		.test = alg_test_null,
>  		.fips_allowed = 1,
> +	}, {
> +		.alg = "xxhash64",
> +		.test = alg_test_hash,
> +		.fips_allowed = 1,

Why is this intended to be allowed in FIPS mode? This does not seem to be a 
FIPS approved cipher.

> +		.suite = {
> +			.hash = __VECS(xxhash64_tv_template)
> +		}
>  	}, {
>  		.alg = "zlib-deflate",
>  		.test = alg_test_comp,
> diff --git a/crypto/testmgr.h b/crypto/testmgr.h
> index d18a37629f05..8e0a9385ee5d 100644
> --- a/crypto/testmgr.h
> +++ b/crypto/testmgr.h
> @@ -33218,6 +33218,32 @@ static const struct hash_testvec
> crc32c_tv_template[] = { }
>  };
> 
> +static const struct hash_testvec xxhash64_tv_template[] = {

Could you please add a note where these vectors come from?

> +	{
> +		.psize = 0,
> +		.digest = "\x99\xe9\xd8\x51\x37\xdb\x46\xef",
> +	},
> +	{
> +		.plaintext = "abcdefg",
> +		.psize = 7,
> +		.digest = "\x2d\x82\x02\x29\x0e\x94\x60\x18",
> +	},
> +	{
> +		.plaintext = "0123456789abcdef0123456789abcdef0123456789"
> +			     "abcdef0123456789abcdef",
> +		.psize = 64,
> +		.digest = "\x85\x2f\xfe\x60\x47\xac\xf3\x1a",
> +	},
> +	{
> +		.key = "\xd2\x02\x96\x49\x00\x00\x00\x00",
> +		.ksize = 8,
> +		.plaintext = "0123456789abcdef0123456789abcdef0123456789"
> +			     "abcdef0123456789abcdef",
> +		.psize = 64,
> +		.digest = "\xab\xea\xc2\x48\x1a\x80\x4e\x7b",
> +	},
> +};
> +
>  static const struct comp_testvec lz4_comp_tv_template[] = {
>  	{
>  		.inlen	= 255,
> diff --git a/crypto/xxhash_generic.c b/crypto/xxhash_generic.c
> new file mode 100644
> index 000000000000..aedaabe55d45
> --- /dev/null
> +++ b/crypto/xxhash_generic.c
> @@ -0,0 +1,112 @@

What about adding the license information compliant with SPDX?

> +#include <crypto/internal/hash.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/xxhash.h>
> +
> +#define XXHASH_BLOCK_SIZE	1
> +#define XXHASH64_DIGEST_SIZE	8
> +
> +struct xxhash64_crypto_ctx {
> +	u64 seed;
> +};
> +
> +struct xxhash64_desc_ctx {
> +	struct xxh64_state xxhstate;
> +	u64 seed;
> +};
> +
> +static int xxhash64_init(struct shash_desc *desc)
> +{
> +	struct xxhash64_crypto_ctx *cctx = crypto_shash_ctx(desc->tfm);
> +	struct xxhash64_desc_ctx *dctx = shash_desc_ctx(desc);
> +
> +	dctx->seed = cctx->seed;
> +	xxh64_reset(&dctx->xxhstate, dctx->seed);
> +
> +	return 0;
> +}
> +
> +static int xxhash64_setkey(struct crypto_shash *tfm, const u8 *key,
> +			 unsigned int keylen)
> +{
> +	struct xxhash64_crypto_ctx *ctx = crypto_shash_ctx(tfm);
> +
> +	if (keylen != sizeof(ctx->seed)) {
> +		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return -EINVAL;
> +	}
> +	ctx->seed = *(u64 *)key;
> +	return 0;
> +}
> +
> +static int xxhash64_update(struct shash_desc *desc, const u8 *data,
> +			 unsigned int length)
> +{
> +	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> +	xxh64_update(&ctx->xxhstate, data, length);
> +
> +	return 0;
> +}
> +
> +static int xxhash64_final(struct shash_desc *desc, u8 *out)
> +{
> +	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> +	*(u64 *)out = xxh64_digest(&ctx->xxhstate);
> +
> +	return 0;
> +}
> +
> +static int xxhash64_finup(struct shash_desc *desc, const u8 *data,
> +			unsigned int len, u8 *out)
> +{
> +	xxhash64_update(desc, data, len);
> +	xxhash64_final(desc, out);
> +
> +	return 0;
> +}
> +
> +static int xxhash64_digest(struct shash_desc *desc, const u8 *data,
> +			 unsigned int length, u8 *out)
> +{
> +	return xxhash64_finup(desc, data, length, out);
> +}
> +
> +static struct shash_alg alg = {
> +	.digestsize		=	XXHASH64_DIGEST_SIZE,
> +	.setkey			= xxhash64_setkey,
> +	.init		=	xxhash64_init,
> +	.update		=	xxhash64_update,
> +	.final		=	xxhash64_final,
> +	.finup		=	xxhash64_finup,
> +	.digest		=	xxhash64_digest,
> +	.descsize		=	sizeof(struct xxhash64_desc_ctx),
> +	.base			=	{
> +		.cra_name		=	"xxhash64",
> +		.cra_driver_name	=	"xxhash64-generic",
> +		.cra_priority		=	100,
> +		.cra_flags		=	CRYPTO_ALG_OPTIONAL_KEY,
> +		.cra_blocksize		=	XXHASH_BLOCK_SIZE,
> +		.cra_ctxsize		=	sizeof(struct 
xxhash64_crypto_ctx),
> +		.cra_module		=	THIS_MODULE,
> +	}
> +};
> +
> +static int __init xxhash_mod_init(void)
> +{
> +	return crypto_register_shash(&alg);
> +}
> +
> +static void __exit xxhash_mod_fini(void)
> +{
> +	crypto_unregister_shash(&alg);
> +}
> +
> +module_init(xxhash_mod_init);
> +module_exit(xxhash_mod_fini);
> +
> +MODULE_AUTHOR("Nikolay Borisov <nborisov@suse.com>");
> +MODULE_DESCRIPTION("xxhash  calculations wrapper for lib/xxhash.c");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_CRYPTO("xxhash-generic");



Ciao
Stephan

Re: [PATCH] crypto: xxhash - Implement xxhash support

[Eric Biggers]

Hi Nikolay,

On Mon, May 27, 2019 at 05:28:10PM +0300, Nikolay Borisov wrote:
> xxhash is currently implemented as a self-contained module in /lib.
> This patch enables that module to be used as part of the generic kernel
> crypto framework. It adds a simple wrapper to the 64bit version. I've
> also added a couple of simplistic test vectors to ensure I haven't
> screwed up anything doing the plumbing.

What is this planned to be used for?

Please also run the crypto self-tests (i.e. boot a kernel with
CONFIG_CRYPTO_MANAGER_DISABLE_TESTS unset) for all crypto API patches.
When that's done this code immediately crashes, see output below.

More comments below.

	[    0.305235] BUG: unable to handle page fault for address: ffff88817c1966fe
	[    0.306613] #PF: supervisor write access in kernel mode
	[    0.307653] #PF: error_code(0x0002) - not-present page
	[    0.308503] PGD 2a01067 P4D 2a01067 PUD 0 
	[    0.308503] Oops: 0002 [#1] SMP
	[    0.308503] CPU: 3 PID: 59 Comm: cryptomgr_test Not tainted 5.2.0-rc2-00001-g4fcf4df09e23d #3
	[    0.308503] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014
	[    0.308503] RIP: 0010:__memcpy+0x12/0x20
	[    0.308503] Code: 8b 43 60 48 2b 43 50 88 43 4e 5b 5d c3 c3 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 4
	[    0.308503] RSP: 0018:ffffc900006b7ab0 EFLAGS: 00010202
	[    0.308503] RAX: ffff88817c1966fe RBX: ffff88807d1a67d8 RCX: 0000000000202024
	[    0.308503] RDX: 0000000000000002 RSI: ffff88807c996000 RDI: ffff88817c1966fe
	[    0.308503] RBP: ffffc900006b7ad8 R08: ffffffff81c81bf0 R09: 0000000000000000
	[    0.308503] R10: ffff88807c996000 R11: ffffffff81a33fb0 R12: 0000000000000007
	[    0.308503] R13: ffff88807c996000 R14: 0000000000000020 R15: ffffffff81a373c8
	[    0.308503] FS:  0000000000000000(0000) GS:ffff88807fd80000(0000) knlGS:0000000000000000
	[    0.308503] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
	[    0.308503] CR2: ffff88817c1966fe CR3: 0000000001c0f000 CR4: 00000000003406e0
	[    0.308503] Call Trace:
	[    0.308503]  ? xxh64_update+0x51/0x1e0
	[    0.308503]  xxhash64_finup+0x18/0x30
	[    0.308503]  xxhash64_digest+0x9/0x10
	[    0.308503]  crypto_shash_digest+0x24/0x40
	[    0.308503]  shash_ahash_digest+0x9a/0xf0
	[    0.308503]  ? shash_ahash_digest+0xf0/0xf0
	[    0.308503]  shash_async_digest+0x19/0x20
	[    0.308503]  crypto_ahash_op+0x24/0x60
	[    0.308503]  crypto_ahash_digest+0x16/0x20
	[    0.308503]  do_ahash_op.constprop.12+0x10/0x40
	[    0.308503]  test_hash_vec_cfg+0x205/0x610
	[    0.308503]  ? _raw_spin_unlock+0x11/0x30
	[    0.308503]  ? sprintf+0x56/0x70
	[    0.308503]  __alg_test_hash.isra.8+0x115/0x1d0
	[    0.308503]  alg_test_hash+0x7b/0x100
	[    0.308503]  alg_test+0xb6/0x375
	[    0.308503]  ? __kthread_parkme+0x5c/0x90
	[    0.308503]  ? lockdep_hardirqs_on+0xf6/0x190
	[    0.308503]  ? _raw_spin_unlock_irqrestore+0x44/0x50
	[    0.308503]  ? trace_hardirqs_on+0x22/0xf0
	[    0.308503]  ? __kthread_parkme+0x2a/0x90
	[    0.308503]  cryptomgr_test+0x26/0x40
	[    0.308503]  kthread+0x124/0x140
	[    0.308503]  ? cryptomgr_probe+0xd0/0xd0
	[    0.308503]  ? __kthread_create_on_node+0x1c0/0x1c0
	[    0.308503]  ret_from_fork+0x24/0x30
	[    0.308503] CR2: ffff88817c1966fe
	[    0.308503] ---[ end trace 3ee93ad10b0b79d0 ]---

> diff --git a/crypto/xxhash_generic.c b/crypto/xxhash_generic.c
> new file mode 100644
> index 000000000000..aedaabe55d45
> --- /dev/null
> +++ b/crypto/xxhash_generic.c
> @@ -0,0 +1,112 @@
> +#include <crypto/internal/hash.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/xxhash.h>
> +
> +#define XXHASH_BLOCK_SIZE	1

This should be set to 32 bytes (the "stripe size" of xxhash64), to match the
size of the chunks in which the algorithm processes data.  I.e., if you pass 31
bytes to xxhash64 it just buffers them, without doing any real work yet.

A "block size" of 1 is only appropriate for algorithms like CRC whose simplest
mathematical description operates directly on bytes.

> +#define XXHASH64_DIGEST_SIZE	8
> +
> +struct xxhash64_crypto_ctx {
> +	u64 seed;
> +};

To make it clearer what kind of "context" this is, please name this
"xxhash64_tfm_ctx", not "xxhash64_crypto_ctx".

Similarly, name the variables "tctx" instead of "cctx".

> +
> +struct xxhash64_desc_ctx {
> +	struct xxh64_state xxhstate;
> +	u64 seed;
> +};
> +
> +static int xxhash64_init(struct shash_desc *desc)
> +{
> +	struct xxhash64_crypto_ctx *cctx = crypto_shash_ctx(desc->tfm);
> +	struct xxhash64_desc_ctx *dctx = shash_desc_ctx(desc);
> +
> +	dctx->seed = cctx->seed;
> +	xxh64_reset(&dctx->xxhstate, dctx->seed);
> +
> +	return 0;
> +}

What's the point of storing 'seed' in the desc_ctx, given that it's already
represented in the initialized 'xxhstate'?

> +
> +static int xxhash64_setkey(struct crypto_shash *tfm, const u8 *key,
> +			 unsigned int keylen)
> +{
> +	struct xxhash64_crypto_ctx *ctx = crypto_shash_ctx(tfm);
> +
> +	if (keylen != sizeof(ctx->seed)) {
> +		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return -EINVAL;
> +	}
> +	ctx->seed = *(u64 *)key;
> +	return 0;
> +}

The crypto API takes the "key" as a byte array which might be unaligned.  Also,
the sequence of "key" bytes is conventionally interpreted the same way on all
architectures, e.g. it's not endian-dependent.  So, I suggest:

	ctx->seed = get_unaligned_le64(key);

Note that without this fix, your test vectors are wrong on big endian CPUs.

> +
> +static int xxhash64_update(struct shash_desc *desc, const u8 *data,
> +			 unsigned int length)
> +{
> +	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> +	xxh64_update(&ctx->xxhstate, data, length);
> +
> +	return 0;
> +}
> +
> +static int xxhash64_final(struct shash_desc *desc, u8 *out)
> +{
> +	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> +	*(u64 *)out = xxh64_digest(&ctx->xxhstate);
> +
> +	return 0;
> +}

Similarly, the 'out' array might be misaligned.  And conventionally the same
bytes are output on all architectures.  So I suggest:

	put_unaligned_u64(xxh64_digest(&ctx->xxhstate), out);

> +
> +static int xxhash64_finup(struct shash_desc *desc, const u8 *data,
> +			unsigned int len, u8 *out)
> +{
> +	xxhash64_update(desc, data, len);
> +	xxhash64_final(desc, out);
> +
> +	return 0;
> +}
> +
> +static int xxhash64_digest(struct shash_desc *desc, const u8 *data,
> +			 unsigned int length, u8 *out)
> +{
> +	return xxhash64_finup(desc, data, length, out);
> +}
> +

It seems that xxhash64_digest() is forgetting to initialize the hash state.

> +static struct shash_alg alg = {
> +	.digestsize		=	XXHASH64_DIGEST_SIZE,
> +	.setkey			= xxhash64_setkey,

Please use consistent indentation.

> +	.init		=	xxhash64_init,
> +	.update		=	xxhash64_update,
> +	.final		=	xxhash64_final,
> +	.finup		=	xxhash64_finup,
> +	.digest		=	xxhash64_digest,
> +	.descsize		=	sizeof(struct xxhash64_desc_ctx),
> +	.base			=	{
> +		.cra_name		=	"xxhash64",
> +		.cra_driver_name	=	"xxhash64-generic",
> +		.cra_priority		=	100,
> +		.cra_flags		=	CRYPTO_ALG_OPTIONAL_KEY,
> +		.cra_blocksize		=	XXHASH_BLOCK_SIZE,
> +		.cra_ctxsize		=	sizeof(struct xxhash64_crypto_ctx),
> +		.cra_module		=	THIS_MODULE,
> +	}
> +};
> +
> +static int __init xxhash_mod_init(void)
> +{
> +	return crypto_register_shash(&alg);
> +}
> +
> +static void __exit xxhash_mod_fini(void)
> +{
> +	crypto_unregister_shash(&alg);
> +}
> +
> +module_init(xxhash_mod_init);
> +module_exit(xxhash_mod_fini);
> +
> +MODULE_AUTHOR("Nikolay Borisov <nborisov@suse.com>");
> +MODULE_DESCRIPTION("xxhash  calculations wrapper for lib/xxhash.c");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_CRYPTO("xxhash-generic");

The purpose of MODULE_ALIAS_CRYPTO() is to allow the module to be dynamically
loaded when someone requests the algorithm by name.  So, putting a string here
which doesn't match the algorithm name is wrong.  It should be:

	MODULE_ALIAS_CRYPTO("xxhash64");
	MODULE_ALIAS_CRYPTO("xxhash64-generic");

- Eric

Re: [PATCH] crypto: xxhash - Implement xxhash support

[Eric Biggers]

On Mon, May 27, 2019 at 11:39:33AM -0700, Eric Biggers wrote:
> 
> Similarly, the 'out' array might be misaligned.  And conventionally the same
> bytes are output on all architectures.  So I suggest:
> 
> 	put_unaligned_u64(xxh64_digest(&ctx->xxhstate), out);
> 

I meant to write put_unaligned_le64.

- Eric

Re: [PATCH] crypto: xxhash - Implement xxhash support

[Eric Biggers]

On Mon, May 27, 2019 at 05:06:53PM +0200, Stephan Mueller wrote:
> >  obj-$(CONFIG_CRYPTO_JITTERENTROPY) += jitterentropy_rng.o
> >  CFLAGS_jitterentropy.o = -O0
> >  jitterentropy_rng-y := jitterentropy.o jitterentropy-kcapi.o
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index 8386038d67c7..322e906b6b6a 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> > @@ -3879,6 +3879,13 @@ static const struct alg_test_desc alg_test_descs[] =
> > { .alg = "xts512(paes)",
> >  		.test = alg_test_null,
> >  		.fips_allowed = 1,
> > +	}, {
> > +		.alg = "xxhash64",
> > +		.test = alg_test_hash,
> > +		.fips_allowed = 1,
> 
> Why is this intended to be allowed in FIPS mode? This does not seem to be a 
> FIPS approved cipher.
> 

The other non-cryptographic algorithms like crc32, crc32c, crct10dif, zstd,
zlib-deflate, lzo, lzohc have the fips_allowed flag set too, the argument being
the FIPS restrictions don't apply to non-cryptographic algorithms.  I'm not very
familiar with FIPS, but I'd assume the same would be true for xxhash.

- Eric

Re: [PATCH] crypto: xxhash - Implement xxhash support

[Nikolay Borisov]

On 27.05.19 г. 18:06 ч., Stephan Mueller wrote:
> Am Montag, 27. Mai 2019, 16:28:10 CEST schrieb Nikolay Borisov:
> 
> Hi Nikolay,
> 
>> xxhash is currently implemented as a self-contained module in /lib.
>> This patch enables that module to be used as part of the generic kernel
>> crypto framework. It adds a simple wrapper to the 64bit version. I've
>> also added a couple of simplistic test vectors to ensure I haven't
>> screwed up anything doing the plumbing.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  crypto/Kconfig          |   8 +++
>>  crypto/Makefile         |   1 +
>>  crypto/testmgr.c        |   7 +++
>>  crypto/testmgr.h        |  26 ++++++++++
>>  crypto/xxhash_generic.c | 112 ++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 154 insertions(+)
>>  create mode 100644 crypto/xxhash_generic.c
>>
>> diff --git a/crypto/Kconfig b/crypto/Kconfig
>> index bbab6bf33519..c56cc450ffc0 100644
>> --- a/crypto/Kconfig
>> +++ b/crypto/Kconfig
>> @@ -665,6 +665,14 @@ config CRYPTO_CRC32_MIPS
>>  	  instructions, when available.
>>
>>
>> +config CRYPTO_XXHASH
>> +	tristate "xxHash hash algorithm"
>> +	select CRYPTO_HASH
>> +	select XXHASH
>> +	help
>> +	  xxHash non-cryptographic hash algorithm. Extremely fast, working at
>> +	  speeds close to RAM limits.
>> +
>>  config CRYPTO_CRCT10DIF
>>  	tristate "CRCT10DIF algorithm"
>>  	select CRYPTO_HASH
>> diff --git a/crypto/Makefile b/crypto/Makefile
>> index fb5bf2a3a666..4dc6c45d026e 100644
>> --- a/crypto/Makefile
>> +++ b/crypto/Makefile
>> @@ -135,6 +135,7 @@ obj-$(CONFIG_CRYPTO_842) += 842.o
>>  obj-$(CONFIG_CRYPTO_RNG2) += rng.o
>>  obj-$(CONFIG_CRYPTO_ANSI_CPRNG) += ansi_cprng.o
>>  obj-$(CONFIG_CRYPTO_DRBG) += drbg.o
>> +obj-$(CONFIG_CRYPTO_XXHASH) += xxhash_generic.o
> 
> Cosmetic question: why is the hash object added inbetween the block of RNG 
> objects?

Geez, I ought to move it somewhere more appropriate, I put it there
while trying to get it to compile...

> 
>>  obj-$(CONFIG_CRYPTO_JITTERENTROPY) += jitterentropy_rng.o
>>  CFLAGS_jitterentropy.o = -O0
>>  jitterentropy_rng-y := jitterentropy.o jitterentropy-kcapi.o
>> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
>> index 8386038d67c7..322e906b6b6a 100644
>> --- a/crypto/testmgr.c
>> +++ b/crypto/testmgr.c
>> @@ -3879,6 +3879,13 @@ static const struct alg_test_desc alg_test_descs[] =
>> { .alg = "xts512(paes)",
>>  		.test = alg_test_null,
>>  		.fips_allowed = 1,
>> +	}, {
>> +		.alg = "xxhash64",
>> +		.test = alg_test_hash,
>> +		.fips_allowed = 1,
> 
> Why is this intended to be allowed in FIPS mode? This does not seem to be a 
> FIPS approved cipher.

As Eric mentioned - other non-crypto hashes have it, in particular I
copied that struct from crc32c.
> 
>> +		.suite = {
>> +			.hash = __VECS(xxhash64_tv_template)
>> +		}
>>  	}, {
>>  		.alg = "zlib-deflate",
>>  		.test = alg_test_comp,
>> diff --git a/crypto/testmgr.h b/crypto/testmgr.h
>> index d18a37629f05..8e0a9385ee5d 100644
>> --- a/crypto/testmgr.h
>> +++ b/crypto/testmgr.h
>> @@ -33218,6 +33218,32 @@ static const struct hash_testvec
>> crc32c_tv_template[] = { }
>>  };
>>
>> +static const struct hash_testvec xxhash64_tv_template[] = {
> 
> Could you please add a note where these vectors come from?

I just came up with them based on vectors for crc32c, I've also emailed
Nick Terrell who merged original xxhash for some vectors that makes sense.

> 
>> +	{
>> +		.psize = 0,
>> +		.digest = "\x99\xe9\xd8\x51\x37\xdb\x46\xef",
>> +	},
>> +	{
>> +		.plaintext = "abcdefg",
>> +		.psize = 7,
>> +		.digest = "\x2d\x82\x02\x29\x0e\x94\x60\x18",
>> +	},
>> +	{
>> +		.plaintext = "0123456789abcdef0123456789abcdef0123456789"
>> +			     "abcdef0123456789abcdef",
>> +		.psize = 64,
>> +		.digest = "\x85\x2f\xfe\x60\x47\xac\xf3\x1a",
>> +	},
>> +	{
>> +		.key = "\xd2\x02\x96\x49\x00\x00\x00\x00",
>> +		.ksize = 8,
>> +		.plaintext = "0123456789abcdef0123456789abcdef0123456789"
>> +			     "abcdef0123456789abcdef",
>> +		.psize = 64,
>> +		.digest = "\xab\xea\xc2\x48\x1a\x80\x4e\x7b",
>> +	},
>> +};
>> +
>>  static const struct comp_testvec lz4_comp_tv_template[] = {
>>  	{
>>  		.inlen	= 255,
>> diff --git a/crypto/xxhash_generic.c b/crypto/xxhash_generic.c
>> new file mode 100644
>> index 000000000000..aedaabe55d45
>> --- /dev/null
>> +++ b/crypto/xxhash_generic.c
>> @@ -0,0 +1,112 @@
> 
> What about adding the license information compliant with SPDX?

Will add .
> 
>> +#include <crypto/internal/hash.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/xxhash.h>
>> +
>> +#define XXHASH_BLOCK_SIZE	1
>> +#define XXHASH64_DIGEST_SIZE	8
>> +
>> +struct xxhash64_crypto_ctx {
>> +	u64 seed;
>> +};
>> +
>> +struct xxhash64_desc_ctx {
>> +	struct xxh64_state xxhstate;
>> +	u64 seed;
>> +};
>> +
>> +static int xxhash64_init(struct shash_desc *desc)
>> +{
>> +	struct xxhash64_crypto_ctx *cctx = crypto_shash_ctx(desc->tfm);
>> +	struct xxhash64_desc_ctx *dctx = shash_desc_ctx(desc);
>> +
>> +	dctx->seed = cctx->seed;
>> +	xxh64_reset(&dctx->xxhstate, dctx->seed);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xxhash64_setkey(struct crypto_shash *tfm, const u8 *key,
>> +			 unsigned int keylen)
>> +{
>> +	struct xxhash64_crypto_ctx *ctx = crypto_shash_ctx(tfm);
>> +
>> +	if (keylen != sizeof(ctx->seed)) {
>> +		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
>> +		return -EINVAL;
>> +	}
>> +	ctx->seed = *(u64 *)key;
>> +	return 0;
>> +}
>> +
>> +static int xxhash64_update(struct shash_desc *desc, const u8 *data,
>> +			 unsigned int length)
>> +{
>> +	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
>> +
>> +	xxh64_update(&ctx->xxhstate, data, length);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xxhash64_final(struct shash_desc *desc, u8 *out)
>> +{
>> +	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
>> +
>> +	*(u64 *)out = xxh64_digest(&ctx->xxhstate);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xxhash64_finup(struct shash_desc *desc, const u8 *data,
>> +			unsigned int len, u8 *out)
>> +{
>> +	xxhash64_update(desc, data, len);
>> +	xxhash64_final(desc, out);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xxhash64_digest(struct shash_desc *desc, const u8 *data,
>> +			 unsigned int length, u8 *out)
>> +{
>> +	return xxhash64_finup(desc, data, length, out);
>> +}
>> +
>> +static struct shash_alg alg = {
>> +	.digestsize		=	XXHASH64_DIGEST_SIZE,
>> +	.setkey			= xxhash64_setkey,
>> +	.init		=	xxhash64_init,
>> +	.update		=	xxhash64_update,
>> +	.final		=	xxhash64_final,
>> +	.finup		=	xxhash64_finup,
>> +	.digest		=	xxhash64_digest,
>> +	.descsize		=	sizeof(struct xxhash64_desc_ctx),
>> +	.base			=	{
>> +		.cra_name		=	"xxhash64",
>> +		.cra_driver_name	=	"xxhash64-generic",
>> +		.cra_priority		=	100,
>> +		.cra_flags		=	CRYPTO_ALG_OPTIONAL_KEY,
>> +		.cra_blocksize		=	XXHASH_BLOCK_SIZE,
>> +		.cra_ctxsize		=	sizeof(struct 
> xxhash64_crypto_ctx),
>> +		.cra_module		=	THIS_MODULE,
>> +	}
>> +};
>> +
>> +static int __init xxhash_mod_init(void)
>> +{
>> +	return crypto_register_shash(&alg);
>> +}
>> +
>> +static void __exit xxhash_mod_fini(void)
>> +{
>> +	crypto_unregister_shash(&alg);
>> +}
>> +
>> +module_init(xxhash_mod_init);
>> +module_exit(xxhash_mod_fini);
>> +
>> +MODULE_AUTHOR("Nikolay Borisov <nborisov@suse.com>");
>> +MODULE_DESCRIPTION("xxhash  calculations wrapper for lib/xxhash.c");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS_CRYPTO("xxhash-generic");
> 
> 
> 
> Ciao
> Stephan
> 
> 
> 

Re: [PATCH] crypto: xxhash - Implement xxhash support

[Nikolay Borisov]

On 27.05.19 г. 21:39 ч., Eric Biggers wrote:
> Hi Nikolay,
> 
> On Mon, May 27, 2019 at 05:28:10PM +0300, Nikolay Borisov wrote:
>> xxhash is currently implemented as a self-contained module in /lib.
>> This patch enables that module to be used as part of the generic kernel
>> crypto framework. It adds a simple wrapper to the 64bit version. I've
>> also added a couple of simplistic test vectors to ensure I haven't
>> screwed up anything doing the plumbing.
> 
> What is this planned to be used for?

Possibly as a replacement hash of crc32c in btrfs.

> 
> Please also run the crypto self-tests (i.e. boot a kernel with
> CONFIG_CRYPTO_MANAGER_DISABLE_TESTS unset) for all crypto API patches.
> When that's done this code immediately crashes, see output below.

Not for me, I tested the vectors I've added for correct results. I
wouldn't have sent the patch otherwise. Strange why I didn't observe
this crash, I wonder if it's due to an unaligned buffer or digest not
initialising the state.

> 
> More comments below.
> 
> 	[    0.305235] BUG: unable to handle page fault for address: ffff88817c1966fe
> 	[    0.306613] #PF: supervisor write access in kernel mode
> 	[    0.307653] #PF: error_code(0x0002) - not-present page
> 	[    0.308503] PGD 2a01067 P4D 2a01067 PUD 0 
> 	[    0.308503] Oops: 0002 [#1] SMP
> 	[    0.308503] CPU: 3 PID: 59 Comm: cryptomgr_test Not tainted 5.2.0-rc2-00001-g4fcf4df09e23d #3
> 	[    0.308503] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014
> 	[    0.308503] RIP: 0010:__memcpy+0x12/0x20
> 	[    0.308503] Code: 8b 43 60 48 2b 43 50 88 43 4e 5b 5d c3 c3 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 4
> 	[    0.308503] RSP: 0018:ffffc900006b7ab0 EFLAGS: 00010202
> 	[    0.308503] RAX: ffff88817c1966fe RBX: ffff88807d1a67d8 RCX: 0000000000202024
> 	[    0.308503] RDX: 0000000000000002 RSI: ffff88807c996000 RDI: ffff88817c1966fe
> 	[    0.308503] RBP: ffffc900006b7ad8 R08: ffffffff81c81bf0 R09: 0000000000000000
> 	[    0.308503] R10: ffff88807c996000 R11: ffffffff81a33fb0 R12: 0000000000000007
> 	[    0.308503] R13: ffff88807c996000 R14: 0000000000000020 R15: ffffffff81a373c8
> 	[    0.308503] FS:  0000000000000000(0000) GS:ffff88807fd80000(0000) knlGS:0000000000000000
> 	[    0.308503] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 	[    0.308503] CR2: ffff88817c1966fe CR3: 0000000001c0f000 CR4: 00000000003406e0
> 	[    0.308503] Call Trace:
> 	[    0.308503]  ? xxh64_update+0x51/0x1e0
> 	[    0.308503]  xxhash64_finup+0x18/0x30
> 	[    0.308503]  xxhash64_digest+0x9/0x10
> 	[    0.308503]  crypto_shash_digest+0x24/0x40
> 	[    0.308503]  shash_ahash_digest+0x9a/0xf0
> 	[    0.308503]  ? shash_ahash_digest+0xf0/0xf0
> 	[    0.308503]  shash_async_digest+0x19/0x20
> 	[    0.308503]  crypto_ahash_op+0x24/0x60
> 	[    0.308503]  crypto_ahash_digest+0x16/0x20
> 	[    0.308503]  do_ahash_op.constprop.12+0x10/0x40
> 	[    0.308503]  test_hash_vec_cfg+0x205/0x610
> 	[    0.308503]  ? _raw_spin_unlock+0x11/0x30
> 	[    0.308503]  ? sprintf+0x56/0x70
> 	[    0.308503]  __alg_test_hash.isra.8+0x115/0x1d0
> 	[    0.308503]  alg_test_hash+0x7b/0x100
> 	[    0.308503]  alg_test+0xb6/0x375
> 	[    0.308503]  ? __kthread_parkme+0x5c/0x90
> 	[    0.308503]  ? lockdep_hardirqs_on+0xf6/0x190
> 	[    0.308503]  ? _raw_spin_unlock_irqrestore+0x44/0x50
> 	[    0.308503]  ? trace_hardirqs_on+0x22/0xf0
> 	[    0.308503]  ? __kthread_parkme+0x2a/0x90
> 	[    0.308503]  cryptomgr_test+0x26/0x40
> 	[    0.308503]  kthread+0x124/0x140
> 	[    0.308503]  ? cryptomgr_probe+0xd0/0xd0
> 	[    0.308503]  ? __kthread_create_on_node+0x1c0/0x1c0
> 	[    0.308503]  ret_from_fork+0x24/0x30
> 	[    0.308503] CR2: ffff88817c1966fe
> 	[    0.308503] ---[ end trace 3ee93ad10b0b79d0 ]---
> 
>> diff --git a/crypto/xxhash_generic.c b/crypto/xxhash_generic.c
>> new file mode 100644
>> index 000000000000..aedaabe55d45
>> --- /dev/null
>> +++ b/crypto/xxhash_generic.c
>> @@ -0,0 +1,112 @@
>> +#include <crypto/internal/hash.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/xxhash.h>
>> +
>> +#define XXHASH_BLOCK_SIZE	1
> 
> This should be set to 32 bytes (the "stripe size" of xxhash64), to match the
> size of the chunks in which the algorithm processes data.  I.e., if you pass 31
> bytes to xxhash64 it just buffers them, without doing any real work yet.
> 
> A "block size" of 1 is only appropriate for algorithms like CRC whose simplest
> mathematical description operates directly on bytes.
> 
>> +#define XXHASH64_DIGEST_SIZE	8
>> +
>> +struct xxhash64_crypto_ctx {
>> +	u64 seed;
>> +};
> 
> To make it clearer what kind of "context" this is, please name this
> "xxhash64_tfm_ctx", not "xxhash64_crypto_ctx".
> 
> Similarly, name the variables "tctx" instead of "cctx".

ACK

> 
>> +
>> +struct xxhash64_desc_ctx {
>> +	struct xxh64_state xxhstate;
>> +	u64 seed;
>> +};
>> +
>> +static int xxhash64_init(struct shash_desc *desc)
>> +{
>> +	struct xxhash64_crypto_ctx *cctx = crypto_shash_ctx(desc->tfm);
>> +	struct xxhash64_desc_ctx *dctx = shash_desc_ctx(desc);
>> +
>> +	dctx->seed = cctx->seed;
>> +	xxh64_reset(&dctx->xxhstate, dctx->seed);
>> +
>> +	return 0;
>> +}
> 
> What's the point of storing 'seed' in the desc_ctx, given that it's already
> represented in the initialized 'xxhstate'?

Good point will remove seed in next iteration.

> 
>> +
>> +static int xxhash64_setkey(struct crypto_shash *tfm, const u8 *key,
>> +			 unsigned int keylen)
>> +{
>> +	struct xxhash64_crypto_ctx *ctx = crypto_shash_ctx(tfm);
>> +
>> +	if (keylen != sizeof(ctx->seed)) {
>> +		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
>> +		return -EINVAL;
>> +	}
>> +	ctx->seed = *(u64 *)key;
>> +	return 0;
>> +}
> 
> The crypto API takes the "key" as a byte array which might be unaligned.  Also,
> the sequence of "key" bytes is conventionally interpreted the same way on all
> architectures, e.g. it's not endian-dependent.  So, I suggest:
> 
> 	ctx->seed = get_unaligned_le64(key);

Can it actually be unaligned though, looking at
crypto_shash_setkey->shash_setkey_unaligned it seems that the generic
layer aligns the buffer and passes the aligned one to ->setkey.
> 
> Note that without this fix, your test vectors are wrong on big endian CPUs.
> 
>> +
>> +static int xxhash64_update(struct shash_desc *desc, const u8 *data,
>> +			 unsigned int length)
>> +{
>> +	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
>> +
>> +	xxh64_update(&ctx->xxhstate, data, length);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xxhash64_final(struct shash_desc *desc, u8 *out)
>> +{
>> +	struct xxhash64_desc_ctx *ctx = shash_desc_ctx(desc);
>> +
>> +	*(u64 *)out = xxh64_digest(&ctx->xxhstate);
>> +
>> +	return 0;
>> +}
> 
> Similarly, the 'out' array might be misaligned.  And conventionally the same
> bytes are output on all architectures.  So I suggest:
> 
> 	put_unaligned_u64(xxh64_digest(&ctx->xxhstate), out);

Fair enough, will fix it in next version.

> 
>> +
>> +static int xxhash64_finup(struct shash_desc *desc, const u8 *data,
>> +			unsigned int len, u8 *out)
>> +{
>> +	xxhash64_update(desc, data, len);
>> +	xxhash64_final(desc, out);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xxhash64_digest(struct shash_desc *desc, const u8 *data,
>> +			 unsigned int length, u8 *out)
>> +{
>> +	return xxhash64_finup(desc, data, length, out);
>> +}
>> +
> 
> It seems that xxhash64_digest() is forgetting to initialize the hash state.
> 
>> +static struct shash_alg alg = {
>> +	.digestsize		=	XXHASH64_DIGEST_SIZE,
>> +	.setkey			= xxhash64_setkey,
> 
> Please use consistent indentation.

This was copy/pasted from crc32c struct but yeah, I admit it looks ugly,
will fix.

> 
>> +	.init		=	xxhash64_init,
>> +	.update		=	xxhash64_update,
>> +	.final		=	xxhash64_final,
>> +	.finup		=	xxhash64_finup,
>> +	.digest		=	xxhash64_digest,
>> +	.descsize		=	sizeof(struct xxhash64_desc_ctx),
>> +	.base			=	{
>> +		.cra_name		=	"xxhash64",
>> +		.cra_driver_name	=	"xxhash64-generic",
>> +		.cra_priority		=	100,
>> +		.cra_flags		=	CRYPTO_ALG_OPTIONAL_KEY,
>> +		.cra_blocksize		=	XXHASH_BLOCK_SIZE,
>> +		.cra_ctxsize		=	sizeof(struct xxhash64_crypto_ctx),
>> +		.cra_module		=	THIS_MODULE,
>> +	}
>> +};
>> +
>> +static int __init xxhash_mod_init(void)
>> +{
>> +	return crypto_register_shash(&alg);
>> +}
>> +
>> +static void __exit xxhash_mod_fini(void)
>> +{
>> +	crypto_unregister_shash(&alg);
>> +}
>> +
>> +module_init(xxhash_mod_init);
>> +module_exit(xxhash_mod_fini);
>> +
>> +MODULE_AUTHOR("Nikolay Borisov <nborisov@suse.com>");
>> +MODULE_DESCRIPTION("xxhash  calculations wrapper for lib/xxhash.c");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS_CRYPTO("xxhash-generic");
> 
> The purpose of MODULE_ALIAS_CRYPTO() is to allow the module to be dynamically
> loaded when someone requests the algorithm by name.  So, putting a string here
> which doesn't match the algorithm name is wrong.  It should be:
> 
> 	MODULE_ALIAS_CRYPTO("xxhash64");
> 	MODULE_ALIAS_CRYPTO("xxhash64-generic");
> 
> - Eric
> 

Re: [PATCH] crypto: xxhash - Implement xxhash support

[Stephan Mueller]

Am Montag, 27. Mai 2019, 21:15:51 CEST schrieb Eric Biggers:

Hi Eric,

> On Mon, May 27, 2019 at 05:06:53PM +0200, Stephan Mueller wrote:
> > >  obj-$(CONFIG_CRYPTO_JITTERENTROPY) += jitterentropy_rng.o
> > >  CFLAGS_jitterentropy.o = -O0
> > >  jitterentropy_rng-y := jitterentropy.o jitterentropy-kcapi.o
> > > 
> > > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > > index 8386038d67c7..322e906b6b6a 100644
> > > --- a/crypto/testmgr.c
> > > +++ b/crypto/testmgr.c
> > > @@ -3879,6 +3879,13 @@ static const struct alg_test_desc
> > > alg_test_descs[] =
> > > { .alg = "xts512(paes)",
> > > 
> > >  		.test = alg_test_null,
> > >  		.fips_allowed = 1,
> > > 
> > > +	}, {
> > > +		.alg = "xxhash64",
> > > +		.test = alg_test_hash,
> > > +		.fips_allowed = 1,
> > 
> > Why is this intended to be allowed in FIPS mode? This does not seem to be
> > a
> > FIPS approved cipher.
> 
> The other non-cryptographic algorithms like crc32, crc32c, crct10dif, zstd,
> zlib-deflate, lzo, lzohc have the fips_allowed flag set too, the argument
> being the FIPS restrictions don't apply to non-cryptographic algorithms. 
> I'm not very familiar with FIPS, but I'd assume the same would be true for
> xxhash.

FIPS relates to cryptographic mechanisms protecting user data. The mechanisms 
you refer to are non-cryptographic and thus do not fall under 
FIPS-"jurisdiction".

Please correct me if I am wrong, but as far as I see, however, xxhash seems to 
be a cryptographic hash function.

Ciao
Stephan

Re: [PATCH] crypto: xxhash - Implement xxhash support

[Nikolay Borisov]

On 28.05.19 г. 7:58 ч., Stephan Mueller wrote:
> Please correct me if I am wrong, but as far as I see, however, xxhash seems to 
> be a cryptographic hash function.

No, xxhash is non-cryptographic. THe official description from
xxhash.com states:

xxHash is an extremely fast non-cryptographic hash algorithm, working at
speeds close to RAM limits. It is proposed in two flavors, 32 and 64 bits.

Re: [PATCH] crypto: xxhash - Implement xxhash support

[Stephan Mueller]

Am Dienstag, 28. Mai 2019, 07:58:19 CEST schrieb Nikolay Borisov:

Hi Nikolay,

> On 28.05.19 г. 7:58 ч., Stephan Mueller wrote:
> > Please correct me if I am wrong, but as far as I see, however, xxhash
> > seems to be a cryptographic hash function.
> 
> No, xxhash is non-cryptographic. THe official description from
> xxhash.com states:
> 
> xxHash is an extremely fast non-cryptographic hash algorithm, working at
> speeds close to RAM limits. It is proposed in two flavors, 32 and 64 bits.

I see, then please disregard my comment on the FIPS flag then.

Ciao
Stephan