LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, Samuel Neves <sneves@dei.uc.pt>,
	Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>,
	Andy Lutomirski <luto@kernel.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	kernel-hardening@lists.openwall.com
Subject: Re: [PATCH net-next v9 12/19] zinc: BLAKE2s generic C implementation and selftest
Date: Tue, 26 Mar 2019 18:38:00 +0100
Message-ID: <20190326173759.GA607@zzz.localdomain> (raw)
In-Reply-To: <20190322071122.6677-13-Jason@zx2c4.com>

Hi Jason, a few comments on this patch:

On Fri, Mar 22, 2019 at 01:11:15AM -0600, Jason A. Donenfeld wrote:
> The C implementation was originally based on Samuel Neves' public
> domain reference implementation but has since been heavily modified
> for the kernel. We're able to do compile-time optimizations by moving
> some scaffolding around the final function into the header file.
> 
> Information: https://blake2.net/
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Samuel Neves <sneves@dei.uc.pt>
> Co-developed-by: Samuel Neves <sneves@dei.uc.pt>
> Cc: Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: kernel-hardening@lists.openwall.com
> Cc: linux-crypto@vger.kernel.org
> ---
>  include/zinc/blake2s.h      |   56 +
>  lib/zinc/Kconfig            |    3 +
>  lib/zinc/Makefile           |    3 +
>  lib/zinc/blake2s/blake2s.c  |  295 +++++
>  lib/zinc/selftest/blake2s.c | 2090 +++++++++++++++++++++++++++++++++++
>  5 files changed, 2447 insertions(+)
>  create mode 100644 include/zinc/blake2s.h
>  create mode 100644 lib/zinc/blake2s/blake2s.c
>  create mode 100644 lib/zinc/selftest/blake2s.c
> 
> diff --git a/include/zinc/blake2s.h b/include/zinc/blake2s.h
> new file mode 100644
> index 000000000000..5f1a1aeaf129
> --- /dev/null
> +++ b/include/zinc/blake2s.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright (C) 2015-2019 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + */
> +
> +#ifndef _ZINC_BLAKE2S_H
> +#define _ZINC_BLAKE2S_H
> +
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <asm/bug.h>
> +
> +enum blake2s_lengths {
> +	BLAKE2S_BLOCK_SIZE = 64,
> +	BLAKE2S_HASH_SIZE = 32,
> +	BLAKE2S_KEY_SIZE = 32

Perhaps call the latter two BLAKE2S_MAX_HASH_SIZE and BLAKE2S_MAX_KEY_SIZE,
since lower values can be selected too?

> +};
> +
> +struct blake2s_state {
> +	u32 h[8];
> +	u32 t[2];
> +	u32 f[2];
> +	u8 buf[BLAKE2S_BLOCK_SIZE];
> +	size_t buflen;
> +	u8 last_node;
> +};

Since this API doesn't actually support any of BLAKE2's tree hashing modes, I
think 'last_node' should be removed from this struct for now.  It's never set.

> +
> +void blake2s_init(struct blake2s_state *state, const size_t outlen);
> +void blake2s_init_key(struct blake2s_state *state, const size_t outlen,
> +		      const void *key, const size_t keylen);
> +void blake2s_update(struct blake2s_state *state, const u8 *in, size_t inlen);
> +void blake2s_final(struct blake2s_state *state, u8 *out, const size_t outlen);

Since the caller must pass the same 'outlen' into blake2s_init() and
blake2s_final(), perhaps that should be verified by the DEBUG checks?  It would
be easy for someone to get this wrong.  Or remove the outlen argument from
blake2s_final(), instead using the saved length from blake2s_init()?

> diff --git a/lib/zinc/blake2s/blake2s.c b/lib/zinc/blake2s/blake2s.c
> new file mode 100644
> index 000000000000..c817954e6bcd
> --- /dev/null
> +++ b/lib/zinc/blake2s/blake2s.c
> @@ -0,0 +1,295 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2012 Samuel Neves <sneves@dei.uc.pt>. All Rights Reserved.

If Samuel's reference implementation is "public domain" as you stated in the
commit message, why is this copyright statement from 2012 here?
Public domain == not copyrighted.

> + * Copyright (C) 2015-2019 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + *
> + * This is an implementation of the BLAKE2s hash and PRF functions.
> + *
> + * Information: https://blake2.net/
> + *
> + */
> +
> +#include <zinc/blake2s.h>
> +#include "../selftest/run.h"
> +
> +#include <linux/types.h>
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/bug.h>
> +#include <asm/unaligned.h>
> +
> +typedef union {
> +	struct {
> +		u8 digest_length;
> +		u8 key_length;
> +		u8 fanout;
> +		u8 depth;
> +		u32 leaf_length;
> +		u32 node_offset;
> +		u16 xof_length;

Technically the leaf_length, node_offset, and xof_length fields should be
__le32, __le32, and __le16 respectively.

> +void blake2s_update(struct blake2s_state *state, const u8 *in, size_t inlen)
> +{
> +	const size_t fill = BLAKE2S_BLOCK_SIZE - state->buflen;
> +
> +	if (unlikely(!inlen))
> +		return;
> +	if (inlen > fill) {
> +		memcpy(state->buf + state->buflen, in, fill);
> +		blake2s_compress(state, state->buf, 1, BLAKE2S_BLOCK_SIZE);
> +		state->buflen = 0;
> +		in += fill;
> +		inlen -= fill;
> +	}
> +	if (inlen > BLAKE2S_BLOCK_SIZE) {
> +		const size_t nblocks =
> +			(inlen + BLAKE2S_BLOCK_SIZE - 1) / BLAKE2S_BLOCK_SIZE;

This can be written as:

		const size_t nblocks = DIV_ROUND_UP(inlen, BLAKE2S_BLOCK_SIZE);

> +
> +static int __init mod_init(void)
> +{
> +	if (!nosimd)
> +		blake2s_fpu_init();
> +	if (!selftest_run("blake2s", blake2s_selftest, blake2s_nobs,
> +			  ARRAY_SIZE(blake2s_nobs)))
> +		return -ENOTRECOVERABLE;
> +	return 0;
> +}
> +
> +static void __exit mod_exit(void)
> +{
> +}
> +
> +module_param(nosimd, bool, 0);
> +module_init(mod_init);
> +module_exit(mod_exit);
> +MODULE_LICENSE("GPL v2");

The MODULE_LICENSE() doesn't match the SPDX license tag.  Intentional?

> +MODULE_DESCRIPTION("BLAKE2s hash function");
> +MODULE_AUTHOR("Jason A. Donenfeld <Jason@zx2c4.com>");
> diff --git a/lib/zinc/selftest/blake2s.c b/lib/zinc/selftest/blake2s.c
> new file mode 100644
> index 000000000000..1b5c210dc7a8
> --- /dev/null
> +++ b/lib/zinc/selftest/blake2s.c
> @@ -0,0 +1,2090 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2015-2019 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + */
> +
> +static const u8 blake2s_testvecs[][BLAKE2S_HASH_SIZE] __initconst = {
[snip]
> +};
> +
> +static const u8 blake2s_keyed_testvecs[][BLAKE2S_HASH_SIZE] __initconst = {
[snip]
> +};

There should be a comment mentioning where these test vectors came from.  Are
they from a published source?  Or did you use a different implementation of
BLAKE2s to generate them?

> +
> +static bool __init blake2s_selftest(void)
> +{
> +	u8 key[BLAKE2S_KEY_SIZE];
> +	u8 buf[ARRAY_SIZE(blake2s_testvecs)];
> +	u8 hash[BLAKE2S_HASH_SIZE];
> +	size_t i;
> +	bool success = true;
> +
> +	for (i = 0; i < BLAKE2S_KEY_SIZE; ++i)
> +		key[i] = (u8)i;
> +
> +	for (i = 0; i < ARRAY_SIZE(blake2s_testvecs); ++i)
> +		buf[i] = (u8)i;
> +
> +	for (i = 0; i < ARRAY_SIZE(blake2s_keyed_testvecs); ++i) {
> +		blake2s(hash, buf, key, BLAKE2S_HASH_SIZE, i, BLAKE2S_KEY_SIZE);
> +		if (memcmp(hash, blake2s_keyed_testvecs[i], BLAKE2S_HASH_SIZE)) {
> +			pr_err("blake2s keyed self-test %zu: FAIL\n", i + 1);
> +			success = false;
> +		}
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(blake2s_testvecs); ++i) {
> +		blake2s(hash, buf, NULL, BLAKE2S_HASH_SIZE, i, 0);
> +		if (memcmp(hash, blake2s_testvecs[i], BLAKE2S_HASH_SIZE)) {
> +			pr_err("blake2s unkeyed self-test %zu: FAIL\n", i + i);
> +			success = false;
> +		}
> +	}
> +	return success;
> +}

This tests here miss a lot of cases.  I can break the implementation in some
pretty major ways and it still passes the tests.

We'll get better test coverage of this via testmgr once this is exposed via the
"regular" crypto API.  E.g. the latest testmgr will automatically test multiple
update() calls, and buffers with various alignments.  If we also make your new
'simd_get()' function call crypto_simd_usable() rather than may_use_simd()
directly, then testmgr will also cover the case where some update() are done in
a SIMD-usable context and some aren't.

However, any functionality that won't be exposed by the regular crypto API still
needs to be tested here, e.g. the following which aren't currently tested:

	- blake2s_hmac()
	- Key and digest lengths other than the default

- Eric

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-22  7:11 [PATCH net-next v9 00/19] WireGuard: Secure Network Tunnel Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 01/19] asm: simd context helper API Jason A. Donenfeld
2019-03-22 11:21   ` Thomas Gleixner
2019-03-22  7:11 ` [PATCH net-next v9 02/19] zinc: introduce minimal cryptography library Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 03/19] zinc: ChaCha20 generic C implementation and selftest Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 04/19] zinc: ChaCha20 x86_64 implementation Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 05/19] zinc: ChaCha20 ARM and ARM64 implementations Jason A. Donenfeld
2019-03-22 23:17   ` Stefan Agner
2019-03-22  7:11 ` [PATCH net-next v9 06/19] zinc: ChaCha20 MIPS32r2 implementation Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 07/19] zinc: Poly1305 generic C implementations and selftest Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 08/19] zinc: Poly1305 x86_64 implementation Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 09/19] zinc: Poly1305 ARM and ARM64 implementations Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 10/19] zinc: Poly1305 MIPS64 and MIPS32r2 implementations Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 11/19] zinc: ChaCha20Poly1305 construction and selftest Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 12/19] zinc: BLAKE2s generic C implementation " Jason A. Donenfeld
2019-03-26 17:38   ` Eric Biggers [this message]
2019-03-22  7:11 ` [PATCH net-next v9 13/19] zinc: BLAKE2s x86_64 implementation Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 14/19] zinc: Curve25519 generic C implementations and selftest Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 15/19] zinc: Curve25519 x86_64 implementation Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 16/19] zinc: import Bernstein and Schwabe's Curve25519 ARM implementation Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 17/19] zinc: " Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 18/19] security/keys: rewrite big_key crypto to use Zinc Jason A. Donenfeld
2019-03-22  7:11 ` [PATCH net-next v9 19/19] net: WireGuard secure network tunnel Jason A. Donenfeld
2019-03-25  0:02   ` David Miller
2019-03-25 10:13     ` Jason A. Donenfeld
2019-03-25 11:51 ` [PATCH net-next v9 00/19] WireGuard: Secure Network Tunnel Herbert Xu
2019-03-25 11:57   ` Jason A. Donenfeld
2019-03-25 12:03     ` Herbert Xu
2019-03-30  5:53     ` Eric Biggers
2019-03-31 18:18       ` Jason A. Donenfeld
2019-03-31 18:42         ` Eric Biggers
2019-03-25 16:04   ` David Miller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190326173759.GA607@zzz.localdomain \
    --to=ebiggers@kernel.org \
    --cc=Jason@zx2c4.com \
    --cc=akpm@linux-foundation.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jeanphilippe.aumasson@gmail.com \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=sneves@dei.uc.pt \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git