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

[Stephan Mueller]

Am Montag, 21. August 2017, 11:23:55 CEST schrieb Tudor Ambarus:

Hi Tudor,

> 
> Oops, I missed the negation. When crypto_akcipher_set_priv_key succeeds
> you return the akcipher_maxsize. Not a bad idea, you save few cpu
> cycles.

I was hoping to save some context switches.
> 
> > crypto akcipher uses a dedicated function for determining the length of
> > the output buffer, crypto_akcipher_maxsize. Should we add a new function
> > pointer in struct af_alg_type that returns the maxsize?
> 
> Your API is different from crypto's akcipher. Should we make them
> identical?

In the early days of the akcipher API it used to be the way algif_akcipher 
implements it today.

Do you see a case where user space wants to deliberately ask for this value? 
As this value never changes after setting a key, I thought we can skip it for 
the user space interface.

Ciao
Stephan

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

[Tudor Ambarus]

On 08/21/2017 11:55 AM, Tudor Ambarus wrote:

Hi, Stephan,


+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?


The idea is that only when the the setting of the priv key fails, it 
returns

the size of the expected privkey.

Which new function are you referring to?


I was referring to crypto_akcipher_maxsize. When
crypto_akcipher_set_priv_key fails, you are overwriting it's return
value with the value of crypto_akcipher_maxsize, hiding the cause of
the error.


Oops, I missed the negation. When crypto_akcipher_set_priv_key succeeds
you return the akcipher_maxsize. Not a bad idea, you save few cpu
cycles.



crypto akcipher uses a dedicated function for determining the length of
the output buffer, crypto_akcipher_maxsize. Should we add a new function
pointer in struct af_alg_type that returns the maxsize?


Your API is different from crypto's akcipher. Should we make them
identical?

Cheers,
ta

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

[Tudor Ambarus]

Hi, Stephan,


+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?


The idea is that only when the the setting of the priv key fails, it returns
the size of the expected privkey.

Which new function are you referring to?


I was referring to crypto_akcipher_maxsize. When
crypto_akcipher_set_priv_key fails, you are overwriting it's return
value with the value of crypto_akcipher_maxsize, hiding the cause of
the error.

crypto akcipher uses a dedicated function for determining the length of
the output buffer, crypto_akcipher_maxsize. Should we add a new function
pointer in struct af_alg_type that returns the maxsize?

Thanks,
ta

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

[Stephan Müller]

Am Freitag, 11. August 2017, 14:51:10 CEST schrieb Tudor Ambarus:

Hi Tudor,

I have covered all your requests
> 
> > +   size_t used = 0;
> 
> initialization to zero not needed. You can directly initialize to
> ctx->used or don't initialize at all.

It is not initialized now. We cannot use ctx->used here as the socket (and 
thus the ctx data structure) is not locked yet.

> > +
> > +   /*
> > +* 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?

We must get something out of the cipher operation as otherwise something is 
wrong. In this case I would like to error out to prevent an endless loop here.

> > +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?

The idea is that only when the the setting of the priv key fails, it returns 
the size of the expected privkey.

Which new function are you referring to?

Ciao
Stephan

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

[Tudor Ambarus]

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 
---
  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 ..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 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


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, );
+   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 */
+   

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

[Stephan Müller]

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 
---
 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 ..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 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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;
+   int maxsize;
+   size_t len = 0;
+   size_t used = 0;
+
+   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, );
+   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(>cra_u.akcipher_req, tfm);
+   akcipher_request_set_crypt(>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(>cra_u.akcipher_req,
+ CRYPTO_TFM_REQ_MAY_SLEEP,
+ af_alg_async_cb, areq);
+   } else
+   /* Synchronous operation */
+