Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 8 Sep 2020 21:35:54 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Elena Petrova <lenaptr@google.com>
Cc:     linux-crypto@vger.kernel.org,
        Stephan =?iso-8859-1?Q?M=FCller?= <smueller@chronox.de>,
        Ard Biesheuvel <ardb@kernel.org>,
        Jeffrey Vander Stoep <jeffv@google.com>
Subject: Re: [PATCH v6] crypto: af_alg - add extra parameters for DRBG
 interface
Message-ID: <20200909043554.GA8311@sol.localdomain>
References: <20200821042443.GA25695@gondor.apana.org.au>
 <20200908170403.2625295-1-lenaptr@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200908170403.2625295-1-lenaptr@google.com>
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Tue, Sep 08, 2020 at 06:04:03PM +0100, Elena Petrova wrote:
> Extend the user-space RNG interface:
>   1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
>   2. Add additional data input via sendmsg syscall.
> 
> This allows DRBG to be tested with test vectors, for example for the
> purpose of CAVP testing, which otherwise isn't possible.
> 
> To prevent erroneous use of entropy input, it is hidden under
> CRYPTO_USER_API_RNG_CAVP config option and requires CAP_SYS_ADMIN to
> succeed.
> 
> Signed-off-by: Elena Petrova <lenaptr@google.com>
> Acked-by: Stephan M?ller <smueller@chronox.de>

This doesn't compile for me.  Can you rebase this onto the latest
"master" branch from
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git?

> +static void rng_reset_addtl(struct rng_ctx *ctx)
>  {
> -	struct sock *sk = sock->sk;
> -	struct alg_sock *ask = alg_sk(sk);
> -	struct rng_ctx *ctx = ask->private;
> -	int err;
> +	kzfree(ctx->addtl);
> +	ctx->addtl = NULL;
> +	ctx->addtl_len = 0;
> +}

kzfree() has been renamed to kfree_sensitive(); see commit 453431a54934
("mm, treewide: rename kzfree() to kfree_sensitive()").
So please use kfree_sensitive() rather than kzfree(), in all three places.

Note, kzfree() won't actually cause a compilation error since it's still
#define'd to kfree_sensitive().  But that #define probably will go away soon.

> +static int rng_test_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> +{
> +	int err;
> +	struct alg_sock *ask = alg_sk(sock->sk);
> +	struct rng_ctx *ctx = ask->private;
> +
> +	lock_sock(sock->sk);
> +	if (len > MAXSIZE)
> +		len = MAXSIZE;

Since this function only supports providing the additional data all at once, not
incrementally, shouldn't it return an error code if the length is too long,
rather than truncate the length?

> +	/*
> +	 * Non NULL pctx->entropy means that CAVP test has been initiated on
> +	 * this socket, replace proto_ops algif_rng_ops with algif_rng_test_ops.
> +	 */
> +	if (pctx->entropy)
> +		sk->sk_socket->ops = algif_rng_test_ops;

This means that providing additional data on a "request socket" via sendmsg will
only work if ALG_SET_DRBG_ENTROPY was used on the "algorithm socket" earlier.
If that's intentional, it needs to be mentioned in the documentation.

>  static const struct af_alg_type algif_type_rng = {
> @@ -169,6 +319,9 @@ static const struct af_alg_type algif_type_rng = {
>  	.release	=	rng_release,
>  	.accept		=	rng_accept_parent,
>  	.setkey		=	rng_setkey,
> +#if IS_ENABLED(CONFIG_CRYPTO_USER_API_RNG_CAVP)
> +	.setentropy	=	rng_setentropy,
> +#endif

Since CRYPTO_USER_API_RNG_CAVP is now a bool rather than a tristate, this should
use '#ifdef CONFIG_CRYPTO_USER_API_RNG_CAVP' instead of
'IS_ENABLED(CONFIG_CRYPTO_USER_API_RNG_CAVP)'.

- Eric