Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Dave Watson]

On 07/12/17 09:20 AM, Steffen Klassert wrote:
> On Tue, Jul 11, 2017 at 11:53:11AM -0700, Dave Watson wrote:
> > On 07/11/17 08:29 AM, Steffen Klassert wrote:
> > > Sorry for replying to old mail...
> > > > +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> > > > +{
> > > 
> > > ...
> > > 
> > > > +
> > > > +   if (!sw_ctx->aead_send) {
> > > > +   sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> > > > +   if (IS_ERR(sw_ctx->aead_send)) {
> > > > +   rc = PTR_ERR(sw_ctx->aead_send);
> > > > +   sw_ctx->aead_send = NULL;
> > > > +   goto free_rec_seq;
> > > > +   }
> > > > +   }
> > > > +
> > > 
> > > When I look on how you allocate the aead transformation, it seems
> > > that you should either register an asynchronous callback with
> > > aead_request_set_callback(), or request for a synchronous algorithm.
> > > 
> > > Otherwise you will crash on an asynchronous crypto return, no?
> > 
> > The intention is for it to be synchronous, and gather directly from
> > userspace buffers.  It looks like calling
> > crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC) is the correct way
> > to request synchronous algorithms only?
> 
> Yes, but then you loose the aes-ni based algorithms because they are
> asynchronous. If you want to have good crypto performance, it is
> better to implement the asynchronous callbacks.

Right, the trick is we want both aesni, and to guarantee that we are
done using the input buffers before sendmsg() returns.  For now I can
set a callback, and wait on a completion.  The initial use case of
userspace openssl integration shouldn't hit the aesni async case
anyway (!irq_fpu_usable())

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Steffen Klassert]

On Tue, Jul 11, 2017 at 11:53:11AM -0700, Dave Watson wrote:
> On 07/11/17 08:29 AM, Steffen Klassert wrote:
> > Sorry for replying to old mail...
> > > +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> > > +{
> > 
> > ...
> > 
> > > +
> > > + if (!sw_ctx->aead_send) {
> > > + sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> > > + if (IS_ERR(sw_ctx->aead_send)) {
> > > + rc = PTR_ERR(sw_ctx->aead_send);
> > > + sw_ctx->aead_send = NULL;
> > > + goto free_rec_seq;
> > > + }
> > > + }
> > > +
> > 
> > When I look on how you allocate the aead transformation, it seems
> > that you should either register an asynchronous callback with
> > aead_request_set_callback(), or request for a synchronous algorithm.
> > 
> > Otherwise you will crash on an asynchronous crypto return, no?
> 
> The intention is for it to be synchronous, and gather directly from
> userspace buffers.  It looks like calling
> crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC) is the correct way
> to request synchronous algorithms only?

Yes, but then you loose the aes-ni based algorithms because they are
asynchronous. If you want to have good crypto performance, it is
better to implement the asynchronous callbacks.

> 
> > Also, it seems that you have your scatterlists on a per crypto
> > transformation base istead of per crypto request. Is this intentional?
> 
> We hold the socket lock and only one crypto op can happen at a time,
> so we reuse the scatterlists.

This is OK as long as the crypto happens synchronous. But as said above,
I think this is not what you want.

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Eric Biggers]

On Tue, Jul 11, 2017 at 11:53:11AM -0700, Dave Watson wrote:
> On 07/11/17 08:29 AM, Steffen Klassert wrote:
> > Sorry for replying to old mail...
> > > +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> > > +{
> > 
> > ...
> > 
> > > +
> > > + if (!sw_ctx->aead_send) {
> > > + sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> > > + if (IS_ERR(sw_ctx->aead_send)) {
> > > + rc = PTR_ERR(sw_ctx->aead_send);
> > > + sw_ctx->aead_send = NULL;
> > > + goto free_rec_seq;
> > > + }
> > > + }
> > > +
> > 
> > When I look on how you allocate the aead transformation, it seems
> > that you should either register an asynchronous callback with
> > aead_request_set_callback(), or request for a synchronous algorithm.
> > 
> > Otherwise you will crash on an asynchronous crypto return, no?
> 
> The intention is for it to be synchronous, and gather directly from
> userspace buffers.  It looks like calling
> crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC) is the correct way
> to request synchronous algorithms only?
> 

Yes, that means the CRYPTO_ALG_ASYNC bit is required to be 0, i.e. the algorithm
must be synchronous.  Currently it's requesting either a synchronous or
asynchronous algorithm, and it will crash if it gets an async one.

Also I think even with a synchronous algorithm, tls_do_encryption() still needs
to call aead_request_set_callback(), passing NULL for the callback and data, so
that the request flags are initialized.

Eric

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Dave Watson]

On 07/11/17 08:29 AM, Steffen Klassert wrote:
> Sorry for replying to old mail...
> > +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> > +{
> 
> ...
> 
> > +
> > +   if (!sw_ctx->aead_send) {
> > +   sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> > +   if (IS_ERR(sw_ctx->aead_send)) {
> > +   rc = PTR_ERR(sw_ctx->aead_send);
> > +   sw_ctx->aead_send = NULL;
> > +   goto free_rec_seq;
> > +   }
> > +   }
> > +
> 
> When I look on how you allocate the aead transformation, it seems
> that you should either register an asynchronous callback with
> aead_request_set_callback(), or request for a synchronous algorithm.
> 
> Otherwise you will crash on an asynchronous crypto return, no?

The intention is for it to be synchronous, and gather directly from
userspace buffers.  It looks like calling
crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC) is the correct way
to request synchronous algorithms only?

> Also, it seems that you have your scatterlists on a per crypto
> transformation base istead of per crypto request. Is this intentional?

We hold the socket lock and only one crypto op can happen at a time,
so we reuse the scatterlists.

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Steffen Klassert]

Sorry for replying to old mail...

On Wed, Jun 14, 2017 at 11:37:39AM -0700, Dave Watson wrote:
> +static int tls_do_encryption(struct tls_context *tls_ctx,
> +  struct tls_sw_context *ctx, size_t data_len,
> +  gfp_t flags)
> +{
> + unsigned int req_size = sizeof(struct aead_request) +
> + crypto_aead_reqsize(ctx->aead_send);
> + struct aead_request *aead_req;
> + int rc;
> +
> + aead_req = kmalloc(req_size, flags);
> + if (!aead_req)
> + return -ENOMEM;
> +
> + ctx->sg_encrypted_data[0].offset += tls_ctx->prepend_size;
> + ctx->sg_encrypted_data[0].length -= tls_ctx->prepend_size;
> +
> + aead_request_set_tfm(aead_req, ctx->aead_send);
> + aead_request_set_ad(aead_req, TLS_AAD_SPACE_SIZE);
> + aead_request_set_crypt(aead_req, ctx->sg_aead_in, ctx->sg_aead_out,
> +data_len, tls_ctx->iv);
> + rc = crypto_aead_encrypt(aead_req);
> +
> + ctx->sg_encrypted_data[0].offset -= tls_ctx->prepend_size;
> + ctx->sg_encrypted_data[0].length += tls_ctx->prepend_size;
> +
> + kfree(aead_req);
> + return rc;
> +}

...

> +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> +{

...

> +
> + if (!sw_ctx->aead_send) {
> + sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> + if (IS_ERR(sw_ctx->aead_send)) {
> + rc = PTR_ERR(sw_ctx->aead_send);
> + sw_ctx->aead_send = NULL;
> + goto free_rec_seq;
> + }
> + }
> +

When I look on how you allocate the aead transformation, it seems
that you should either register an asynchronous callback with
aead_request_set_callback(), or request for a synchronous algorithm.

Otherwise you will crash on an asynchronous crypto return, no?

Also, it seems that you have your scatterlists on a per crypto
transformation base istead of per crypto request. Is this intentional?

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Dave Watson]

On 06/16/17 01:58 PM, Stephen Hemminger wrote:
> On Wed, 14 Jun 2017 11:37:39 -0700
> Dave Watson  wrote:
> 
> > --- /dev/null
> > +++ b/net/tls/Kconfig
> > @@ -0,0 +1,12 @@
> > +#
> > +# TLS configuration
> > +#
> > +config TLS
> > +   tristate "Transport Layer Security support"
> > +   depends on NET
> > +   default m
> > +   ---help---
> > +   Enable kernel support for TLS protocol. This allows symmetric
> > +   encryption handling of the TLS protocol to be done in-kernel.
> > +
> > +   If unsure, say M.
> 
> I understand that this will be useful to lots of people and most distributions
> will enable it. But the defacto policy in kernel configuration has been that
> new features in kernel default to being disabled.

Sure, will send a patch to switch to default n.

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Stephen Hemminger]

On Wed, 14 Jun 2017 11:37:39 -0700
Dave Watson  wrote:

> --- /dev/null
> +++ b/net/tls/Kconfig
> @@ -0,0 +1,12 @@
> +#
> +# TLS configuration
> +#
> +config TLS
> + tristate "Transport Layer Security support"
> + depends on NET
> + default m
> + ---help---
> + Enable kernel support for TLS protocol. This allows symmetric
> + encryption handling of the TLS protocol to be done in-kernel.
> +
> + If unsure, say M.

I understand that this will be useful to lots of people and most distributions
will enable it. But the defacto policy in kernel configuration has been that
new features in kernel default to being disabled.

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Stephen Hemminger]

On Wed, 14 Jun 2017 11:37:39 -0700
Dave Watson  wrote:

> +
> +static inline struct tls_context *tls_get_ctx(const struct sock *sk)
> +{
> + struct inet_connection_sock *icsk = inet_csk(sk);
> +
> + return icsk->icsk_ulp_data;
> +}
> +
> +static inline struct tls_sw_context *tls_sw_ctx(
> + const struct tls_context *tls_ctx)
> +{
> + return (struct tls_sw_context *)tls_ctx->priv_ctx;
> +}
> +
> +static inline struct tls_offload_context *tls_offload_ctx(
> + const struct tls_context *tls_ctx)
> +{
> + return (struct tls_offload_context *)tls_ctx->priv_ctx;
> +}
> +

Since priv_ctx is void *, casts here are unnecessary.

[PATCH v3 net-next 3/4] tls: kernel TLS support

[Dave Watson]

Software implementation of transport layer security, implemented using ULP
infrastructure.  tcp proto_ops are replaced with tls equivalents of sendmsg and
sendpage.

Only symmetric crypto is done in the kernel, keys are passed by setsockopt
after the handshake is complete.  All control messages are supported via CMSG
data - the actual symmetric encryption is the same, just the message type needs
to be passed separately.

For user API, please see Documentation patch.

Pieces that can be shared between hw and sw implementation
are in tls_main.c

Signed-off-by: Boris Pismenny 
Signed-off-by: Ilya Lesokhin 
Signed-off-by: Aviad Yehezkel 
Signed-off-by: Dave Watson 
---
 MAINTAINERS  |  10 +
 include/linux/socket.h   |   1 +
 include/net/tls.h| 237 +++
 include/uapi/linux/tls.h |  79 +
 net/Kconfig  |   1 +
 net/Makefile |   1 +
 net/tls/Kconfig  |  12 +
 net/tls/Makefile |   7 +
 net/tls/tls_main.c   | 487 ++
 net/tls/tls_sw.c | 772 +++
 10 files changed, 1607 insertions(+)
 create mode 100644 include/net/tls.h
 create mode 100644 include/uapi/linux/tls.h
 create mode 100644 net/tls/Kconfig
 create mode 100644 net/tls/Makefile
 create mode 100644 net/tls/tls_main.c
 create mode 100644 net/tls/tls_sw.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f4e682c..710af53 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8979,6 +8979,16 @@ F:   net/ipv6/
 F: include/net/ip*
 F: arch/x86/net/*
 
+NETWORKING [TLS]
+M: Ilya Lesokhin 
+M: Aviad Yehezkel 
+M: Dave Watson 
+L: net...@vger.kernel.org
+S: Maintained
+F: net/tls/*
+F: include/uapi/linux/tls.h
+F: include/net/tls.h
+
 NETWORKING [IPSEC]
 M: Steffen Klassert 
 M: Herbert Xu 
diff --git a/include/linux/socket.h b/include/linux/socket.h
index 0820274..8b13db5 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -334,6 +334,7 @@ struct ucred {
 #define SOL_ALG279
 #define SOL_NFC280
 #define SOL_KCM281
+#define SOL_TLS282
 
 /* IPX options */
 #define IPX_TYPE   1
diff --git a/include/net/tls.h b/include/net/tls.h
new file mode 100644
index 000..b89d397
--- /dev/null
+++ b/include/net/tls.h
@@ -0,0 +1,237 @@
+/*
+ * Copyright (c) 2016-2017, Mellanox Technologies. All rights reserved.
+ * Copyright (c) 2016-2017, Dave Watson . All rights 
reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ *  - Redistributions of source code must retain the above
+ *copyright notice, this list of conditions and the following
+ *disclaimer.
+ *
+ *  - Redistributions in binary form must reproduce the above
+ *copyright notice, this list of conditions and the following
+ *disclaimer in the documentation and/or other materials
+ *provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef _TLS_OFFLOAD_H
+#define _TLS_OFFLOAD_H
+
+#include 
+
+#include 
+
+
+/* Maximum data size carried in a TLS record */
+#define TLS_MAX_PAYLOAD_SIZE   ((size_t)1 << 14)
+
+#define TLS_HEADER_SIZE5
+#define TLS_NONCE_OFFSET   TLS_HEADER_SIZE
+
+#define TLS_CRYPTO_INFO_READY(info)((info)->cipher_type)
+
+#define TLS_RECORD_TYPE_DATA   0x17
+
+#define TLS_AAD_SPACE_SIZE 13
+
+struct tls_sw_context {
+   struct crypto_aead *aead_send;
+
+   /* Sending context */
+   char aad_space[TLS_AAD_SPACE_SIZE];
+
+   unsigned int sg_plaintext_size;
+   int sg_plaintext_num_elem;
+   struct scatterlist sg_plaintext_data[MAX_SKB_FRAGS];
+
+   unsigned int sg_encrypted_size;
+   int sg_encrypted_num_elem;
+   struct scatterlist