Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kernel TLS Receive Side #7848

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
7 participants
@borisPis
Copy link

commented Dec 7, 2018

This pull request completes the support for the Linux kernel TLS data-path
by adding receive-side support to the send-side support introduced in pull
request 5253.

Similarly to the send-side, in the receive-side the kernel returns
plaintext data while stripping the TLS record layer. This should improve
efficiency, because the kernel can decrypt data directly into user space
buffers and avoid a data-copy.

The user API is as follows: (also see [1])

After setting the TLS_RX socket option, all recv family socket calls
are decrypted using TLS parameters provided. A full TLS record must
be received before decryption can happen.

char buffer[16384];
recv(sock, buffer, 16384);

Received data is decrypted directly in to the user buffer if it is
large enough, and no additional allocations occur. If the userspace
buffer is too small, data is decrypted in the kernel and copied to
userspace.

EINVAL is returned if the TLS version in the received message does not
match the version passed in setsockopt.

EMSGSIZE is returned if the received message is too big.

EBADMSG is returned if decryption failed for any other reason.

@FdaSilvaYY

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

$ if [ -n "$CHECKDOCS" ]; then if $make doc-nits; then echo -e '+\057\057 MAKE DOC-NITS OK'; else echo -e '+\057\057 MAKE DOC-NITS FAILED'; false; fi; fi
(cd .; /usr/bin/perl util/find-doc-nits -n -p ) >doc-nits
doc/man3/BIO_ctrl.pod:1: missing comma in NAME
make: *** [doc-nits] Error 1
Show resolved Hide resolved doc/man3/BIO_ctrl.pod Outdated
Show resolved Hide resolved include/internal/ktls.h Outdated
Show resolved Hide resolved ssl/record/rec_layer_s3.c Outdated
Show resolved Hide resolved doc/man3/BIO_ctrl.pod

@borisPis borisPis force-pushed the Mellanox:tls_rx3 branch 3 times, most recently from 67f7599 to aa41da9 Dec 13, 2018

@borisPis

This comment has been minimized.

Copy link
Author

commented Dec 16, 2018

Hi @mattcaswell, this completes PR 5253 maybe you can take a look?

Show resolved Hide resolved doc/man3/BIO_ctrl.pod Outdated
Show resolved Hide resolved doc/man3/BIO_ctrl.pod Outdated
Show resolved Hide resolved include/internal/bio.h Outdated
Show resolved Hide resolved include/internal/ktls.h Outdated
Show resolved Hide resolved include/internal/ktls.h
Show resolved Hide resolved ssl/t1_enc.c Outdated
Show resolved Hide resolved ssl/t1_enc.c Outdated
Show resolved Hide resolved ssl/t1_enc.c Outdated
Show resolved Hide resolved ssl/t1_enc.c Outdated
Show resolved Hide resolved ssl/t1_enc.c

@borisPis borisPis force-pushed the Mellanox:tls_rx3 branch 3 times, most recently from 2943da0 to 2f970c5 Feb 6, 2019

@mattcaswell
Copy link
Member

left a comment

A few minor niggles remaining from me

Show resolved Hide resolved include/internal/ktls.h Outdated
*(p + 2) = TLS1_2_VERSION_MINOR;
*(p + 3) = ret >> 8 & 0xff;
*(p + 4) = ret & 0xff;
ret += prepend_length;

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Feb 12, 2019

Member

WPACKET seems more appropriate here.

This comment has been minimized.

Copy link
@borisPis

borisPis Feb 18, 2019

Author

WPACKET is internal to "/ssl", using it here in "/include/internal" is somewhat unusual. Additionally it adds some unnecessary allocations that could be avoided for its internal sub packets structure.

The code is not much more readable: (something like this)
WPACKET wpkt;
WPACKET_init_static_len(&wpkt, p, length, 0);
WPACKET_put_bytes_u8(&wpkt, *((unsigned char *)CMSG_DATA(cmsg)));
WPACKET_put_bytes_u8(&wpkt, TLS1_2_VERSION_MAJOR);
WPACKET_put_bytes_u8(&wpkt, TLS1_2_VERSION_MINOR);
WPACKET_put_bytes_u8(&wpkt, (ret >> 8) & 0xff);
WPACKET_put_bytes_u8(&wpkt, ret & 0xff)

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Feb 19, 2019

Member

Ok - but see comment about length above (WPACKET provides us with additional safety around checking length).

This comment has been minimized.

Copy link
@borisPis

borisPis Feb 21, 2019

Author

OK. So I'll add a comment here. We are safe because the kernel returns a value shorter than the length provided to it, which is shorter than (SIZE_MAX / 2).

Show resolved Hide resolved ssl/record/rec_layer_s3.c Outdated
Show resolved Hide resolved ssl/record/rec_layer_s3.c Outdated

@borisPis borisPis force-pushed the Mellanox:tls_rx3 branch from 2f970c5 to 5d0e446 Feb 18, 2019

msg.msg_controllen = sizeof(buf);

msg_iov.iov_base = p + prepend_length;
msg_iov.iov_len = length - prepend_length - EVP_GCM_TLS_TAG_LEN;

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Feb 19, 2019

Member

Don't we need to sanity check that length is long enough here? i.e. must be at least prepend_length + EVP_GCM_TLS_TAG_LEN

This comment has been minimized.

Copy link
@borisPis

borisPis Feb 21, 2019

Author

We modify ssl3_read_n to use the maximum length. I'll add a sanity check here just in case.

Show resolved Hide resolved include/internal/ktls.h Outdated
*(p + 2) = TLS1_2_VERSION_MINOR;
*(p + 3) = ret >> 8 & 0xff;
*(p + 4) = ret & 0xff;
ret += prepend_length;

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Feb 19, 2019

Member

Ok - but see comment about length above (WPACKET provides us with additional safety around checking length).

Show resolved Hide resolved include/internal/bio.h Outdated
* Ktls always reads full records.
* Also, we always act like read_ahead is set for DTLS.
*/
if (!BIO_get_ktls_recv(s->rbio) && !s->rlayer.read_ahead

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Feb 19, 2019

Member

Not sure I understand why a change is required here?

This comment has been minimized.

Copy link
@borisPis

borisPis Feb 21, 2019

Author

This is used to avoid splitting the read between header and data in ssl3_get_record. Instead, using this, we read the maximum size on the call to read the header.

For ktls reading the header alone doesn't make any sense...

@borisPis borisPis force-pushed the Mellanox:tls_rx3 branch from 5d0e446 to 685d3f1 Feb 21, 2019

@borisPis

This comment has been minimized.

Copy link
Author

commented Mar 3, 2019

@mattcaswell
Copy link
Member

left a comment

Approved assuming the one minor style nit is fixed

return rret; /* error or non-blocking */
if (rret <= 0) {
if (!BIO_get_ktls_recv(s->rbio))
return rret; /* error or non-blocking */

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Mar 4, 2019

Member

Style nit: since this if has an "else" which uses {}, the main "if" block should also have {}

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Needs second review @openssl/committers

@mattcaswell mattcaswell added the master label Mar 4, 2019

Show resolved Hide resolved include/internal/bio.h Outdated
Show resolved Hide resolved include/internal/ktls.h Outdated
Show resolved Hide resolved ssl/record/ssl3_record.c Outdated
Show resolved Hide resolved ssl/record/ssl3_record.c Outdated

@borisPis borisPis force-pushed the Mellanox:tls_rx3 branch 2 times, most recently from 35f1f40 to b3a010d Mar 7, 2019

@bernd-edlinger
Copy link
Member

left a comment

please include the linux headers directly.
Since you check the linux version you can assume the other defines are
also available. In general we avoid to define stuff from the a system name scope.

Show resolved Hide resolved include/internal/ktls.h Outdated

@borisPis borisPis force-pushed the Mellanox:tls_rx3 branch from b3a010d to 64f8d1d Mar 7, 2019

@borisPis

This comment has been minimized.

Copy link
Author

commented Mar 7, 2019

I don't think you can work around broken linux headers like this.

Good point. Will Fix.

@dcbeckett

This comment has been minimized.

Copy link

commented Mar 7, 2019

I tried a test with this patch set

fallocate -l 100M 100M.out
./openssl s_server -accept 443 -tls1_2 -WWW

echo -e "GET /100M.out \r\n\r\n" | ./openssl s_client -quiet -connect [fc00::1]:443 -cipher 'ECDHE-RSA-AES128-GCM-SHA256' -tls1_2

And I received this error

139661666381824:error:1408F092:SSL routines:ssl3_get_record:data length too long:ssl/record/ssl3_record.c:783:

Debugging this, it shows the incoming ktls record was 16688 Bytes.

It appears that KTLS RX is able to send > 16KB up to openssl, this is due to the socket buffer being 16712 bytes (defined in ssl3_setup_read_buffer). Not sure how best to solve this as I'm not overly familiar with the openssl codebase.

@borisPis borisPis force-pushed the Mellanox:tls_rx3 branch from 64f8d1d to 82508a9 Mar 11, 2019

Boris Pismenny added some commits Mar 11, 2018

Boris Pismenny
Linux ktls Rx infrastructure
Introduce the infrastructure for supproting receive side Linux Kernel TLS
data-path.

Change-Id: I71864d8f9d74a701cc8b0ad5536005f3c1716c1c
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Boris Pismenny
bio: Linux TLS Rx Offload
Add support for Linux TLS Rx offload in the BIO layer.

Change-Id: I79924b25dd290a873d69f6c8d429e1f5bb2c3365
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Boris Pismenny
ssl: Linux TLS Rx Offload
This patch adds support for the Linux TLS Rx socket option.
It completes the previous patch for TLS Tx offload.
If the socket option is successful, then the receive data-path of the TCP
socket is implemented by the kernel.
We choose to set this option at the earliest - just after CCS is complete.

Change-Id: I59741e04d89dddca7fb138e88fffcc1259b30132
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Boris Pismenny
sslapitest: add test ktls Rx
Add a unit-test for ktls receive side.

Change-Id: I890588681d05fba419f644f6d903be6dc83c9ed5
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Boris Pismenny

@borisPis borisPis force-pushed the Mellanox:tls_rx3 branch from f4e6ccd to 59a7cf1 Apr 1, 2019

@bernd-edlinger
Copy link
Member

left a comment

reconfirmed, and apologies for the delay.

@mattcaswell
Copy link
Member

left a comment

Reconfirm

@mattcaswell mattcaswell added ready and removed pending 2nd review labels Apr 1, 2019

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Pushed. Thanks.

@mattcaswell mattcaswell closed this Apr 1, 2019

levitte pushed a commit that referenced this pull request Apr 1, 2019

Linux ktls Rx infrastructure
Introduce the infrastructure for supproting receive side Linux Kernel TLS
data-path.

Change-Id: I71864d8f9d74a701cc8b0ad5536005f3c1716c1c
Signed-off-by: Boris Pismenny <borisp@mellanox.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #7848)

levitte pushed a commit that referenced this pull request Apr 1, 2019

bio: Linux TLS Rx Offload
Add support for Linux TLS Rx offload in the BIO layer.

Change-Id: I79924b25dd290a873d69f6c8d429e1f5bb2c3365
Signed-off-by: Boris Pismenny <borisp@mellanox.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #7848)

levitte pushed a commit that referenced this pull request Apr 1, 2019

ssl: Linux TLS Rx Offload
This patch adds support for the Linux TLS Rx socket option.
It completes the previous patch for TLS Tx offload.
If the socket option is successful, then the receive data-path of the TCP
socket is implemented by the kernel.
We choose to set this option at the earliest - just after CCS is complete.

Change-Id: I59741e04d89dddca7fb138e88fffcc1259b30132
Signed-off-by: Boris Pismenny <borisp@mellanox.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #7848)

levitte pushed a commit that referenced this pull request Apr 1, 2019

sslapitest: add test ktls Rx
Add a unit-test for ktls receive side.

Change-Id: I890588681d05fba419f644f6d903be6dc83c9ed5
Signed-off-by: Boris Pismenny <borisp@mellanox.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #7848)

levitte pushed a commit that referenced this pull request Apr 1, 2019

apps: print Kernel receive side TLS in s_client and s_server
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #7848)

levitte pushed a commit that referenced this pull request Apr 1, 2019

add documentation
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #7848)
@richsalz

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Seems to me supporting kernel TLS deserves a note in CHANGES NEWS or something.

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

There already is one:

  *) Added support for Linux Kernel TLS data-path. The Linux Kernel data-path
     improves application performance by removing data copies and providing
     applications with zero-copy system calls such as sendfile and splice.
     [Boris Pismenny]
@bernd-edlinger

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Hmm, since this got merged all BoringSSL tests started to fail.

@borisPis

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

Hmm, since this got merged all BoringSSL tests started to fail.

Interesting. I'd like to reproduce this on my setup. Where can I find the tests?

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Information on how to run the BoringSSL tests is here:

https://github.com/openssl/openssl/blob/master/test/README.external

beldmit added a commit to beldmit/openssl that referenced this pull request May 31, 2019

Linux ktls Rx infrastructure
Introduce the infrastructure for supproting receive side Linux Kernel TLS
data-path.

Change-Id: I71864d8f9d74a701cc8b0ad5536005f3c1716c1c
Signed-off-by: Boris Pismenny <borisp@mellanox.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#7848)

beldmit added a commit to beldmit/openssl that referenced this pull request May 31, 2019

bio: Linux TLS Rx Offload
Add support for Linux TLS Rx offload in the BIO layer.

Change-Id: I79924b25dd290a873d69f6c8d429e1f5bb2c3365
Signed-off-by: Boris Pismenny <borisp@mellanox.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#7848)

beldmit added a commit to beldmit/openssl that referenced this pull request May 31, 2019

ssl: Linux TLS Rx Offload
This patch adds support for the Linux TLS Rx socket option.
It completes the previous patch for TLS Tx offload.
If the socket option is successful, then the receive data-path of the TCP
socket is implemented by the kernel.
We choose to set this option at the earliest - just after CCS is complete.

Change-Id: I59741e04d89dddca7fb138e88fffcc1259b30132
Signed-off-by: Boris Pismenny <borisp@mellanox.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#7848)

beldmit added a commit to beldmit/openssl that referenced this pull request May 31, 2019

sslapitest: add test ktls Rx
Add a unit-test for ktls receive side.

Change-Id: I890588681d05fba419f644f6d903be6dc83c9ed5
Signed-off-by: Boris Pismenny <borisp@mellanox.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#7848)

beldmit added a commit to beldmit/openssl that referenced this pull request May 31, 2019

apps: print Kernel receive side TLS in s_client and s_server
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#7848)

beldmit added a commit to beldmit/openssl that referenced this pull request May 31, 2019

add documentation
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#7848)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.