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, 28 Jul 2020 10:36:03 -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 v3] crypto: af_alg - add extra parameters for DRBG
 interface
Message-ID: <20200728173603.GD4053562@gmail.com>
References: <20200722155905.GA789@sol.localdomain>
 <20200728155159.2156480-1-lenaptr@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200728155159.2156480-1-lenaptr@google.com>
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 28, 2020 at 04:51:59PM +0100, Elena Petrova wrote:
> +static int rng_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;
> +
> +	rng_reset_addtl(ctx);
> +	ctx->addtl = kmalloc(len, GFP_KERNEL);
> +	if (!ctx->addtl) {
> +		err = -ENOMEM;
> +		goto unlock;

This error code isn't actually returned.

> +	}
> +
> +	err = memcpy_from_msg(ctx->addtl, msg, len);
> +	if (err) {
> +		rng_reset_addtl(ctx);
> +		goto unlock;

Likewise.

> +#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
> +static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
> +{
> +	struct rng_parent_ctx *pctx = private;
> +	u8 *kentropy = NULL;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;

This should be EACCES, not EPERM.  EACCES means the operation will succeed if
you acquire the needed privileges.  EPERM means it will never succeed.

> +	if (len > MAXSIZE)
> +		len = MAXSIZE;

Truncating the length is error prone.  Shouldn't this instead return an error
(EMSGSIZE?) if the length is too long, and 0 on success?  Remember this is
setsockopt(), not write().

- Eric