Re: [PATCH v8 3/4] crypto: AF_ALG -- add asymmetric cipher

[Tudor Ambarus <tudor.ambarus@microchip.com>]

Hi, Stephan,

On 08/10/2017 09:40 AM, Stephan Müller wrote:
> This patch adds the user space interface for asymmetric ciphers. The
> interface allows the use of sendmsg as well as vmsplice to provide data.
> 
> The akcipher interface implementation uses the common AF_ALG interface
> code regarding TX and RX SGL handling.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>   crypto/algif_akcipher.c | 466 ++++++++++++++++++++++++++++++++++++++++++++++++
>   include/crypto/if_alg.h |   2 +
>   2 files changed, 468 insertions(+)
>   create mode 100644 crypto/algif_akcipher.c
> 
> diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
> new file mode 100644
> index 000000000000..1b36eb0b6e8f
> --- /dev/null
> +++ b/crypto/algif_akcipher.c
> @@ -0,0 +1,466 @@
> +/*
> + * algif_akcipher: User-space interface for asymmetric cipher algorithms
> + *
> + * Copyright (C) 2017, Stephan Mueller <smueller@chronox.de>
> + *
> + * This file provides the user-space API for asymmetric ciphers.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + *
> + * The following concept of the memory management is used:
> + *
> + * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is
> + * filled by user space with the data submitted via sendpage/sendmsg. Filling
> + * up the TX SGL does not cause a crypto operation -- the data will only be
> + * tracked by the kernel. Upon receipt of one recvmsg call, the caller must
> + * provide a buffer which is tracked with the RX SGL.
> + *
> + * During the processing of the recvmsg operation, the cipher request is
> + * allocated and prepared. As part of the recvmsg operation, the processed
> + * TX buffers are extracted from the TX SGL into a separate SGL.
> + *
> + * After the completion of the crypto operation, the RX SGL and the cipher
> + * request is released. The extracted TX SGL parts are released together with
> + * the RX SGL release.
> + */
> +
> +#include <crypto/akcipher.h>
> +#include <crypto/scatterwalk.h>
> +#include <crypto/if_alg.h>
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/net.h>
> +#include <net/sock.h>

I like that you ordered these alphabetically. Is there a reason why
crypto/scatterwalk.h is not after crypto/if_alg.h?

> +
> +struct akcipher_tfm {
> +	struct crypto_akcipher *akcipher;
> +	bool has_key;
> +};
> +
> +static int akcipher_sendmsg(struct socket *sock, struct msghdr *msg,
> +			    size_t size)
> +{
> +	return af_alg_sendmsg(sock, msg, size, 0);
> +}
> +
> +static int _akcipher_recvmsg(struct socket *sock, struct msghdr *msg,
> +			     size_t ignored, int flags)
> +{
> +	struct sock *sk = sock->sk;
> +	struct alg_sock *ask = alg_sk(sk);
> +	struct sock *psk = ask->parent;
> +	struct alg_sock *pask = alg_sk(psk);
> +	struct af_alg_ctx *ctx = ask->private;
> +	struct akcipher_tfm *akc = pask->private;
> +	struct crypto_akcipher *tfm = akc->akcipher;
> +	struct af_alg_async_req *areq;
> +	int err = 0;

initialization not needed.

> +	int maxsize;
> +	size_t len = 0;

initialization not needed. len will be initialized in af_alg_get_rsgl.

Also, size_t could be 32 or 64 bit wide. Could you declare the size_t
variables before the int ones? This will avoid stack padding on 64bit
systems if one adds a new int variable in the same place where the ints
are now.

> +	size_t used = 0;

initialization to zero not needed. You can directly initialize to
ctx->used or don't initialize at all.

> +
> +	maxsize = crypto_akcipher_maxsize(tfm);
> +	if (maxsize < 0)
> +		return maxsize;
> +
> +	/* Allocate cipher request for current operation. */
> +	areq = af_alg_alloc_areq(sk, sizeof(struct af_alg_async_req) +
> +				     crypto_akcipher_reqsize(tfm));
> +	if (IS_ERR(areq))
> +		return PTR_ERR(areq);
> +
> +	/* convert iovecs of output buffers into RX SGL */
> +	err = af_alg_get_rsgl(sk, msg, flags, areq, maxsize, &len);
> +	if (err)
> +		goto free;
> +
> +	/* ensure output buffer is sufficiently large */
> +	if (len < maxsize) {
> +		err = -EMSGSIZE;
> +		goto free;
> +	}
> +
> +	/*
> +	 * Create a per request TX SGL for this request which tracks the
> +	 * SG entries from the global TX SGL.
> +	 */
> +	used = ctx->used;
> +	areq->tsgl_entries = af_alg_count_tsgl(sk, used, 0);
> +	if (!areq->tsgl_entries)
> +		areq->tsgl_entries = 1;
> +	areq->tsgl = sock_kmalloc(sk, sizeof(*areq->tsgl) * areq->tsgl_entries,
> +				  GFP_KERNEL);
> +	if (!areq->tsgl) {
> +		err = -ENOMEM;
> +		goto free;
> +	}
> +	sg_init_table(areq->tsgl, areq->tsgl_entries);
> +	af_alg_pull_tsgl(sk, used, areq->tsgl, 0);
> +
> +	/* Initialize the crypto operation */
> +	akcipher_request_set_tfm(&areq->cra_u.akcipher_req, tfm);
> +	akcipher_request_set_crypt(&areq->cra_u.akcipher_req, areq->tsgl,
> +				   areq->first_rsgl.sgl.sg, used, len);
> +
> +	if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
> +		/* AIO operation */
> +		areq->iocb = msg->msg_iocb;
> +		akcipher_request_set_callback(&areq->cra_u.akcipher_req,
> +					      CRYPTO_TFM_REQ_MAY_SLEEP,
> +					      af_alg_async_cb, areq);
> +	} else

Unbalanced braces around else statement.

> +		/* Synchronous operation */
> +		akcipher_request_set_callback(&areq->cra_u.akcipher_req,
> +					      CRYPTO_TFM_REQ_MAY_SLEEP |
> +					      CRYPTO_TFM_REQ_MAY_BACKLOG,
> +					      af_alg_complete,
> +					      &ctx->completion);
> +
> +	switch (ctx->op) {
> +	case ALG_OP_ENCRYPT:
> +		err = crypto_akcipher_encrypt(&areq->cra_u.akcipher_req);
> +		break;
> +	case ALG_OP_DECRYPT:
> +		err = crypto_akcipher_decrypt(&areq->cra_u.akcipher_req);
> +		break;
> +	case ALG_OP_SIGN:
> +		err = crypto_akcipher_sign(&areq->cra_u.akcipher_req);
> +		break;
> +	case ALG_OP_VERIFY:
> +		err = crypto_akcipher_verify(&areq->cra_u.akcipher_req);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		goto free;
> +	}
> +
> +	/* Wait for synchronous operation completion */
> +	if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb))
> +		err = af_alg_wait_for_completion(err, &ctx->completion);
> +
> +	/* AIO operation in progress */
> +	if (err == -EINPROGRESS) {
> +		sock_hold(sk);
> +
> +		/* Remember output size that will be generated. */
> +		areq->outlen = areq->cra_u.akcipher_req.dst_len;

don't you have the dst_len value in len variable?

> +
> +		return -EIOCBQUEUED;
> +	}
> +
> +free:
> +	af_alg_free_areq_sgls(areq);
> +	sock_kfree_s(sk, areq, areq->areqlen);
> +
> +	return err ? err : areq->cra_u.akcipher_req.dst_len;
> +}
> +
> +static int akcipher_recvmsg(struct socket *sock, struct msghdr *msg,
> +			    size_t ignored, int flags)
> +{
> +	struct sock *sk = sock->sk;
> +	struct alg_sock *ask = alg_sk(sk);
> +	struct sock *psk = ask->parent;
> +	struct alg_sock *pask = alg_sk(psk);
> +	struct akcipher_tfm *akc = pask->private;
> +	struct crypto_akcipher *tfm = akc->akcipher;
> +

blank line

> +	int ret = 0;
> +
> +	lock_sock(sk);
> +
> +	while (msg_data_left(msg)) {
> +		int err = _akcipher_recvmsg(sock, msg, ignored, flags);

If the compiler will not make any optimization, you will end up in
allocating an int on the stack for each iteration.

> +
> +		/*
> +		 * This error covers -EIOCBQUEUED which implies that we can
> +		 * only handle one AIO request. If the caller wants to have
> +		 * multiple AIO requests in parallel, he must make multiple
> +		 * separate AIO calls.
> +		 */
> +		if (err <= 0) {

why the equal?

> +			if (err == -EIOCBQUEUED || err == -EBADMSG || !ret)
> +				ret = err;
> +			goto out;
> +		}
> +
> +		ret += err;
> +
> +		/*
> +		 * The caller must provide crypto_akcipher_maxsize per request.
> +		 * If he provides more, we conclude that multiple akcipher
> +		 * operations are requested.
> +		 */
> +		iov_iter_advance(&msg->msg_iter,
> +				 crypto_akcipher_maxsize(tfm) - err);
> +	}
> +
> +out:
> +	af_alg_wmem_wakeup(sk);
> +	release_sock(sk);
> +	return ret;
> +}
> +
> +static struct proto_ops algif_akcipher_ops = {

static const struct?

> +	.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,
> +
> +	.release	=	af_alg_release,
> +	.sendmsg	=	akcipher_sendmsg,
> +	.sendpage	=	af_alg_sendpage,
> +	.recvmsg	=	akcipher_recvmsg,
> +	.poll		=	af_alg_poll,
> +};
> +
> +static int akcipher_check_key(struct socket *sock)
> +{
> +	int err = 0;

initialization not needed.
You can avoid stack padding on 64bit systems if you move the int after
pointers.

> +	struct sock *psk;
> +	struct alg_sock *pask;
> +	struct akcipher_tfm *tfm;
> +	struct sock *sk = sock->sk;
> +	struct alg_sock *ask = alg_sk(sk);
> +
> +	lock_sock(sk);
> +	if (ask->refcnt)
> +		goto unlock_child;
> +
> +	psk = ask->parent;
> +	pask = alg_sk(ask->parent);
> +	tfm = pask->private;
> +
> +	err = -ENOKEY;

see https://rusty.ozlabs.org/?p=232

> +	lock_sock_nested(psk, SINGLE_DEPTH_NESTING);
> +	if (!tfm->has_key)
> +		goto unlock;
> +
> +	if (!pask->refcnt++)
> +		sock_hold(psk);
> +
> +	ask->refcnt = 1;
> +	sock_put(psk);
> +
> +	err = 0;
> +
> +unlock:
> +	release_sock(psk);
> +unlock_child:
> +	release_sock(sk);
> +
> +	return err;
> +}
> +
> +static int akcipher_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
> +				  size_t size)
> +{
> +	int err;
> +
> +	err = akcipher_check_key(sock);
> +	if (err)
> +		return err;
> +
> +	return akcipher_sendmsg(sock, msg, size);
> +}
> +
> +static ssize_t akcipher_sendpage_nokey(struct socket *sock, struct page *page,
> +				       int offset, size_t size, int flags)
> +{
> +	int err;
> +
> +	err = akcipher_check_key(sock);
> +	if (err)
> +		return err;
> +
> +	return af_alg_sendpage(sock, page, offset, size, flags);
> +}
> +
> +static int akcipher_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
> +				  size_t ignored, int flags)
> +{
> +	int err;
> +
> +	err = akcipher_check_key(sock);
> +	if (err)
> +		return err;
> +
> +	return akcipher_recvmsg(sock, msg, ignored, flags);
> +}
> +
> +static struct proto_ops algif_akcipher_ops_nokey = {

static const struct?

> +	.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,
> +
> +	.release	=	af_alg_release,
> +	.sendmsg	=	akcipher_sendmsg_nokey,
> +	.sendpage	=	akcipher_sendpage_nokey,
> +	.recvmsg	=	akcipher_recvmsg_nokey,
> +	.poll		=	af_alg_poll,
> +};
> +
> +static void *akcipher_bind(const char *name, u32 type, u32 mask)
> +{
> +	struct akcipher_tfm *tfm;
> +	struct crypto_akcipher *akcipher;
> +
> +	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
> +	if (!tfm)
> +		return ERR_PTR(-ENOMEM);
> +
> +	akcipher = crypto_alloc_akcipher(name, type, mask);
> +	if (IS_ERR(akcipher)) {
> +		kfree(tfm);
> +		return ERR_CAST(akcipher);
> +	}
> +
> +	tfm->akcipher = akcipher;

tfm->akcipher was initialized to zero and now you overwrite it.
Maybe it's better to use kmalloc and tfm->has_key = false so you
can get rid of the superfluous initialization.

> +
> +	return tfm;
> +}
> +
> +static void akcipher_release(void *private)
> +{
> +	struct akcipher_tfm *tfm = private;
> +	struct crypto_akcipher *akcipher = tfm->akcipher;
> +
> +	crypto_free_akcipher(akcipher);
> +	kfree(tfm);
> +}
> +
> +static int akcipher_setprivkey(void *private, const u8 *key,
> +			       unsigned int keylen)
> +{
> +	struct akcipher_tfm *tfm = private;
> +	struct crypto_akcipher *akcipher = tfm->akcipher;
> +	int err;
> +
> +	err = crypto_akcipher_set_priv_key(akcipher, key, keylen);
> +	tfm->has_key = !err;
> +
> +	/* Return the maximum size of the akcipher operation. */
> +	if (!err)
> +		err = crypto_akcipher_maxsize(akcipher);

crypto subsystem returns zero when setkey is successful and introduces
a new function for determining the maxsize. Should we comply with that?

> +
> +	return err;
> +}
> +
> +static int akcipher_setpubkey(void *private, const u8 *key, unsigned int keylen)
> +{
> +	struct akcipher_tfm *tfm = private;
> +	struct crypto_akcipher *akcipher = tfm->akcipher;
> +	int err;
> +
> +	err = crypto_akcipher_set_pub_key(akcipher, key, keylen);
> +	tfm->has_key = !err;
> +
> +	/* Return the maximum size of the akcipher operation. */
> +	if (!err)
> +		err = crypto_akcipher_maxsize(akcipher);
> +
> +	return err;
> +}
> +
> +static void akcipher_sock_destruct(struct sock *sk)
> +{
> +	struct alg_sock *ask = alg_sk(sk);
> +	struct af_alg_ctx *ctx = ask->private;
> +
> +	af_alg_pull_tsgl(sk, ctx->used, NULL, 0);
> +	sock_kfree_s(sk, ctx, ctx->len);
> +	af_alg_release_parent(sk);
> +}
> +
> +static int akcipher_accept_parent_nokey(void *private, struct sock *sk)
> +{
> +	struct af_alg_ctx *ctx;
> +	struct alg_sock *ask = alg_sk(sk);
> +	unsigned int len = sizeof(*ctx);
> +
> +	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +	memset(ctx, 0, len);

do you really need the memset? you can initialize the members that you
will use as you did below.

> +
> +	INIT_LIST_HEAD(&ctx->tsgl_list);
> +	ctx->len = len;
> +	ctx->used = 0;
> +	ctx->rcvused = 0;
> +	ctx->more = 0;
> +	ctx->merge = 0;
> +	ctx->op = 0;
> +	af_alg_init_completion(&ctx->completion);
> +
> +	ask->private = ctx;
> +
> +	sk->sk_destruct = akcipher_sock_destruct;
> +
> +	return 0;
> +}
> +
> +static int akcipher_accept_parent(void *private, struct sock *sk)
> +{
> +	struct akcipher_tfm *tfm = private;
> +
> +	if (!tfm->has_key)
> +		return -ENOKEY;
> +
> +	return akcipher_accept_parent_nokey(private, sk);
> +}
> +
> +static const struct af_alg_type algif_type_akcipher = {
> +	.bind		=	akcipher_bind,
> +	.release	=	akcipher_release,
> +	.setkey		=	akcipher_setprivkey,
> +	.setpubkey	=	akcipher_setpubkey,
> +	.setauthsize	=	NULL,
> +	.accept		=	akcipher_accept_parent,
> +	.accept_nokey	=	akcipher_accept_parent_nokey,
> +	.ops		=	&algif_akcipher_ops,
> +	.ops_nokey	=	&algif_akcipher_ops_nokey,
> +	.name		=	"akcipher",
> +	.owner		=	THIS_MODULE
> +};
> +
> +static int __init algif_akcipher_init(void)
> +{
> +	return af_alg_register_type(&algif_type_akcipher);
> +}
> +
> +static void __exit algif_akcipher_exit(void)
> +{
> +	int err = af_alg_unregister_type(&algif_type_akcipher);

checkpatch suggests to insert a blank line after declaration.

Cheers,
ta

> +	BUG_ON(err);
> +}
> +
> +module_init(algif_akcipher_init);
> +module_exit(algif_akcipher_exit);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Stephan Mueller <smueller@chronox.de>");
> +MODULE_DESCRIPTION("Asymmetric kernel crypto API user space interface");
> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index d1de8ed3e77b..a2b396756e24 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -22,6 +22,7 @@
>   
>   #include <crypto/aead.h>
>   #include <crypto/skcipher.h>
> +#include <crypto/akcipher.h>
>   
>   #define ALG_MAX_PAGES			16
>   
> @@ -119,6 +120,7 @@ struct af_alg_async_req {
>   	union {
>   		struct aead_request aead_req;
>   		struct skcipher_request skcipher_req;
> +		struct akcipher_request akcipher_req;
>   	} cra_u;
>   
>   	/* req ctx trails this struct */
>