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:   Thu, 13 Aug 2020 12:32:39 -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 v5] crypto: af_alg - add extra parameters for DRBG
 interface
Message-ID: <20200813193239.GA2470@sol.localdomain>
References: <CABvBcwYPXvK1_b2hR5THaqfq8nKVwppdBd2aJF6cmS_aCxHxUg@mail.gmail.com>
 <20200813160811.3568494-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: <20200813160811.3568494-1-lenaptr@google.com>
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Thu, Aug 13, 2020 at 05:08:11PM +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_CAVP_DRBG 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>
> ---
> 
> Updates in v5:
>   1) use __maybe_unused instead of #ifdef;
>   2) separate code path for a testing mode;
>   3) only allow Additional Data input in a testing mode.
> 
> Updates in v4:
>   1) setentropy returns 0 or error code (used to return length);
>   2) bigfixes suggested by Eric.
> 
> Updates in v3:
>   1) More details in commit message;
>   2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
>   3) fixed a bug of not releasing socket locks.
> 
> Updates in v2:
>   1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
>   2) Requiring CAP_SYS_ADMIN for entropy reset.
>   3) Locking for send and recv.
>   4) Length checks added for send and setentropy; send and setentropy now return
>      number of bytes accepted.
>   5) Minor code style corrections.
> 
> libkcapi patch for testing:
>   https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531
> 
>  Documentation/crypto/userspace-if.rst |  17 ++-
>  crypto/Kconfig                        |   9 ++
>  crypto/af_alg.c                       |   8 ++
>  crypto/algif_rng.c                    | 172 ++++++++++++++++++++++++--
>  include/crypto/if_alg.h               |   1 +
>  include/uapi/linux/if_alg.h           |   1 +
>  6 files changed, 193 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
> index ff86befa61e0..ef7132802c2d 100644
> --- a/Documentation/crypto/userspace-if.rst
> +++ b/Documentation/crypto/userspace-if.rst
> @@ -296,15 +296,23 @@ follows:
>  
>      struct sockaddr_alg sa = {
>          .salg_family = AF_ALG,
> -        .salg_type = "rng", /* this selects the symmetric cipher */
> -        .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
> +        .salg_type = "rng", /* this selects the random number generator */
> +        .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
>      };
>  
>  
>  Depending on the RNG type, the RNG must be seeded. The seed is provided
>  using the setsockopt interface to set the key. For example, the
>  ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
> -seeded.
> +seeded. The seed is also known as a *Personalization String* in DRBG800-90A
> +standard.

Isn't the standard called "NIST SP 800-90A"?
"DRBG800-90A" doesn't return many hits on Google.

> +For the purpose of CAVP testing, the concatenation of *Entropy* and *Nonce*
> +can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface. This
> +requires a kernel built with CONFIG_CRYPTO_USER_API_CAVP_DRBG, and
> +CAP_SYS_ADMIN permission.
> +
> +*Additional Data* can be provided using the send()/sendmsg() system calls.

This doesn't make it clear whether the support for "Additional Data" is
dependent on CONFIG_CRYPTO_USER_API_CAVP_DRBG or not.

> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 091c0a0bbf26..7c8736f71681 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1896,6 +1896,15 @@ config CRYPTO_STATS
>  config CRYPTO_HASH_INFO
>  	bool
>  
> +config CRYPTO_USER_API_CAVP_DRBG
> +	tristate "Enable CAVP testing of DRBG"
> +	depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
> +	help
> +	  This option enables extra API for CAVP testing via the user-space
> +	  interface: resetting of DRBG entropy, and providing Additional Data.
> +	  This should only be enabled for CAVP testing. You should say
> +	  no unless you know what this is.

Using "tristate" here is incorrect because this option is not a module itself.
It's an option *for* a module.  So it needs to be "bool" instead.

Also, since this is an option to refine CRYPTO_USER_API_RNG, how about renaming
it to "CRYPTO_USER_API_RNG_CAVP", and moving it to just below the definition of
"CRYPTO_USER_API_RNG" so that they show up adjacent to each other?

> +static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> +			    int flags)
> +{
> +	struct sock *sk = sock->sk;
> +	struct alg_sock *ask = alg_sk(sk);
> +	struct rng_ctx *ctx = ask->private;
> +	int err;
> +
> +	lock_sock(sock->sk);
> +	err = _rng_recvmsg(ctx->drng, msg, len, ctx->addtl, ctx->addtl_len);
> +	rng_reset_addtl(ctx);
> +	release_sock(sock->sk);
> +
> +	return err ? err : len;

Shouldn't this just return the value that _rng_recvmsg() returned?

Also 'err' is conventionally just for 0 or -errno codes.  Use 'ret' if the
variable can also hold a length.

> +static struct proto_ops __maybe_unused algif_rng_test_ops = {
> +	.family		=	PF_ALG,
> +
> +	.connect	=	sock_no_connect,
> +	.socketpair	=	sock_no_socketpair,
> +	.getname	=	sock_no_getname,
> +	.ioctl		=	sock_no_ioctl,
> +	.listen		=	sock_no_listen,
> +	.shutdown	=	sock_no_shutdown,
> +	.getsockopt	=	sock_no_getsockopt,
> +	.mmap		=	sock_no_mmap,
> +	.bind		=	sock_no_bind,
> +	.accept		=	sock_no_accept,
> +	.setsockopt	=	sock_no_setsockopt,
> +	.sendpage	=	sock_no_sendpage,
> +
> +	.release	=	af_alg_release,
> +	.recvmsg	=	rng_test_recvmsg,
> +	.sendmsg	=	rng_test_sendmsg,
> +};
[...]
>  static const struct af_alg_type algif_type_rng = {
> @@ -171,7 +314,12 @@ 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_CAVP_DRBG)
> +	.setentropy	=	rng_setentropy,
> +	.ops		=	&algif_rng_test_ops,
> +#else
>  	.ops		=	&algif_rng_ops,
> +#endif
>  	.name		=	"rng",
>  	.owner		=	THIS_MODULE
>  };

I think that switching to the separate proto_ops structs made the patch worse.
Now there's duplicated code.

Since proto_ops are almost identical, and only one is used in a given kernel
build, why not just do:

static struct proto_ops algif_rng_ops = {
	...
#ifdef CONFIG_CRYPTO_USER_API_RNG_CAVP
	.sendmsg	= rng_sendmsg,
#else
	.sendmsg	= sock_no_sendmsg,
#endif
	...
};

Similarly for .recvmsg(), although I don't understand what's wrong with just
adding the lock_sock() instead...  The RNG algorithms do locking anyway, so it's
not like that would regress the ability to recvmsg() in parallel.  Also,
conditional locking depending on the kernel config makes it more difficult to
find kernel bugs like deadlocks.

- Eric