LinuxLists.cc - [PATCH v6 0/6] crypto: algif

[permalink] [raw]

Subject: [PATCH v6 1/6] crypto: AF_ALG -- add sign/verify API

From: Stephan Mueller <[email protected]>

Add the flags for handling signature generation and signature
verification.

Also, the patch adds the interface for setting a public key.

Signed-off-by: Stephan Mueller <[email protected]>
Signed-off-by: Tadeusz Struk <[email protected]>
---
include/uapi/linux/if_alg.h | 3 +++
1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index f2acd2f..02e6162 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -34,9 +34,12 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_PUBKEY 6

/* Operations */
#define ALG_OP_DECRYPT 0
#define ALG_OP_ENCRYPT 1
+#define ALG_OP_SIGN 2
+#define ALG_OP_VERIFY 3

#endif /* _LINUX_IF_ALG_H */

2016-05-15 04:18:10

[permalink] [raw]

Subject: [PATCH v6 4/6] crypto: algif_akcipher - enable compilation

From: Stephan Mueller <[email protected]>

Add the Makefile and Kconfig updates to allow algif_akcipher to be
compiled.

Signed-off-by: Stephan Mueller <[email protected]>
Signed-off-by: Tadeusz Struk <[email protected]>
---
crypto/Kconfig | 9 +++++++++
crypto/Makefile | 1 +
2 files changed, 10 insertions(+)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 93a1fdc..b932319 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1626,6 +1626,15 @@ config CRYPTO_USER_API_AEAD
This option enables the user-spaces interface for AEAD
cipher algorithms.

+config CRYPTO_USER_API_AKCIPHER
+ tristate "User-space interface for asymmetric key cipher algorithms"
+ depends on NET
+ select CRYPTO_AKCIPHER2
+ select CRYPTO_USER_API
+ help
+ This option enables the user-spaces interface for asymmetric
+ key cipher algorithms.
+
config CRYPTO_HASH_INFO
bool

diff --git a/crypto/Makefile b/crypto/Makefile
index 4f4ef7e..c51ac16 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -121,6 +121,7 @@ obj-$(CONFIG_CRYPTO_USER_API_HASH) += algif_hash.o
obj-$(CONFIG_CRYPTO_USER_API_SKCIPHER) += algif_skcipher.o
obj-$(CONFIG_CRYPTO_USER_API_RNG) += algif_rng.o
obj-$(CONFIG_CRYPTO_USER_API_AEAD) += algif_aead.o
+obj-$(CONFIG_CRYPTO_USER_API_AKCIPHER) += algif_akcipher.o

#
# generic algorithms and the async_tx api

2016-05-15 04:17:13

[permalink] [raw]

Subject: [PATCH v6 5/6] crypto: algif_akcipher - add ops_nokey

Similar to algif_skcipher and algif_hash, algif_akcipher needs
to prevent user space from using the interface in an improper way.
This patch adds nokey ops handlers, which do just that.

Signed-off-by: Tadeusz Struk <[email protected]>
---
crypto/algif_akcipher.c | 159 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 152 insertions(+), 7 deletions(-)

diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
index 6342b6e..e00793d 100644
--- a/crypto/algif_akcipher.c
+++ b/crypto/algif_akcipher.c
@@ -27,6 +27,11 @@ struct akcipher_sg_list {
struct scatterlist sg[ALG_MAX_PAGES];
};

+struct akcipher_tfm {
+ struct crypto_akcipher *akcipher;
+ bool has_key;
+};
+
struct akcipher_ctx {
struct akcipher_sg_list tsgl;
struct af_alg_sgl rsgl[ALG_MAX_PAGES];
@@ -450,25 +455,151 @@ static struct proto_ops algif_akcipher_ops = {
.poll = akcipher_poll,
};

+static int akcipher_check_key(struct socket *sock)
+{
+ int err = 0;
+ 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;
+ 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 akcipher_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 = {
+ .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 = akcipher_poll,
+};
+
static void *akcipher_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_akcipher(name, type, 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;
+ return tfm;
}

static void akcipher_release(void *private)
{
- crypto_free_akcipher(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)
{
- return crypto_akcipher_set_priv_key(private, key, 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 err;
}

static int akcipher_setpubkey(void *private, const u8 *key, unsigned int keylen)
{
- return crypto_akcipher_set_pub_key(private, key, 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 err;
}

static void akcipher_sock_destruct(struct sock *sk)
@@ -481,11 +612,13 @@ static void akcipher_sock_destruct(struct sock *sk)
af_alg_release_parent(sk);
}

-static int akcipher_accept_parent(void *private, struct sock *sk)
+static int akcipher_accept_parent_nokey(void *private, struct sock *sk)
{
struct akcipher_ctx *ctx;
struct alg_sock *ask = alg_sk(sk);
- unsigned int len = sizeof(*ctx) + crypto_akcipher_reqsize(private);
+ struct akcipher_tfm *tfm = private;
+ struct crypto_akcipher *akcipher = tfm->akcipher;
+ unsigned int len = sizeof(*ctx) + crypto_akcipher_reqsize(akcipher);

ctx = sock_kmalloc(sk, len, GFP_KERNEL);
if (!ctx)
@@ -503,7 +636,7 @@ static int akcipher_accept_parent(void *private, struct sock *sk)

ask->private = ctx;

- akcipher_request_set_tfm(&ctx->req, private);
+ akcipher_request_set_tfm(&ctx->req, akcipher);
akcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
af_alg_complete, &ctx->completion);

@@ -512,13 +645,25 @@ static int akcipher_accept_parent(void *private, struct sock *sk)
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,
.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
};

2016-05-15 04:17:19

[permalink] [raw]

Subject: [PATCH v6 6/6] crypto: AF_ALG - add support for key_id

This patch adds support for asymmetric key type to AF_ALG.
It will work as follows: A new PF_ALG socket options are
added on top of existing ALG_SET_KEY and ALG_SET_PUBKEY, namely
ALG_SET_KEY_ID and ALG_SET_PUBKEY_ID for setting public and
private keys respectively. When these new options will be used
the user, instead of providing the key material, will provide a
key id and the key itself will be obtained from kernel keyring
subsystem. The user will use the standard tools (keyctl tool
or the keyctl syscall) for key instantiation and to obtain the
key id. The key id can also be obtained by reading the
/proc/keys file.

When a key corresponding to the given keyid is found, it is stored
in the socket context and subsequent crypto operation invoked by the
user will use the new asymmetric accessor functions instead of akcipher
api. The asymmetric subtype can internally use akcipher api or
invoke operations defined by a given subtype, depending on the
key type.

Signed-off-by: Tadeusz Struk <[email protected]>
---
crypto/af_alg.c | 10 ++
crypto/algif_akcipher.c | 207 ++++++++++++++++++++++++++++++++++++++++++-
include/crypto/if_alg.h | 1
include/uapi/linux/if_alg.h | 2
4 files changed, 215 insertions(+), 5 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 24dc082..59c8244 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -260,6 +260,16 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,

err = alg_setkey(sk, optval, optlen, type->setpubkey);
break;
+
+ case ALG_SET_KEY_ID:
+ case ALG_SET_PUBKEY_ID:
+ /* ALG_SET_KEY_ID is only for akcipher */
+ if (!strcmp(type->name, "akcipher") ||
+ sock->state == SS_CONNECTED)
+ goto unlock;
+
+ err = alg_setkey(sk, optval, optlen, type->setkeyid);
+ break;
case ALG_SET_AEAD_AUTHSIZE:
if (sock->state == SS_CONNECTED)
goto unlock;
diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
index e00793d..6733df1 100644
--- a/crypto/algif_akcipher.c
+++ b/crypto/algif_akcipher.c
@@ -14,6 +14,8 @@
#include <crypto/akcipher.h>
#include <crypto/scatterwalk.h>
#include <crypto/if_alg.h>
+#include <crypto/public_key.h>
+#include <keys/asymmetric-type.h>
#include <linux/init.h>
#include <linux/list.h>
#include <linux/kernel.h>
@@ -29,6 +31,7 @@ struct akcipher_sg_list {

struct akcipher_tfm {
struct crypto_akcipher *akcipher;
+ char keyid[12];
bool has_key;
};

@@ -37,6 +40,7 @@ struct akcipher_ctx {
struct af_alg_sgl rsgl[ALG_MAX_PAGES];

struct af_alg_completion completion;
+ struct key *key;

unsigned long used;

@@ -322,6 +326,153 @@ unlock:
return err ? err : size;
}

+static int asym_key_encrypt(const struct key *key, struct akcipher_request *req)
+{
+ struct kernel_pkey_params params = {0};
+ char *src = NULL, *dst = NULL, *in, *out;
+ int ret;
+
+ if (!sg_is_last(req->src)) {
+ src = kmalloc(req->src_len, GFP_KERNEL);
+ if (!src)
+ return -ENOMEM;
+ scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+ in = src;
+ } else {
+ in = sg_virt(req->src);
+ }
+ if (!sg_is_last(req->dst)) {
+ dst = kmalloc(req->dst_len, GFP_KERNEL);
+ if (!dst) {
+ kfree(src);
+ return -ENOMEM;
+ }
+ out = dst;
+ } else {
+ out = sg_virt(req->dst);
+ }
+ params.key = (struct key *)key;
+ params.in_len = req->src_len;
+ params.out_len = req->dst_len;
+ ret = encrypt_blob(&params, in, out);
+ if (ret)
+ goto free;
+
+ if (dst)
+ scatterwalk_map_and_copy(dst, req->dst, 0, req->dst_len, 1);
+free:
+ kfree(src);
+ kfree(dst);
+ return ret;
+}
+
+static int asym_key_decrypt(const struct key *key, struct akcipher_request *req)
+{
+ struct kernel_pkey_params params = {0};
+ char *src = NULL, *dst = NULL, *in, *out;
+ int ret;
+
+ if (!sg_is_last(req->src)) {
+ src = kmalloc(req->src_len, GFP_KERNEL);
+ if (!src)
+ return -ENOMEM;
+ scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+ in = src;
+ } else {
+ in = sg_virt(req->src);
+ }
+ if (!sg_is_last(req->dst)) {
+ dst = kmalloc(req->dst_len, GFP_KERNEL);
+ if (!dst) {
+ kfree(src);
+ return -ENOMEM;
+ }
+ out = dst;
+ } else {
+ out = sg_virt(req->dst);
+ }
+ params.key = (struct key *)key;
+ params.in_len = req->src_len;
+ params.out_len = req->dst_len;
+ ret = decrypt_blob(&params, in, out);
+ if (ret)
+ goto free;
+
+ if (dst)
+ scatterwalk_map_and_copy(dst, req->dst, 0, req->dst_len, 1);
+free:
+ kfree(src);
+ kfree(dst);
+ return ret;
+}
+
+static int asym_key_sign(const struct key *key, struct akcipher_request *req)
+{
+ struct kernel_pkey_params params = {0};
+ char *src = NULL, *dst = NULL, *in, *out;
+ int ret;
+
+ if (!sg_is_last(req->src)) {
+ src = kmalloc(req->src_len, GFP_KERNEL);
+ if (!src)
+ return -ENOMEM;
+ scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+ in = src;
+ } else {
+ in = sg_virt(req->src);
+ }
+ if (!sg_is_last(req->dst)) {
+ dst = kmalloc(req->dst_len, GFP_KERNEL);
+ if (!dst) {
+ kfree(src);
+ return -ENOMEM;
+ }
+ out = dst;
+ } else {
+ out = sg_virt(req->dst);
+ }
+ params.key = (struct key *)key;
+ params.in_len = req->src_len;
+ params.out_len = req->dst_len;
+ ret = create_signature(&params, in, out);
+ if (ret)
+ goto free;
+
+ if (dst)
+ scatterwalk_map_and_copy(dst, req->dst, 0, req->dst_len, 1);
+free:
+ kfree(src);
+ kfree(dst);
+ return ret;
+}
+
+static int asym_key_verify(const struct key *key, struct akcipher_request *req)
+{
+ struct public_key_signature sig;
+ char *src = NULL, *in;
+ int ret;
+
+ if (!sg_is_last(req->src)) {
+ src = kmalloc(req->src_len, GFP_KERNEL);
+ if (!src)
+ return -ENOMEM;
+ scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+ in = src;
+ } else {
+ in = sg_virt(req->src);
+ }
+ sig.pkey_algo = "rsa";
+ sig.encoding = "pkcs1";
+ /* Need to find a way to pass the hash param */
+ sig.hash_algo = "sha1";
+ sig.digest_size = 20;
+ sig.s_size = req->src_len;
+ sig.s = src;
+ ret = verify_signature(key, NULL, &sig);
+ kfree(src);
+ return ret;
+}
+
static int akcipher_recvmsg(struct socket *sock, struct msghdr *msg,
size_t ignored, int flags)
{
@@ -377,16 +528,28 @@ static int akcipher_recvmsg(struct socket *sock, struct msghdr *msg,
usedpages);
switch (ctx->op) {
case ALG_OP_VERIFY:
- err = crypto_akcipher_verify(&ctx->req);
+ if (ctx->key)
+ err = asym_key_verify(ctx->key, &ctx->req);
+ else
+ err = crypto_akcipher_verify(&ctx->req);
break;
case ALG_OP_SIGN:
- err = crypto_akcipher_sign(&ctx->req);
+ if (ctx->key)
+ err = asym_key_sign(ctx->key, &ctx->req);
+ else
+ err = crypto_akcipher_sign(&ctx->req);
break;
case ALG_OP_ENCRYPT:
- err = crypto_akcipher_encrypt(&ctx->req);
+ if (ctx->key)
+ err = asym_key_encrypt(ctx->key, &ctx->req);
+ else
+ err = crypto_akcipher_encrypt(&ctx->req);
break;
case ALG_OP_DECRYPT:
- err = crypto_akcipher_decrypt(&ctx->req);
+ if (ctx->key)
+ err = asym_key_decrypt(ctx->key, &ctx->req);
+ else
+ err = crypto_akcipher_decrypt(&ctx->req);
break;
default:
err = -EFAULT;
@@ -579,6 +742,27 @@ static void akcipher_release(void *private)
kfree(tfm);
}

+static int akcipher_setkeyid(void *private, const u8 *key, unsigned int keylen)
+{
+ struct akcipher_tfm *tfm = private;
+ struct key *akey;
+ u32 keyid = *((u32 *)key);
+ int err = -ENOKEY;
+
+ /* Store the key id and verify that a key with the given id is present.
+ * The actual key will be acquired in the accept_parent function
+ */
+ sprintf(tfm->keyid, "id:%08x", keyid);
+ akey = request_key(&key_type_asymmetric, tfm->keyid, NULL);
+ if (IS_ERR(key))
+ goto out;
+
+ tfm->has_key = true;
+ key_put(akey);
+out:
+ return err;
+}
+
static int akcipher_setprivkey(void *private, const u8 *key,
unsigned int keylen)
{
@@ -610,6 +794,8 @@ static void akcipher_sock_destruct(struct sock *sk)
akcipher_put_sgl(sk);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
+ if (ctx->key)
+ key_put(ctx->key);
}

static int akcipher_accept_parent_nokey(void *private, struct sock *sk)
@@ -618,6 +804,7 @@ static int akcipher_accept_parent_nokey(void *private, struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct akcipher_tfm *tfm = private;
struct crypto_akcipher *akcipher = tfm->akcipher;
+ struct key *key;
unsigned int len = sizeof(*ctx) + crypto_akcipher_reqsize(akcipher);

ctx = sock_kmalloc(sk, len, GFP_KERNEL);
@@ -634,11 +821,20 @@ static int akcipher_accept_parent_nokey(void *private, struct sock *sk)
af_alg_init_completion(&ctx->completion);
sg_init_table(ctx->tsgl.sg, ALG_MAX_PAGES);

- ask->private = ctx;
+ if (strlen(tfm->keyid)) {
+ key = request_key(&key_type_asymmetric, tfm->keyid, NULL);
+ if (IS_ERR(key)) {
+ sock_kfree_s(sk, ctx, len);
+ return -ENOKEY;
+ }

+ ctx->key = key;
+ memset(tfm->keyid, '\0', sizeof(tfm->keyid));
+ }
akcipher_request_set_tfm(&ctx->req, akcipher);
akcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
af_alg_complete, &ctx->completion);
+ ask->private = ctx;

sk->sk_destruct = akcipher_sock_destruct;

@@ -660,6 +856,7 @@ static const struct af_alg_type algif_type_akcipher = {
.release = akcipher_release,
.setkey = akcipher_setprivkey,
.setpubkey = akcipher_setpubkey,
+ .setkeyid = akcipher_setkeyid,
.accept = akcipher_accept_parent,
.accept_nokey = akcipher_accept_parent_nokey,
.ops = &algif_akcipher_ops,
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 6c3e6e7..09c99ab 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -53,6 +53,7 @@ struct af_alg_type {
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
int (*setpubkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setkeyid)(void *private, const u8 *key, unsigned int keylen);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index 02e6162..0379766 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,8 @@ struct af_alg_iv {
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
#define ALG_SET_PUBKEY 6
+#define ALG_SET_PUBKEY_ID 7
+#define ALG_SET_KEY_ID 8

/* Operations */
#define ALG_OP_DECRYPT 0

2016-05-15 04:17:01

[permalink] [raw]

Subject: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

From: Stephan Mueller <[email protected]>

This patch adds the user space interface for asymmetric ciphers. The
interface allows the use of sendmsg as well as vmsplice to provide data.

This version has been rebased on top of 4.6 and a few chackpatch issues
have been fixed.

Signed-off-by: Stephan Mueller <[email protected]>
Signed-off-by: Tadeusz Struk <[email protected]>
---
crypto/algif_akcipher.c | 542 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 542 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 0000000..6342b6e
--- /dev/null
+++ b/crypto/algif_akcipher.c
@@ -0,0 +1,542 @@
+/*
+ * algif_akcipher: User-space interface for asymmetric cipher algorithms
+ *
+ * Copyright (C) 2015, Stephan Mueller <[email protected]>
+ *
+ * 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.
+ */
+
+#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>
+
+struct akcipher_sg_list {
+ unsigned int cur;
+ struct scatterlist sg[ALG_MAX_PAGES];
+};
+
+struct akcipher_ctx {
+ struct akcipher_sg_list tsgl;
+ struct af_alg_sgl rsgl[ALG_MAX_PAGES];
+
+ struct af_alg_completion completion;
+
+ unsigned long used;
+
+ unsigned int len;
+ bool more;
+ bool merge;
+ int op;
+
+ struct akcipher_request req;
+};
+
+static inline int akcipher_sndbuf(struct sock *sk)
+{
+ struct alg_sock *ask = alg_sk(sk);
+ struct akcipher_ctx *ctx = ask->private;
+
+ return max_t(int, max_t(int, sk->sk_sndbuf & PAGE_MASK, PAGE_SIZE) -
+ ctx->used, 0);
+}
+
+static inline bool akcipher_writable(struct sock *sk)
+{
+ return akcipher_sndbuf(sk) >= PAGE_SIZE;
+}
+
+static inline int akcipher_calcsize(struct akcipher_ctx *ctx)
+{
+ return crypto_akcipher_maxsize(crypto_akcipher_reqtfm(&ctx->req));
+}
+
+static void akcipher_put_sgl(struct sock *sk)
+{
+ struct alg_sock *ask = alg_sk(sk);
+ struct akcipher_ctx *ctx = ask->private;
+ struct akcipher_sg_list *sgl = &ctx->tsgl;
+ struct scatterlist *sg = sgl->sg;
+ unsigned int i;
+
+ for (i = 0; i < sgl->cur; i++) {
+ if (!sg_page(sg + i))
+ continue;
+
+ put_page(sg_page(sg + i));
+ sg_assign_page(sg + i, NULL);
+ }
+ sg_init_table(sg, ALG_MAX_PAGES);
+ sgl->cur = 0;
+ ctx->used = 0;
+ ctx->more = 0;
+ ctx->merge = 0;
+}
+
+static void akcipher_wmem_wakeup(struct sock *sk)
+{
+ struct socket_wq *wq;
+
+ if (!akcipher_writable(sk))
+ return;
+
+ rcu_read_lock();
+ wq = rcu_dereference(sk->sk_wq);
+ if (wq_has_sleeper(&wq->wait))
+ wake_up_interruptible_sync_poll(&wq->wait, POLLIN |
+ POLLRDNORM |
+ POLLRDBAND);
+ sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
+ rcu_read_unlock();
+}
+
+static int akcipher_wait_for_data(struct sock *sk, unsigned int flags)
+{
+ struct alg_sock *ask = alg_sk(sk);
+ struct akcipher_ctx *ctx = ask->private;
+ long timeout;
+ DEFINE_WAIT(wait);
+ int err = -ERESTARTSYS;
+
+ if (flags & MSG_DONTWAIT)
+ return -EAGAIN;
+
+ set_bit(SOCKWQ_ASYNC_WAITDATA, &sk->sk_socket->flags);
+
+ for (;;) {
+ if (signal_pending(current))
+ break;
+ prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
+ timeout = MAX_SCHEDULE_TIMEOUT;
+ if (sk_wait_event(sk, &timeout, !ctx->more)) {
+ err = 0;
+ break;
+ }
+ }
+ finish_wait(sk_sleep(sk), &wait);
+
+ clear_bit(SOCKWQ_ASYNC_WAITDATA, &sk->sk_socket->flags);
+
+ return err;
+}
+
+static void akcipher_data_wakeup(struct sock *sk)
+{
+ struct alg_sock *ask = alg_sk(sk);
+ struct akcipher_ctx *ctx = ask->private;
+ struct socket_wq *wq;
+
+ if (ctx->more)
+ return;
+ if (!ctx->used)
+ return;
+
+ rcu_read_lock();
+ wq = rcu_dereference(sk->sk_wq);
+ if (wq_has_sleeper(&wq->wait))
+ wake_up_interruptible_sync_poll(&wq->wait, POLLOUT |
+ POLLRDNORM |
+ POLLRDBAND);
+ sk_wake_async(sk, SOCK_WAKE_SPACE, POLL_OUT);
+ rcu_read_unlock();
+}
+
+static int akcipher_sendmsg(struct socket *sock, struct msghdr *msg,
+ size_t size)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct akcipher_ctx *ctx = ask->private;
+ struct akcipher_sg_list *sgl = &ctx->tsgl;
+ struct af_alg_control con = {};
+ long copied = 0;
+ int op = 0;
+ bool init = 0;
+ int err;
+
+ if (msg->msg_controllen) {
+ err = af_alg_cmsg_send(msg, &con);
+ if (err)
+ return err;
+
+ init = 1;
+ switch (con.op) {
+ case ALG_OP_VERIFY:
+ case ALG_OP_SIGN:
+ case ALG_OP_ENCRYPT:
+ case ALG_OP_DECRYPT:
+ op = con.op;
+ break;
+ default:
+ return -EINVAL;
+ }
+ }
+
+ lock_sock(sk);
+ if (!ctx->more && ctx->used)
+ goto unlock;
+
+ if (init)
+ ctx->op = op;
+
+ while (size) {
+ unsigned long len = size;
+ struct scatterlist *sg = NULL;
+
+ /* use the existing memory in an allocated page */
+ if (ctx->merge) {
+ sg = sgl->sg + sgl->cur - 1;
+ len = min_t(unsigned long, len,
+ PAGE_SIZE - sg->offset - sg->length);
+ err = memcpy_from_msg(page_address(sg_page(sg)) +
+ sg->offset + sg->length,
+ msg, len);
+ if (err)
+ goto unlock;
+
+ sg->length += len;
+ ctx->merge = (sg->offset + sg->length) &
+ (PAGE_SIZE - 1);
+
+ ctx->used += len;
+ copied += len;
+ size -= len;
+ continue;
+ }
+
+ if (!akcipher_writable(sk)) {
+ /* user space sent too much data */
+ akcipher_put_sgl(sk);
+ err = -EMSGSIZE;
+ goto unlock;
+ }
+
+ /* allocate a new page */
+ len = min_t(unsigned long, size, akcipher_sndbuf(sk));
+ while (len) {
+ int plen = 0;
+
+ if (sgl->cur >= ALG_MAX_PAGES) {
+ akcipher_put_sgl(sk);
+ err = -E2BIG;
+ goto unlock;
+ }
+
+ sg = sgl->sg + sgl->cur;
+ plen = min_t(int, len, PAGE_SIZE);
+
+ sg_assign_page(sg, alloc_page(GFP_KERNEL));
+ if (!sg_page(sg)) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ err = memcpy_from_msg(page_address(sg_page(sg)),
+ msg, plen);
+ if (err) {
+ __free_page(sg_page(sg));
+ sg_assign_page(sg, NULL);
+ goto unlock;
+ }
+
+ sg->offset = 0;
+ sg->length = plen;
+ len -= plen;
+ ctx->used += plen;
+ copied += plen;
+ sgl->cur++;
+ size -= plen;
+ ctx->merge = plen & (PAGE_SIZE - 1);
+ }
+ }
+
+ err = 0;
+
+ ctx->more = msg->msg_flags & MSG_MORE;
+
+unlock:
+ akcipher_data_wakeup(sk);
+ release_sock(sk);
+
+ return err ?: copied;
+}
+
+static ssize_t akcipher_sendpage(struct socket *sock, struct page *page,
+ int offset, size_t size, int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct akcipher_ctx *ctx = ask->private;
+ struct akcipher_sg_list *sgl = &ctx->tsgl;
+ int err = 0;
+
+ if (flags & MSG_SENDPAGE_NOTLAST)
+ flags |= MSG_MORE;
+
+ if (sgl->cur >= ALG_MAX_PAGES)
+ return -E2BIG;
+
+ lock_sock(sk);
+ if (!ctx->more && ctx->used)
+ goto unlock;
+
+ if (!size)
+ goto done;
+
+ if (!akcipher_writable(sk)) {
+ /* user space sent too much data */
+ akcipher_put_sgl(sk);
+ err = -EMSGSIZE;
+ goto unlock;
+ }
+
+ ctx->merge = 0;
+
+ get_page(page);
+ sg_set_page(sgl->sg + sgl->cur, page, size, offset);
+ sgl->cur++;
+ ctx->used += size;
+
+done:
+ ctx->more = flags & MSG_MORE;
+unlock:
+ akcipher_data_wakeup(sk);
+ release_sock(sk);
+
+ return err ? err : size;
+}
+
+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 akcipher_ctx *ctx = ask->private;
+ struct akcipher_sg_list *sgl = &ctx->tsgl;
+ unsigned int i = 0;
+ int err;
+ unsigned long used = 0;
+ size_t usedpages = 0;
+ unsigned int cnt = 0;
+
+ /* Limit number of IOV blocks to be accessed below */
+ if (msg->msg_iter.nr_segs > ALG_MAX_PAGES)
+ return -ENOMSG;
+
+ lock_sock(sk);
+
+ if (ctx->more) {
+ err = akcipher_wait_for_data(sk, flags);
+ if (err)
+ goto unlock;
+ }
+
+ used = ctx->used;
+
+ /* convert iovecs of output buffers into scatterlists */
+ while (iov_iter_count(&msg->msg_iter)) {
+ /* make one iovec available as scatterlist */
+ err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
+ iov_iter_count(&msg->msg_iter));
+ if (err < 0)
+ goto unlock;
+ usedpages += err;
+ /* chain the new scatterlist with previous one */
+ if (cnt)
+ af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
+
+ iov_iter_advance(&msg->msg_iter, err);
+ cnt++;
+ }
+
+ /* ensure output buffer is sufficiently large */
+ if (usedpages < akcipher_calcsize(ctx)) {
+ err = -EMSGSIZE;
+ goto unlock;
+ }
+
+ sg_mark_end(sgl->sg + sgl->cur - 1);
+
+ akcipher_request_set_crypt(&ctx->req, sgl->sg, ctx->rsgl[0].sg, used,
+ usedpages);
+ switch (ctx->op) {
+ case ALG_OP_VERIFY:
+ err = crypto_akcipher_verify(&ctx->req);
+ break;
+ case ALG_OP_SIGN:
+ err = crypto_akcipher_sign(&ctx->req);
+ break;
+ case ALG_OP_ENCRYPT:
+ err = crypto_akcipher_encrypt(&ctx->req);
+ break;
+ case ALG_OP_DECRYPT:
+ err = crypto_akcipher_decrypt(&ctx->req);
+ break;
+ default:
+ err = -EFAULT;
+ goto unlock;
+ }
+
+ err = af_alg_wait_for_completion(err, &ctx->completion);
+
+ if (err) {
+ /* EBADMSG implies a valid cipher operation took place */
+ if (err == -EBADMSG)
+ akcipher_put_sgl(sk);
+ goto unlock;
+ }
+
+ akcipher_put_sgl(sk);
+
+unlock:
+ for (i = 0; i < cnt; i++)
+ af_alg_free_sg(&ctx->rsgl[i]);
+
+ akcipher_wmem_wakeup(sk);
+ release_sock(sk);
+
+ return err ? err : ctx->req.dst_len;
+}
+
+static unsigned int akcipher_poll(struct file *file, struct socket *sock,
+ poll_table *wait)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct akcipher_ctx *ctx = ask->private;
+ unsigned int mask = 0;
+
+ sock_poll_wait(file, sk_sleep(sk), wait);
+
+ if (!ctx->more)
+ mask |= POLLIN | POLLRDNORM;
+
+ if (akcipher_writable(sk))
+ mask |= POLLOUT | POLLWRNORM | POLLWRBAND;
+
+ return mask;
+}
+
+static struct proto_ops algif_akcipher_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,
+
+ .release = af_alg_release,
+ .sendmsg = akcipher_sendmsg,
+ .sendpage = akcipher_sendpage,
+ .recvmsg = akcipher_recvmsg,
+ .poll = akcipher_poll,
+};
+
+static void *akcipher_bind(const char *name, u32 type, u32 mask)
+{
+ return crypto_alloc_akcipher(name, type, mask);
+}
+
+static void akcipher_release(void *private)
+{
+ crypto_free_akcipher(private);
+}
+
+static int akcipher_setprivkey(void *private, const u8 *key,
+ unsigned int keylen)
+{
+ return crypto_akcipher_set_priv_key(private, key, keylen);
+}
+
+static int akcipher_setpubkey(void *private, const u8 *key, unsigned int keylen)
+{
+ return crypto_akcipher_set_pub_key(private, key, keylen);
+}
+
+static void akcipher_sock_destruct(struct sock *sk)
+{
+ struct alg_sock *ask = alg_sk(sk);
+ struct akcipher_ctx *ctx = ask->private;
+
+ akcipher_put_sgl(sk);
+ sock_kfree_s(sk, ctx, ctx->len);
+ af_alg_release_parent(sk);
+}
+
+static int akcipher_accept_parent(void *private, struct sock *sk)
+{
+ struct akcipher_ctx *ctx;
+ struct alg_sock *ask = alg_sk(sk);
+ unsigned int len = sizeof(*ctx) + crypto_akcipher_reqsize(private);
+
+ ctx = sock_kmalloc(sk, len, GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+ memset(ctx, 0, len);
+
+ ctx->len = len;
+ ctx->used = 0;
+ ctx->more = 0;
+ ctx->merge = 0;
+ ctx->op = 0;
+ ctx->tsgl.cur = 0;
+ af_alg_init_completion(&ctx->completion);
+ sg_init_table(ctx->tsgl.sg, ALG_MAX_PAGES);
+
+ ask->private = ctx;
+
+ akcipher_request_set_tfm(&ctx->req, private);
+ akcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+ af_alg_complete, &ctx->completion);
+
+ sk->sk_destruct = akcipher_sock_destruct;
+
+ return 0;
+}
+
+static const struct af_alg_type algif_type_akcipher = {
+ .bind = akcipher_bind,
+ .release = akcipher_release,
+ .setkey = akcipher_setprivkey,
+ .setpubkey = akcipher_setpubkey,
+ .accept = akcipher_accept_parent,
+ .ops = &algif_akcipher_ops,
+ .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);
+
+ WARN_ON(err);
+}
+
+module_init(algif_akcipher_init);
+module_exit(algif_akcipher_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Stephan Mueller <[email protected]>");
+MODULE_DESCRIPTION("Asymmetric kernel crypto API user space interface");

2016-05-15 04:18:13

[permalink] [raw]

Subject: [PATCH v6 2/6] crypto: AF_ALG -- add setpubkey setsockopt call

From: Stephan Mueller <[email protected]>

For supporting asymmetric ciphers, user space must be able to set the
public key. The patch adds a new setsockopt call for setting the public
key.

Signed-off-by: Stephan Mueller <[email protected]>
---
crypto/af_alg.c | 18 +++++++++++++-----
include/crypto/if_alg.h | 1 +
2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index f5e18c2..24dc082 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -202,13 +202,17 @@ unlock:
}

static int alg_setkey(struct sock *sk, char __user *ukey,
- unsigned int keylen)
+ unsigned int keylen,
+ int (*setkey)(void *private, const u8 *key,
+ unsigned int keylen))
{
struct alg_sock *ask = alg_sk(sk);
- const struct af_alg_type *type = ask->type;
u8 *key;
int err;

+ if (!setkey)
+ return -ENOPROTOOPT;
+
key = sock_kmalloc(sk, keylen, GFP_KERNEL);
if (!key)
return -ENOMEM;
@@ -217,7 +221,7 @@ static int alg_setkey(struct sock *sk, char __user *ukey,
if (copy_from_user(key, ukey, keylen))
goto out;

- err = type->setkey(ask->private, key, keylen);
+ err = setkey(ask->private, key, keylen);

out:
sock_kzfree_s(sk, key, keylen);
@@ -247,10 +251,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
case ALG_SET_KEY:
if (sock->state == SS_CONNECTED)
goto unlock;
- if (!type->setkey)
+
+ err = alg_setkey(sk, optval, optlen, type->setkey);
+ break;
+ case ALG_SET_PUBKEY:
+ if (sock->state == SS_CONNECTED)
goto unlock;

- err = alg_setkey(sk, optval, optlen);
+ err = alg_setkey(sk, optval, optlen, type->setpubkey);
break;
case ALG_SET_AEAD_AUTHSIZE:
if (sock->state == SS_CONNECTED)
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index a2bfd78..6c3e6e7 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -52,6 +52,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setpubkey)(void *private, const u8 *key, unsigned int keylen);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);

2016-05-15 11:59:14

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] crypto: algif - add akcipher

Am Samstag, 14. Mai 2016, 21:16:45 schrieb Tadeusz Struk:

Hi Tadeusz,

> First four patches are a resend of the v3 algif_akcipher from
> Stephan Mueller, with minor changes after rebase on top of 4.6-rc1.

All four patches from me:

Acked-by: Stephan Mueller <[email protected]>

Ciao
Stephan

2016-05-16 20:47:13

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] crypto: algif - add akcipher

On 05/14/2016 09:16 PM, Tadeusz Struk wrote:
> First four patches are a resend of the v3 algif_akcipher from
> Stephan Mueller, with minor changes after rebase on top of 4.6-rc1.
>
> The next three patches add support for keys stored in system
> keyring subsystem.
>
> First patch adds algif_akcipher nokey hadlers.
>
> Second patch adds generic sign, verify, encrypt, decrypt accessors
> functions to the asymmetric key type. These will be defined by
> asymmetric subtypes, similarly to how public_key currently defines
> the verify_signature function.
>
> Third patch adds support for ALG_SET_KEY_ID and ALG_SET_PUBKEY_ID
> commands to AF_ALG and setkeyid operation to the af_alg_type struct.
> If the keyid is used then the afalg layer acquires the key for the
> keyring subsystem and uses the new asymmetric accessor functions
> instead of akcipher api. The asymmetric subtypes can use akcipher
> api internally.
>
> This is the same v5 version as before rebased on top of
> http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-asym-keyctl
>
> v6 changes:
> - update to reflect changes in kernel_pkey_params struct
>
> v5 changes:
> - drop public key changes and use new version provided by David
>
> v4 changes:
> - don't use internal public_key struct in af_alg.
> - add generic accessor functions to asymmetric key type, which take
> the generic struct key type and resolve the specific subtype internally
>
> v3 changes:
> - include Stephan's patches (rebased on 4.6-rc1)
> - add algif_akcipher nokey hadlers
> - add public_key info struct to public_key and helper query functions
> - add a check if a key is a software accessible key on af_alg, and
> return -ENOKEY if it isn't
>
> v2 changes:
> - pass the original skcipher request in ablkcipher.base.data instead of
> casting it back from the ablkcipher request.
> - rename _req to base_req
> - dropped 3/3
>
> ---
>
> Stephan Mueller (4):
> crypto: AF_ALG -- add sign/verify API
> crypto: AF_ALG -- add setpubkey setsockopt call
> crypto: AF_ALG -- add asymmetric cipher interface
> crypto: algif_akcipher - enable compilation
>
> Tadeusz Struk (2):
> crypto: algif_akcipher - add ops_nokey
> crypto: AF_ALG - add support for key_id
>
> crypto/Kconfig | 9
> crypto/Makefile | 1
> crypto/af_alg.c | 28 +
> crypto/algif_akcipher.c | 884 +++++++++++++++++++++++++++++++++++++++++++
> include/crypto/if_alg.h | 2
> include/uapi/linux/if_alg.h | 5
> 6 files changed, 924 insertions(+), 5 deletions(-)
> create mode 100644 crypto/algif_akcipher.c

Hi David,
Are you ok to take it into 4.7 via your linux-fs?
Thanks,
--
TS

2016-05-26 00:45:48

[permalink] [raw]

Subject: Re: [PATCH v6 6/6] crypto: AF_ALG - add support for key_id

On Sat, 14 May 2016, Tadeusz Struk wrote:

> diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
> index e00793d..6733df1 100644
> --- a/crypto/algif_akcipher.c
> +++ b/crypto/algif_akcipher.c
> +static int asym_key_verify(const struct key *key, struct akcipher_request *req)
> +{
> + struct public_key_signature sig;
> + char *src = NULL, *in;
> + int ret;
> +
> + if (!sg_is_last(req->src)) {
> + src = kmalloc(req->src_len, GFP_KERNEL);
> + if (!src)
> + return -ENOMEM;
> + scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
> + in = src;
> + } else {
> + in = sg_virt(req->src);
> + }
> + sig.pkey_algo = "rsa";
> + sig.encoding = "pkcs1";
> + /* Need to find a way to pass the hash param */

Are you referring to sig.digest here? It looks like you will hit a
BUG_ON() in public_key_verify_signature() if sig.digest is 0. However,
sig.digest is unlikely to be 0 because the struct is not cleared - should
fix this, since public_key_verify_signature() will try to follow that
random pointer.

> + sig.hash_algo = "sha1";
> + sig.digest_size = 20;
> + sig.s_size = req->src_len;
> + sig.s = src;
> + ret = verify_signature(key, NULL, &sig);

Is the idea to write the signature to the socket, and then read out the
expected digest (the digest comparison being done elsewhere)? Is that
something that will be supported by a future hardware asymmetric key
subtype?

verify_signature() ends up calling public_key_verify_signature(), which
currently expects to get both the digest and signature as input and
returns an error if verification fails. The output of
crypto_akcipher_verify() is discarded before public_key_verify_signature()
returns so nothing ends up in req->dst to read from the socket.

ALG_OP_VERIFY should behave the same whether using ALG_SET_PUBKEY or
ALG_SET_PUBKEY_ID, and they aren't right now.

If sig.digest is 0, verify_signature() could return the expected digest in
the sig structure and skip the digest comparison it currently does. Then
that data could be packaged up in req as if crypto_akcipher_verify() had
been called. I don't know if this change confuses the semantics of
verify_signature() too much, maybe a new function is required with all the
requisite plumbing to the asymmetric key subtype.

2016-05-31 17:44:03

[permalink] [raw]

Subject: Re: [PATCH v6 6/6] crypto: AF_ALG - add support for key_id

Hi Mat,
On 05/25/2016 05:45 PM, Mat Martineau wrote:
>
> On Sat, 14 May 2016, Tadeusz Struk wrote:
>
>> diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
>> index e00793d..6733df1 100644
>> --- a/crypto/algif_akcipher.c
>> +++ b/crypto/algif_akcipher.c
>> +static int asym_key_verify(const struct key *key, struct akcipher_request *req)
>> +{
>> + struct public_key_signature sig;
>> + char *src = NULL, *in;
>> + int ret;
>> +
>> + if (!sg_is_last(req->src)) {
>> + src = kmalloc(req->src_len, GFP_KERNEL);
>> + if (!src)
>> + return -ENOMEM;
>> + scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
>> + in = src;
>> + } else {
>> + in = sg_virt(req->src);
>> + }
>> + sig.pkey_algo = "rsa";
>> + sig.encoding = "pkcs1";
>> + /* Need to find a way to pass the hash param */
>
> Are you referring to sig.digest here? It looks like you will hit a BUG_ON() in public_key_verify_signature() if sig.digest is 0. However, sig.digest is unlikely to be 0 because the struct is not cleared - should fix this, since public_key_verify_signature() will try to follow that random pointer.
>

Right, I need to have a local buffer for the digest here.

>> + sig.hash_algo = "sha1";
>> + sig.digest_size = 20;
>> + sig.s_size = req->src_len;
>> + sig.s = src;
>> + ret = verify_signature(key, NULL, &sig);
>
> Is the idea to write the signature to the socket, and then read out the expected digest (the digest comparison being done elsewhere)? Is that something that will be supported by a future hardware asymmetric key subtype?

After the verify operation the output will be copied to the user,
and the user needs to verify it.

>
> verify_signature() ends up calling public_key_verify_signature(), which currently expects to get both the digest and signature as input and returns an error if verification fails. The output of crypto_akcipher_verify() is discarded before public_key_verify_signature() returns so nothing ends up in req->dst to read from the socket.
>
> ALG_OP_VERIFY should behave the same whether using ALG_SET_PUBKEY or ALG_SET_PUBKEY_ID, and they aren't right now.
>
> If sig.digest is 0, verify_signature() could return the expected digest in the sig structure and skip the digest comparison it currently does. Then that data could be packaged up in req as if crypto_akcipher_verify() had been called. I don't know if this change confuses the semantics of verify_signature() too much, maybe a new function is required with all the requisite plumbing to the asymmetric key subtype.
>

We need to copy output to the user to verify because we don't have it.
That will be consistent for both ALG_SET_PUBKEY and ALG_SET_PUBKEY_ID.
Thanks for your comments and sorry for the delayed response. I'll will send v7 soon.
--
TS

2016-06-08 00:28:10

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Stephan,

On Sat, 14 May 2016, Tadeusz Struk wrote:

> From: Stephan Mueller <[email protected]>
>
> This patch adds the user space interface for asymmetric ciphers. The
> interface allows the use of sendmsg as well as vmsplice to provide data.
>
> This version has been rebased on top of 4.6 and a few chackpatch issues
> have been fixed.
>
> Signed-off-by: Stephan Mueller <[email protected]>
> Signed-off-by: Tadeusz Struk <[email protected]>
> ---
> diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
> new file mode 100644
> index 0000000..6342b6e
> --- /dev/null
> +++ b/crypto/algif_akcipher.c
> +
> +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 akcipher_ctx *ctx = ask->private;
> + struct akcipher_sg_list *sgl = &ctx->tsgl;
> + unsigned int i = 0;
> + int err;
> + unsigned long used = 0;
> + size_t usedpages = 0;
> + unsigned int cnt = 0;
> +
> + /* Limit number of IOV blocks to be accessed below */
> + if (msg->msg_iter.nr_segs > ALG_MAX_PAGES)
> + return -ENOMSG;
> +
> + lock_sock(sk);
> +
> + if (ctx->more) {
> + err = akcipher_wait_for_data(sk, flags);
> + if (err)
> + goto unlock;
> + }
> +
> + used = ctx->used;
> +
> + /* convert iovecs of output buffers into scatterlists */
> + while (iov_iter_count(&msg->msg_iter)) {
> + /* make one iovec available as scatterlist */
> + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
> + iov_iter_count(&msg->msg_iter));
> + if (err < 0)
> + goto unlock;
> + usedpages += err;
> + /* chain the new scatterlist with previous one */
> + if (cnt)
> + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
> +
> + iov_iter_advance(&msg->msg_iter, err);
> + cnt++;
> + }
> +
> + /* ensure output buffer is sufficiently large */
> + if (usedpages < akcipher_calcsize(ctx)) {
> + err = -EMSGSIZE;
> + goto unlock;
> + }

Why is the size of the output buffer enforced here instead of depending on
the algorithm implementation?

Thanks,

Mat

> + sg_mark_end(sgl->sg + sgl->cur - 1);
> +
> + akcipher_request_set_crypt(&ctx->req, sgl->sg, ctx->rsgl[0].sg, used,
> + usedpages);
> + switch (ctx->op) {
> + case ALG_OP_VERIFY:
> + err = crypto_akcipher_verify(&ctx->req);
> + break;
> + case ALG_OP_SIGN:
> + err = crypto_akcipher_sign(&ctx->req);
> + break;
> + case ALG_OP_ENCRYPT:
> + err = crypto_akcipher_encrypt(&ctx->req);
> + break;
> + case ALG_OP_DECRYPT:
> + err = crypto_akcipher_decrypt(&ctx->req);
> + break;
> + default:
> + err = -EFAULT;
> + goto unlock;
> + }
> +
> + err = af_alg_wait_for_completion(err, &ctx->completion);
> +
> + if (err) {
> + /* EBADMSG implies a valid cipher operation took place */
> + if (err == -EBADMSG)
> + akcipher_put_sgl(sk);
> + goto unlock;
> + }
> +
> + akcipher_put_sgl(sk);
> +
> +unlock:
> + for (i = 0; i < cnt; i++)
> + af_alg_free_sg(&ctx->rsgl[i]);
> +
> + akcipher_wmem_wakeup(sk);
> + release_sock(sk);
> +
> + return err ? err : ctx->req.dst_len;
> +}

--
Mat Martineau
Intel OTC

2016-06-08 05:31:19

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau:

Hi Mat,

> > + used = ctx->used;
> > +
> > + /* convert iovecs of output buffers into scatterlists */
> > + while (iov_iter_count(&msg->msg_iter)) {
> > + /* make one iovec available as scatterlist */
> > + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
> > + iov_iter_count(&msg->msg_iter));
> > + if (err < 0)
> > + goto unlock;
> > + usedpages += err;
> > + /* chain the new scatterlist with previous one */
> > + if (cnt)
> > + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
> > +
> > + iov_iter_advance(&msg->msg_iter, err);
> > + cnt++;
> > + }
> > +
> > + /* ensure output buffer is sufficiently large */
> > + if (usedpages < akcipher_calcsize(ctx)) {
> > + err = -EMSGSIZE;
> > + goto unlock;
> > + }
>
> Why is the size of the output buffer enforced here instead of depending on
> the algorithm implementation?

akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size the
algorithm generates as output during its operation.

The code ensures that the caller provided at least that amount of memory for
the kernel to store its data in. This check therefore is present to ensure the
kernel does not overstep memory boundaries in user space.

What is your concern?

Thanks

Ciao
Stephan

2016-06-08 19:14:49

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

On Wed, 8 Jun 2016, Stephan Mueller wrote:

> Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau:
>
> Hi Mat,
>
>>> + used = ctx->used;
>>> +
>>> + /* convert iovecs of output buffers into scatterlists */
>>> + while (iov_iter_count(&msg->msg_iter)) {
>>> + /* make one iovec available as scatterlist */
>>> + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
>>> + iov_iter_count(&msg->msg_iter));
>>> + if (err < 0)
>>> + goto unlock;
>>> + usedpages += err;
>>> + /* chain the new scatterlist with previous one */
>>> + if (cnt)
>>> + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
>>> +
>>> + iov_iter_advance(&msg->msg_iter, err);
>>> + cnt++;
>>> + }
>>> +
>>> + /* ensure output buffer is sufficiently large */
>>> + if (usedpages < akcipher_calcsize(ctx)) {
>>> + err = -EMSGSIZE;
>>> + goto unlock;
>>> + }
>>
>> Why is the size of the output buffer enforced here instead of depending on
>> the algorithm implementation?
>
> akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size the
> algorithm generates as output during its operation.
>
> The code ensures that the caller provided at least that amount of memory for
> the kernel to store its data in. This check therefore is present to ensure the
> kernel does not overstep memory boundaries in user space.

Yes, it's understood that the userspace buffer length must not be
exceeded. But dst_len is part of the akcipher_request struct, so why does
it need to be checked *here* when it is also checked later?

> What is your concern?

Userspace must allocate larger buffers than it knows are necessary for
expected results.

It looks like the software rsa implementation handles shorter output
buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is
too small), however I see at least one hardware rsa driver that requires
the output buffer to be the maximum size. But this inconsistency might be
best addressed within the software cipher or drivers rather than in
recvmsg.

2016-06-09 09:28:59

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Mittwoch, 8. Juni 2016, 12:14:49 schrieb Mat Martineau:

Hi Mat,

> On Wed, 8 Jun 2016, Stephan Mueller wrote:
> > Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau:
> >
> > Hi Mat,
> >
> >>> + used = ctx->used;
> >>> +
> >>> + /* convert iovecs of output buffers into scatterlists */
> >>> + while (iov_iter_count(&msg->msg_iter)) {
> >>> + /* make one iovec available as scatterlist */
> >>> + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
> >>> + iov_iter_count(&msg->msg_iter));
> >>> + if (err < 0)
> >>> + goto unlock;
> >>> + usedpages += err;
> >>> + /* chain the new scatterlist with previous one */
> >>> + if (cnt)
> >>> + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
> >>> +
> >>> + iov_iter_advance(&msg->msg_iter, err);
> >>> + cnt++;
> >>> + }
> >>> +
> >>> + /* ensure output buffer is sufficiently large */
> >>> + if (usedpages < akcipher_calcsize(ctx)) {
> >>> + err = -EMSGSIZE;
> >>> + goto unlock;
> >>> + }
> >>
> >> Why is the size of the output buffer enforced here instead of depending
> >> on
> >> the algorithm implementation?
> >
> > akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size
> > the
> > algorithm generates as output during its operation.
> >
> > The code ensures that the caller provided at least that amount of memory
> > for the kernel to store its data in. This check therefore is present to
> > ensure the kernel does not overstep memory boundaries in user space.
>
> Yes, it's understood that the userspace buffer length must not be
> exceeded. But dst_len is part of the akcipher_request struct, so why does
> it need to be checked *here* when it is also checked later?

I am always uneasy when the kernel has a user space interface and expects
layers deep down inside the kernel to check for user space related boundaries.
Note, we do not hand the __user flag down, so sparse and other tools cannot
detect whether a particular cipher implementation has the right checks.

I therefore always would like to check parameters at the interface handling
logic. Cryptographers rightly should worry about their code implementing the
cipher correctly. But I do not think that the cipher implementations should
worry about security implications since they may be called from user space.
>
> > What is your concern?
>
> Userspace must allocate larger buffers than it knows are necessary for
> expected results.
>
> It looks like the software rsa implementation handles shorter output
> buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is
> too small), however I see at least one hardware rsa driver that requires
> the output buffer to be the maximum size. But this inconsistency might be
> best addressed within the software cipher or drivers rather than in
> recvmsg.

Is your concern that we have a double check check for lengths here? If yes, I
think we can live with an additional if() here.

Or is your concern that the user space interface restricts things too much and
thus prevents a valid use case?

Ciao
Stephan

2016-06-09 18:18:23

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

On Thu, 9 Jun 2016, Stephan Mueller wrote:

> Am Mittwoch, 8. Juni 2016, 12:14:49 schrieb Mat Martineau:
>
> Hi Mat,
>
>> On Wed, 8 Jun 2016, Stephan Mueller wrote:
>>> Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau:
>>>
>>> Hi Mat,
>>>
>>>>> + used = ctx->used;
>>>>> +
>>>>> + /* convert iovecs of output buffers into scatterlists */
>>>>> + while (iov_iter_count(&msg->msg_iter)) {
>>>>> + /* make one iovec available as scatterlist */
>>>>> + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
>>>>> + iov_iter_count(&msg->msg_iter));
>>>>> + if (err < 0)
>>>>> + goto unlock;
>>>>> + usedpages += err;
>>>>> + /* chain the new scatterlist with previous one */
>>>>> + if (cnt)
>>>>> + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
>>>>> +
>>>>> + iov_iter_advance(&msg->msg_iter, err);
>>>>> + cnt++;
>>>>> + }
>>>>> +
>>>>> + /* ensure output buffer is sufficiently large */
>>>>> + if (usedpages < akcipher_calcsize(ctx)) {
>>>>> + err = -EMSGSIZE;
>>>>> + goto unlock;
>>>>> + }
>>>>
>>>> Why is the size of the output buffer enforced here instead of
>>>> depending on the algorithm implementation?
>>>
>>> akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum
>>> size the algorithm generates as output during its operation.
>>>
>>> The code ensures that the caller provided at least that amount of memory
>>> for the kernel to store its data in. This check therefore is present to
>>> ensure the kernel does not overstep memory boundaries in user space.
>>
>> Yes, it's understood that the userspace buffer length must not be
>> exceeded. But dst_len is part of the akcipher_request struct, so why does
>> it need to be checked *here* when it is also checked later?
>
> I am always uneasy when the kernel has a user space interface and expects
> layers deep down inside the kernel to check for user space related boundaries.
> Note, we do not hand the __user flag down, so sparse and other tools cannot
> detect whether a particular cipher implementation has the right checks.
>
> I therefore always would like to check parameters at the interface handling
> logic. Cryptographers rightly should worry about their code implementing the
> cipher correctly. But I do not think that the cipher implementations should
> worry about security implications since they may be called from user space.

Userspace or not, buffer lengths need to be strictly checked.

>>
>>> What is your concern?
>>
>> Userspace must allocate larger buffers than it knows are necessary for
>> expected results.
>>
>> It looks like the software rsa implementation handles shorter output
>> buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is
>> too small), however I see at least one hardware rsa driver that requires
>> the output buffer to be the maximum size. But this inconsistency might be
>> best addressed within the software cipher or drivers rather than in
>> recvmsg.
>
> Is your concern that we have a double check check for lengths here? If yes, I
> think we can live with an additional if() here.
>
> Or is your concern that the user space interface restricts things too much and
> thus prevents a valid use case?

The latter - my primary concern is the constraint this places on userspace
by forcing larger buffer sizes than might be necessary for the operation.
struct akcipher_request has separate members for src_len and dst_len, and
dst_len is documented as needing "to be at least as big as the expected
result depending on the operation". Not the maximum result, the expected
result. It's also documented that the cipher will generate an error if
dst_len is insufficient and update the value with the required size.

I'm updating some userspace TLS code that worked with an earlier, unmerged
patch set for AF_ALG akcipher (from last year). The read calls with
shorter buffers were the main porting problem.

--
Mat Martineau
Intel OTC

2016-06-09 18:24:17

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau:

Hi Mat,

> > Or is your concern that the user space interface restricts things too much
> > and thus prevents a valid use case?
>
> The latter - my primary concern is the constraint this places on userspace
> by forcing larger buffer sizes than might be necessary for the operation.
> struct akcipher_request has separate members for src_len and dst_len, and
> dst_len is documented as needing "to be at least as big as the expected
> result depending on the operation". Not the maximum result, the expected
> result. It's also documented that the cipher will generate an error if
> dst_len is insufficient and update the value with the required size.
>
> I'm updating some userspace TLS code that worked with an earlier, unmerged
> patch set for AF_ALG akcipher (from last year). The read calls with
> shorter buffers were the main porting problem.

I see -- are you proposing to drop that check entirely?

Ciao
Stephan

2016-06-09 18:27:16

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

On Thu, 9 Jun 2016, Stephan Mueller wrote:

> Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau:
>
> Hi Mat,
>
>>> Or is your concern that the user space interface restricts things too much
>>> and thus prevents a valid use case?
>>
>> The latter - my primary concern is the constraint this places on userspace
>> by forcing larger buffer sizes than might be necessary for the operation.
>> struct akcipher_request has separate members for src_len and dst_len, and
>> dst_len is documented as needing "to be at least as big as the expected
>> result depending on the operation". Not the maximum result, the expected
>> result. It's also documented that the cipher will generate an error if
>> dst_len is insufficient and update the value with the required size.
>>
>> I'm updating some userspace TLS code that worked with an earlier, unmerged
>> patch set for AF_ALG akcipher (from last year). The read calls with
>> shorter buffers were the main porting problem.
>
> I see -- are you proposing to drop that check entirely?

Yes.

Best regards,

--
Mat Martineau
Intel OTC

2016-06-09 18:36:29

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Donnerstag, 9. Juni 2016, 11:27:13 schrieb Mat Martineau:

Hi Mat, Tadeusz,

> On Thu, 9 Jun 2016, Stephan Mueller wrote:
> > Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau:
> >
> > Hi Mat,
> >
> >>> Or is your concern that the user space interface restricts things too
> >>> much
> >>> and thus prevents a valid use case?
> >>
> >> The latter - my primary concern is the constraint this places on
> >> userspace
> >> by forcing larger buffer sizes than might be necessary for the operation.
> >> struct akcipher_request has separate members for src_len and dst_len, and
> >> dst_len is documented as needing "to be at least as big as the expected
> >> result depending on the operation". Not the maximum result, the expected
> >> result. It's also documented that the cipher will generate an error if
> >> dst_len is insufficient and update the value with the required size.
> >>
> >> I'm updating some userspace TLS code that worked with an earlier,
> >> unmerged
> >> patch set for AF_ALG akcipher (from last year). The read calls with
> >> shorter buffers were the main porting problem.
> >
> > I see -- are you proposing to drop that check entirely?
>
> Yes.

Ok, after checking the code again, I think that dropping that sanity check
should be ok given that this length is part of the akcipher API.

Tadeusz, as you are currently managing that patch set, would you re-spin it
with the following check removed?

+ if (usedpages < akcipher_calcsize(ctx)) {
+ err = -EMSGSIZE;
+ goto unlock;
+ }

Ciao
Stephan

2016-06-10 14:42:00

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

On 06/09/2016 11:36 AM, Stephan Mueller wrote:
> Am Donnerstag, 9. Juni 2016, 11:27:13 schrieb Mat Martineau:
>
> Hi Mat, Tadeusz,
>
>> On Thu, 9 Jun 2016, Stephan Mueller wrote:
>>> Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau:
>>>
>>> Hi Mat,
>>>
>>>>> Or is your concern that the user space interface restricts things too
>>>>> much
>>>>> and thus prevents a valid use case?
>>>>
>>>> The latter - my primary concern is the constraint this places on
>>>> userspace
>>>> by forcing larger buffer sizes than might be necessary for the operation.
>>>> struct akcipher_request has separate members for src_len and dst_len, and
>>>> dst_len is documented as needing "to be at least as big as the expected
>>>> result depending on the operation". Not the maximum result, the expected
>>>> result. It's also documented that the cipher will generate an error if
>>>> dst_len is insufficient and update the value with the required size.
>>>>
>>>> I'm updating some userspace TLS code that worked with an earlier,
>>>> unmerged
>>>> patch set for AF_ALG akcipher (from last year). The read calls with
>>>> shorter buffers were the main porting problem.
>>>
>>> I see -- are you proposing to drop that check entirely?
>>
>> Yes.
>
> Ok, after checking the code again, I think that dropping that sanity check
> should be ok given that this length is part of the akcipher API.
>
> Tadeusz, as you are currently managing that patch set, would you re-spin it
> with the following check removed?
>
> + if (usedpages < akcipher_calcsize(ctx)) {
> + err = -EMSGSIZE;
> + goto unlock;
> + }
>

Ok, I'll update the patch.
Thanks,
--
TS

2016-06-13 22:16:13

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Hi,

On 8 June 2016 at 21:14, Mat Martineau
<[email protected]> wrote:
> On Wed, 8 Jun 2016, Stephan Mueller wrote:
>> What is your concern?
> Userspace must allocate larger buffers than it knows are necessary for
> expected results.
>
> It looks like the software rsa implementation handles shorter output buffers
> ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is too small),
> however I see at least one hardware rsa driver that requires the output
> buffer to be the maximum size. But this inconsistency might be best
> addressed within the software cipher or drivers rather than in recvmsg.

Should the hardware drivers fix this instead? I've looked at the qat
and caam drivers, they both require the destination buffer size to be
the key size and in both cases there would be no penalty for dropping
this requirement as far as I see. Both do a memmove if the result
ends up being shorter than key size. In case the caller knows it is
expecting a specific output size, the driver will have to use a self
allocated buffer + a memcpy in those same cases where it would later
use memmove instead. Alternatively the sg passed to dma_map_sg can be
prepended with a dummy segment the right size to save the memcpy.

akcipher.h only says:
@dst_len: Size of the output buffer. It needs to be at least as big as
the expected result depending on the operation

Note that for random input data the memmove will be done about 1 in
256 times but with PKCS#1 padding the signature always has a leading
zero.

Requiring buffers bigger than needed makes the added work of dropping
the zero bytes from the sglist and potentially re-adding them in the
client difficult to justify. RSA doing this sets a precedent for a
future pkcs1pad (or other algorithm) implementation to do the same
thing and a portable client having to always know the key size and use
key-sized buffers.

Best regards

2016-06-14 05:12:28

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Dienstag, 14. Juni 2016, 00:16:11 schrieb Andrew Zaborowski:

Hi Andrew,

> Hi,
>
> On 8 June 2016 at 21:14, Mat Martineau
>
> <[email protected]> wrote:
> > On Wed, 8 Jun 2016, Stephan Mueller wrote:
> >> What is your concern?
> >
> > Userspace must allocate larger buffers than it knows are necessary for
> > expected results.
> >
> > It looks like the software rsa implementation handles shorter output
> > buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is
> > too small), however I see at least one hardware rsa driver that requires
> > the output buffer to be the maximum size. But this inconsistency might be
> > best addressed within the software cipher or drivers rather than in
> > recvmsg.
> Should the hardware drivers fix this instead? I've looked at the qat
> and caam drivers, they both require the destination buffer size to be
> the key size and in both cases there would be no penalty for dropping
> this requirement as far as I see. Both do a memmove if the result
> ends up being shorter than key size. In case the caller knows it is
> expecting a specific output size, the driver will have to use a self
> allocated buffer + a memcpy in those same cases where it would later
> use memmove instead. Alternatively the sg passed to dma_map_sg can be
> prepended with a dummy segment the right size to save the memcpy.
>
> akcipher.h only says:
> @dst_len: Size of the output buffer. It needs to be at least as big as
> the expected result depending on the operation
>
> Note that for random input data the memmove will be done about 1 in
> 256 times but with PKCS#1 padding the signature always has a leading
> zero.
>
> Requiring buffers bigger than needed makes the added work of dropping
> the zero bytes from the sglist and potentially re-adding them in the
> client difficult to justify. RSA doing this sets a precedent for a
> future pkcs1pad (or other algorithm) implementation to do the same
> thing and a portable client having to always know the key size and use
> key-sized buffers.

I think we have agreed on dropping the length enforcement at the interface
level.

Ciao
Stephan

2016-06-14 07:42:34

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Hi Stephan,

On 14 June 2016 at 07:12, Stephan Mueller <[email protected]> wrote:
> Am Dienstag, 14. Juni 2016, 00:16:11 schrieb Andrew Zaborowski:
>> On 8 June 2016 at 21:14, Mat Martineau
>>
>> <mathew.j.martineau-VuQAYsv1563Yd54FQh9/[email protected]> wrote:
>> > On Wed, 8 Jun 2016, Stephan Mueller wrote:
>> >> What is your concern?
>> >
>> > Userspace must allocate larger buffers than it knows are necessary for
>> > expected results.
>> >
>> > It looks like the software rsa implementation handles shorter output
>> > buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is
>> > too small), however I see at least one hardware rsa driver that requires
>> > the output buffer to be the maximum size. But this inconsistency might be
>> > best addressed within the software cipher or drivers rather than in
>> > recvmsg.
>> Should the hardware drivers fix this instead? I've looked at the qat
>> and caam drivers, they both require the destination buffer size to be
>> the key size and in both cases there would be no penalty for dropping
>> this requirement as far as I see. Both do a memmove if the result
>> ends up being shorter than key size. In case the caller knows it is
>> expecting a specific output size, the driver will have to use a self
>> allocated buffer + a memcpy in those same cases where it would later
>> use memmove instead. Alternatively the sg passed to dma_map_sg can be
>> prepended with a dummy segment the right size to save the memcpy.
>>
>> akcipher.h only says:
>> @dst_len: Size of the output buffer. It needs to be at least as big as
>> the expected result depending on the operation
>>
>> Note that for random input data the memmove will be done about 1 in
>> 256 times but with PKCS#1 padding the signature always has a leading
>> zero.
>>
>> Requiring buffers bigger than needed makes the added work of dropping
>> the zero bytes from the sglist and potentially re-adding them in the
>> client difficult to justify. RSA doing this sets a precedent for a
>> future pkcs1pad (or other algorithm) implementation to do the same
>> thing and a portable client having to always know the key size and use
>> key-sized buffers.
>
> I think we have agreed on dropping the length enforcement at the interface
> level.

Separately from this there's a problem with the user being unable to
know if the algorithm is going to fail because of destination buffer
size != key size (including kernel users). For RSA, the qat
implementation will fail while the software implementation won't. For
pkcs1pad(...) there's currently just one implementation but the user
can't assume that.

Best regards

2016-06-14 17:22:15

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

2016-06-15 07:04:35

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Dienstag, 14. Juni 2016, 10:22:15 schrieb Mat Martineau:

Hi Mat,

> Stephan,
>
> On Sat, 14 May 2016, Tadeusz Struk wrote:
> > From: Stephan Mueller <[email protected]>
> >
> > This patch adds the user space interface for asymmetric ciphers. The
> > interface allows the use of sendmsg as well as vmsplice to provide data.
> >
> > This version has been rebased on top of 4.6 and a few chackpatch issues
> > have been fixed.
> >
> > Signed-off-by: Stephan Mueller <[email protected]>
> > Signed-off-by: Tadeusz Struk <[email protected]>
> > ---
> > crypto/algif_akcipher.c | 542
> > +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 542
> > 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 0000000..6342b6e
> > --- /dev/null
> > +++ b/crypto/algif_akcipher.c
> > +
> > +static int akcipher_sendmsg(struct socket *sock, struct msghdr *msg,
> > + size_t size)
> > +{
> > + struct sock *sk = sock->sk;
> > + struct alg_sock *ask = alg_sk(sk);
> > + struct akcipher_ctx *ctx = ask->private;
> > + struct akcipher_sg_list *sgl = &ctx->tsgl;
> > + struct af_alg_control con = {};
> > + long copied = 0;
> > + int op = 0;
> > + bool init = 0;
> > + int err;
> > +
> > + if (msg->msg_controllen) {
> > + err = af_alg_cmsg_send(msg, &con);
> > + if (err)
> > + return err;
> > +
> > + init = 1;
> > + switch (con.op) {
> > + case ALG_OP_VERIFY:
> > + case ALG_OP_SIGN:
> > + case ALG_OP_ENCRYPT:
> > + case ALG_OP_DECRYPT:
> > + op = con.op;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + lock_sock(sk);
> > + if (!ctx->more && ctx->used)
> > + goto unlock;
>
> err might be uninitialised at this goto. Should it be set to something
> like -EALREADY to indicate that data is already queued for a different
> crypto op?

Thanks for the hint. Tadeusz, I will provide you with an updated
algif_akcipher.c for your patchset.

I will also have a look at the comment from Andrew.

>
> <snip>
>
> > +unlock:
> > + akcipher_data_wakeup(sk);
> > + release_sock(sk);
> > +
> > + return err ?: copied;
> > +}
>
> Regards,
>
> --
> Mat Martineau
> Intel OTC

Ciao
Stephan

2016-06-16 08:05:52

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Dienstag, 14. Juni 2016, 09:42:34 schrieb Andrew Zaborowski:

Hi Andrew,

> >
> > I think we have agreed on dropping the length enforcement at the interface
> > level.
>
> Separately from this there's a problem with the user being unable to
> know if the algorithm is going to fail because of destination buffer
> size != key size (including kernel users). For RSA, the qat
> implementation will fail while the software implementation won't. For
> pkcs1pad(...) there's currently just one implementation but the user
> can't assume that.

If I understand your issue correctly, my initial code requiring the caller to
provide sufficient memory would have covered the issue, right? If so, we seem
to have implementations which can handle shorter buffer sizes and some which
do not. Should a caller really try to figure the right buffer size out? Why
not requiring a mandatory buffer size and be done with it? I.e. what is the
gain to allow shorter buffer sizes (as pointed out by Mat)? So, bottom line, I
am wondering whether we should keep the algif_akcipher code to require a
minimum buffer size.

If there is really a good argument to allow shorter buffers, then I guess we
need an in-kernel API call (which should be reported to user space) which
gives us the smallest usable buffer size. I guess that call would only be
valid after a setkey operation as the output size depends on the key size.
Instead of inventing a complete new API call, shouldn't the call
crypto_akcipher_maxsize() be converted for this purpose? I requested that API
call during the time the akcipher API was developed explicitly for getting the
minimum buffer size the caller needs to provide.

Ciao
Stephan

2016-06-16 14:59:03

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Hi Stephan,

On 16 June 2016 at 10:05, Stephan Mueller <[email protected]> wrote:
> Am Dienstag, 14. Juni 2016, 09:42:34 schrieb Andrew Zaborowski:
>
> Hi Andrew,
>
>> >
>> > I think we have agreed on dropping the length enforcement at the interface
>> > level.
>>
>> Separately from this there's a problem with the user being unable to
>> know if the algorithm is going to fail because of destination buffer
>> size != key size (including kernel users). For RSA, the qat
>> implementation will fail while the software implementation won't. For
>> pkcs1pad(...) there's currently just one implementation but the user
>> can't assume that.
>
> If I understand your issue correctly, my initial code requiring the caller to
> provide sufficient memory would have covered the issue, right?

This isn't an issue with AF_ALG, I should have changed the subject
line perhaps. In this case it's an inconsistency between some
implementations and the documentation (header comment). It affects
users accessing the cipher through AF_ALG but also directly.

> If so, we seem
> to have implementations which can handle shorter buffer sizes and some which
> do not. Should a caller really try to figure the right buffer size out? Why
> not requiring a mandatory buffer size and be done with it? I.e. what is the
> gain to allow shorter buffer sizes (as pointed out by Mat)?

It's that client code doesn't need an intermediate layer with an
additional buffer and a memcpy to provide a sensible API. If the code
wants to decrypt a 32-byte Digest Info structure with a given key or a
reference to a key it makes no sense, logically or in terms of
performance, for it to provide a key-sized buffer.

In the case of the userspace interface I think it's also rare for a
recv() or read() on Linux to require a buffer larger than it's going
to use, correct me if i'm wrong. (I.e. fail if given a 32-byte
buffer, return 32 bytes of data anyway) Turning your questino around
is there a gain from requiring larger buffers?

Best regards

2016-06-16 15:38:14

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Donnerstag, 16. Juni 2016, 16:59:01 schrieb Andrew Zaborowski:

Hi Andrew,

> Hi Stephan,
>
> On 16 June 2016 at 10:05, Stephan Mueller <[email protected]> wrote:
> > Am Dienstag, 14. Juni 2016, 09:42:34 schrieb Andrew Zaborowski:
> >
> > Hi Andrew,
> >
> >> > I think we have agreed on dropping the length enforcement at the
> >> > interface
> >> > level.
> >>
> >> Separately from this there's a problem with the user being unable to
> >> know if the algorithm is going to fail because of destination buffer
> >> size != key size (including kernel users). For RSA, the qat
> >> implementation will fail while the software implementation won't. For
> >> pkcs1pad(...) there's currently just one implementation but the user
> >> can't assume that.
> >
> > If I understand your issue correctly, my initial code requiring the caller
> > to provide sufficient memory would have covered the issue, right?
>
> This isn't an issue with AF_ALG, I should have changed the subject
> line perhaps. In this case it's an inconsistency between some
> implementations and the documentation (header comment). It affects
> users accessing the cipher through AF_ALG but also directly.

As I want to send a new version of the algif_akcipher shortly now (hoping for
an inclusion into 4.8), is there anything you see that I should prepare for
regarding this issue? I.e. do you forsee a potential fix that would change the
API or ABI of algif_akcipher?
>
> > If so, we seem
> > to have implementations which can handle shorter buffer sizes and some
> > which do not. Should a caller really try to figure the right buffer size
> > out? Why not requiring a mandatory buffer size and be done with it? I.e.
> > what is the gain to allow shorter buffer sizes (as pointed out by Mat)?
>
> It's that client code doesn't need an intermediate layer with an
> additional buffer and a memcpy to provide a sensible API. If the code
> wants to decrypt a 32-byte Digest Info structure with a given key or a
> reference to a key it makes no sense, logically or in terms of
> performance, for it to provide a key-sized buffer.
>
> In the case of the userspace interface I think it's also rare for a
> recv() or read() on Linux to require a buffer larger than it's going
> to use, correct me if i'm wrong. (I.e. fail if given a 32-byte
> buffer, return 32 bytes of data anyway) Turning your questino around
> is there a gain from requiring larger buffers?

That is a good one :-)

I have that check removed.

Ciao
Stephan

2016-06-17 00:39:39

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Hi Stephan,

On 16 June 2016 at 17:38, Stephan Mueller <[email protected]> wrote:
>> This isn't an issue with AF_ALG, I should have changed the subject
>> line perhaps. In this case it's an inconsistency between some
>> implementations and the documentation (header comment). It affects
>> users accessing the cipher through AF_ALG but also directly.
>
> As I want to send a new version of the algif_akcipher shortly now (hoping for
> an inclusion into 4.8), is there anything you see that I should prepare for
> regarding this issue? I.e. do you forsee a potential fix that would change the
> API or ABI of algif_akcipher?

No, as far as I understand algif_akcipher will do the right thing now
if the algorithm does the right thing. It's only the two RSA drivers
that would need to align with the software RSA in what buffer length
they accept.

Best regards

2016-06-22 22:46:45

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Stephan and Tadeusz,

On Fri, 10 Jun 2016, Tadeusz Struk wrote:

> On 06/09/2016 11:36 AM, Stephan Mueller wrote:
>> Am Donnerstag, 9. Juni 2016, 11:27:13 schrieb Mat Martineau:
>>
>> Hi Mat, Tadeusz,
>>
>> Ok, after checking the code again, I think that dropping that sanity check
>> should be ok given that this length is part of the akcipher API.
>>
>> Tadeusz, as you are currently managing that patch set, would you re-spin it
>> with the following check removed?
>>
>> + if (usedpages < akcipher_calcsize(ctx)) {
>> + err = -EMSGSIZE;
>> + goto unlock;
>> + }
>>
>
> Ok, I'll update the patch.

Thanks, that helps (especially with pkcs1pad).

This brings me to another proposal for read buffer sizing: AF_ALG akcipher
can guarantee that partial reads (where the read buffer is shorter than
the output of the crypto op) will work using the same semantics as
SOCK_DGRAM/SOCK_SEQPACKET. With those sockets, as much data as will fit is
copied in to the read buffer and the remainder is discarded.

I realize there's a performance and memory tradeoff, since the crypto
algorithm needs a sufficiently large output buffer that would have to be
created by AF_ALG akcipher. The user could manage that tradeoff by
providing a larger buffer (typically key_size?) if it wants to avoid
allocating and copying intermediate buffers inside the kernel.

--
Mat Martineau
Intel OTC

2016-06-23 05:07:05