Re: [PATCH v9 crypto 06/12] cxgb4: LLD driver changes to enable TLS
On 3/7/2018 6:29 PM, Sabrina Dubroca wrote:
> 2018-03-06, 21:09:25 +0530, Atul Gupta wrote:
>> Read FW capability. Read key area size. Dump the TLS record count.
> That's not a really helpful commit message. Have a look at other
> commit messages and try to be more descriptive.
> It's also not clear if those changes belong together in one patch, but
> I can't judge because the commit message is really too terse.
will add the description
>
>> Signed-off-by: Atul Gupta
>> ---
>> drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 32 +---
>> drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.h | 7 ++
>> drivers/net/ethernet/chelsio/cxgb4/sge.c| 98
>> -
>> 3 files changed, 126 insertions(+), 11 deletions(-)
> [snip]
>> +int cxgb4_immdata_send(struct net_device *dev, unsigned int idx,
>> + const void *src, unsigned int len)
>> +{
>> +struct adapter *adap = netdev2adap(dev);
>> +struct sge_uld_txq_info *txq_info;
>> +struct sge_uld_txq *txq;
>> +int ret;
>> +
>> +local_bh_disable();
>> +txq_info = adap->sge.uld_txq_info[CXGB4_TX_OFLD];
>> +if (unlikely(!txq_info)) {
>> +WARN_ON(true);
>> +return NET_XMIT_DROP;
> Don't you need to local_bh_enable() before returning? If not, this
> needs a comment to explain why.
should enable before return [thanks]
>
>> +}
>> +txq = &txq_info->uldtxq[idx];
>> +
>> +ret = ofld_xmit_direct(txq, src, len);
>> +local_bh_enable();
>> +return net_xmit_eval(ret);
>> +}
>> +EXPORT_SYMBOL(cxgb4_immdata_send);
Re: [PATCH] crypto/ecc: Remove stack VLA usage
Hi, Kees,
On 03/07/2018 11:56 PM, Kees Cook wrote:
On the quest to remove all VLAs from the kernel[1], this switches to
a pair of kmalloc regions instead of using the stack. This also moves
the get_random_bytes() after all allocations (and drops the needless
"nbytes" variable).
[1] https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Kees Cook
---
crypto/ecc.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/crypto/ecc.c b/crypto/ecc.c
index 18f32f2a5e1c..5bfa63603da0 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
unsigned int ndigits,
{
int ret = 0;
struct ecc_point *product, *pk;
- u64 priv[ndigits];
- u64 rand_z[ndigits];
- unsigned int nbytes;
+ u64 *priv, *rand_z;
const struct ecc_curve *curve = ecc_get_curve(curve_id);
if (!private_key || !public_key || !curve) {
@@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
unsigned int ndigits,
goto out;
}
- nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
+ priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ ret = -ENOMEM;
+ goto out;
+ }
- get_random_bytes(rand_z, nbytes);
+ rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL);
+ if (!rand_z) {
+ ret = -ENOMEM;
+ goto kfree_out;
+ }
pk = ecc_alloc_point(ndigits);
if (!pk) {
ret = -ENOMEM;
- goto out;
+ goto kfree_out;
}
product = ecc_alloc_point(ndigits);
@@ -1051,6 +1057,8 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
unsigned int ndigits,
goto err_alloc_product;
}
+ get_random_bytes(rand_z, ndigits << ECC_DIGITS_TO_BYTES_SHIFT);
+
ecc_swap_digits(public_key, pk->x, ndigits);
ecc_swap_digits(&public_key[ndigits], pk->y, ndigits);
ecc_swap_digits(private_key, priv, ndigits);
@@ -1065,6 +1073,9 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
unsigned int ndigits,
ecc_free_point(product);
err_alloc_product:
ecc_free_point(pk);
+kfree_out:
+ kfree(priv);
I think we should use kzfree here.
+ kfree(rand_z);
Probably here too. Looks like there are few intermediate buffers in ecc
that should be zeroized as well.
Best,
ta
out:
return ret;
}
Re: [PATCH v9 crypto 08/12] chtls: Key program
On 3/7/2018 8:35 PM, Sabrina Dubroca wrote:
> 2018-03-06, 21:09:27 +0530, Atul Gupta wrote:
> [snip]
>> +static int chtls_set_tcb_field(struct sock *sk, u16 word, u64 mask, u64 val)
>> +{
>> +struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
>> +struct sk_buff *skb;
>> +struct cpl_set_tcb_field *req;
>> +struct ulptx_idata *sc;
>> +unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
>> +unsigned int credits_needed = DIV_ROUND_UP(wrlen, 16);
>> +
>> +skb = alloc_skb(wrlen, GFP_ATOMIC);
> I haven't followed the whole call chain, but does this really need to
> be an atomic allocation?
this is used to send control info to HW, will look at it again for atomic usage
> Should this skb be charged to the socket's memory accounting?
>
>> +if (!skb)
>> +return -ENOMEM;
>> +
>> +__set_tcb_field(sk, skb, word, mask, val, 0, 1);
>> +set_queue(skb, (csk->txq_idx << 1) | CPL_PRIORITY_DATA, sk);
>> +csk->wr_credits -= credits_needed;
>> +csk->wr_unacked += credits_needed;
>> +enqueue_wr(csk, skb);
>> +cxgb4_ofld_send(csk->egress_dev, skb);
> Should the return value be checked?
Yes, good to check
>
>> +return 0;
>> +}
>
> [snip]
>> +static void *chtls_alloc_mem(unsigned long size)
>> +{
>> +void *p = kmalloc(size, GFP_KERNEL);
>> +
>> +if (!p)
>> +p = vmalloc(size);
>> +if (p)
>> +memset(p, 0, size);
>> +return p;
>> +}
> I think you replace this with kvzalloc().
taken care, thanks
>
>> +static void chtls_free_mem(void *addr)
>> +{
>> +unsigned long p = (unsigned long)addr;
>> +
>> +if (p >= VMALLOC_START && p < VMALLOC_END)
>> +vfree(addr);
>> +else
>> +kfree(addr);
>> +}
> Use kvfree().
done
>
>> +/* TLS Key bitmap processing */
>> +int chtls_init_kmap(struct chtls_dev *cdev, struct cxgb4_lld_info *lldi)
>> +{
>> +unsigned int num_key_ctx, bsize;
>> +
>> +num_key_ctx = (lldi->vr->key.size / TLS_KEY_CONTEXT_SZ);
>> +bsize = BITS_TO_LONGS(num_key_ctx);
>> +
>> +cdev->kmap.size = num_key_ctx;
>> +cdev->kmap.available = bsize;
>> +cdev->kmap.addr = chtls_alloc_mem(sizeof(*cdev->kmap.addr) *
>> + bsize);
>> +if (!cdev->kmap.addr)
>> +return -1;
> The return value of this function is never checked.
return not required, will revisit.
>
>> +
>> +cdev->kmap.start = lldi->vr->key.start;
>> +spin_lock_init(&cdev->kmap.lock);
>> +return 0;
>> +}
>> +
>> +void chtls_free_kmap(struct chtls_dev *cdev)
>> +{
>> +if (cdev->kmap.addr)
>> +chtls_free_mem(cdev->kmap.addr);
>> +}
> I think this wrapper function is not necessary.
removed
>
>> +static int get_new_keyid(struct chtls_sock *csk, u32 optname)
>> +{
>> +struct chtls_dev *cdev = csk->cdev;
>> +struct chtls_hws *hws = &csk->tlshws;
>> +struct net_device *dev = csk->egress_dev;
>> +struct adapter *adap = netdev2adap(dev);
>> +int keyid;
>> +
>> +spin_lock_bh(&cdev->kmap.lock);
>> +keyid = find_first_zero_bit(cdev->kmap.addr, cdev->kmap.size);
>> +if (keyid < cdev->kmap.size) {
>> +__set_bit(keyid, cdev->kmap.addr);
>> +if (optname == TLS_RX)
> TLS_RX only gets defined in patch 11, so the only reason you're not
> breaking the build is because all these new files aren't getting
> compiled until patch 12.
yes
>
>> +hws->rxkey = keyid;
>> +else
>> +hws->txkey = keyid;
>> +atomic_inc(&adap->chcr_stats.tls_key);
>> +} else {
>> +keyid = -1;
>> +}
>> +spin_unlock_bh(&cdev->kmap.lock);
>> +return keyid;
>> +}
>> +
> [snip]
>> +int chtls_setkey(struct chtls_sock *csk, u32 keylen, u32 optname)
>> +{
> ...
>> +skb = alloc_skb(len, GFP_KERNEL);
>> +if (!skb)
>> +return -ENOMEM;
> This return code is also ignored by its caller. Please review the
> whole patchset for that problem.
will do, thanks
>
> Same question as before, does this need to be accounted?
>
>
>> +/* ulptx command */
>> +kwr->req.cmd = cpu_to_be32(ULPTX_CMD_V(ULP_TX_MEM_WRITE) |
>> +T5_ULP_MEMIO_ORDER_V(1) |
>> +T5_ULP_MEMIO_IMM_V(1));
>> +kwr->req.len16 = cpu_to_be32((csk->tid << 8) |
>> + DIV_ROUND_UP(len - sizeof(kwr->wr), 16));
>> +kwr->req.dlen = cpu_to_be32(ULP_MEMIO_DATA_LEN_V(klen >> 5));
>> +kwr->req.lock_addr = cpu_to_be32(ULP_MEMIO_ADDR_V(keyid_to_addr
>> +(cdev->kmap.start, keyid)));
> Breaking this line in that way makes it really hard to read for
> humans.
I will clean this
Thanks
Atul
>
[PATCH RFC 0/5] TLX Rx
TLS tcp socket RX implementation, to match existing TX code.
This patchset completes the software TLS socket, allowing full
bi-directional communication over TLS using normal socket syscalls,
after the handshake has been done in userspace. Only the symmetric
encryption is done in the kernel.
This allows usage of TLS sockets from within the kernel (for example
with network block device, or from bpf). Performance can be better
than userspace, with appropriate crypto routines [1].
sk->sk_socket->ops must be overridden to implement splice_read and
poll, but otherwise the interface & implementation match TX closely.
strparser is used to parse TLS framing on receive.
There are Openssl RX patches that work with this interface [2], as
well as a testing tool using the socket interface directly (without
cmsg support) [3]. An example tcp socket setup is:
// Normal tcp socket connect/accept, and TLS handshake
// using any TLS library.
setsockopt(sock, SOL_TCP, TCP_ULP, "tls", sizeof("tls"));
struct tls12_crypto_info_aes_gcm_128 crypto_info_rx;
// Fill in crypto_info based on negotiated keys.
setsockopt(sock, SOL_TLS, TLS_RX, &crypto_info, sizeof(crypto_info_rx));
// You can optionally TLX_TX as well.
char buffer[16384];
int ret = recv(sock, buffer, 16384);
// cmsg can be received using recvmsg and a msg_control
// of type TLS_GET_RECORD_TYPE will be set.
[1] Recent crypto patchset to remove copies, resulting in optimally
zero copies vs. userspace's one, vs. previous kernel's two.
https://marc.info/?l=linux-crypto-vger&m=151931242406416&w=2
[2] https://github.com/Mellanox/openssl/commits/tls_rx
[3] https://github.com/ktls/af_ktls-tool/tree/RX
Dave Watson (5):
tls: Generalize zerocopy_from_iter
tls: Move cipher info to a separate struct
tls: Pass error code explicitly to tls_err_abort
tls: RX path for ktls
tls: Add receive path documentation
Documentation/networking/tls.txt | 59 +++-
include/net/tls.h| 59 +++-
include/uapi/linux/tls.h | 2 +
net/tls/Kconfig | 1 +
net/tls/tls_main.c | 70 -
net/tls/tls_sw.c | 631 ++-
6 files changed, 708 insertions(+), 114 deletions(-)
--
2.9.5
[PATCH RFC 2/5] tls: Move cipher info to a separate struct
Separate tx crypto parameters to a separate cipher_context struct.
The same parameters will be used for rx using the same struct.
tls_advance_record_sn is modified to only take the cipher info.
Signed-off-by: Dave Watson
---
include/net/tls.h | 26 +---
net/tls/tls_main.c | 8
net/tls/tls_sw.c | 58 --
3 files changed, 49 insertions(+), 43 deletions(-)
diff --git a/include/net/tls.h b/include/net/tls.h
index 4913430..019e52d 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -81,6 +81,16 @@ enum {
TLS_PENDING_CLOSED_RECORD
};
+struct cipher_context {
+ u16 prepend_size;
+ u16 tag_size;
+ u16 overhead_size;
+ u16 iv_size;
+ char *iv;
+ u16 rec_seq_size;
+ char *rec_seq;
+};
+
struct tls_context {
union {
struct tls_crypto_info crypto_send;
@@ -91,13 +101,7 @@ struct tls_context {
u8 tx_conf:2;
- u16 prepend_size;
- u16 tag_size;
- u16 overhead_size;
- u16 iv_size;
- char *iv;
- u16 rec_seq_size;
- char *rec_seq;
+ struct cipher_context tx;
struct scatterlist *partially_sent_record;
u16 partially_sent_offset;
@@ -190,7 +194,7 @@ static inline bool tls_bigint_increment(unsigned char *seq,
int len)
}
static inline void tls_advance_record_sn(struct sock *sk,
-struct tls_context *ctx)
+struct cipher_context *ctx)
{
if (tls_bigint_increment(ctx->rec_seq, ctx->rec_seq_size))
tls_err_abort(sk);
@@ -203,9 +207,9 @@ static inline void tls_fill_prepend(struct tls_context *ctx,
size_t plaintext_len,
unsigned char record_type)
{
- size_t pkt_len, iv_size = ctx->iv_size;
+ size_t pkt_len, iv_size = ctx->tx.iv_size;
- pkt_len = plaintext_len + iv_size + ctx->tag_size;
+ pkt_len = plaintext_len + iv_size + ctx->tx.tag_size;
/* we cover nonce explicit here as well, so buf should be of
* size KTLS_DTLS_HEADER_SIZE + KTLS_DTLS_NONCE_EXPLICIT_SIZE
@@ -217,7 +221,7 @@ static inline void tls_fill_prepend(struct tls_context *ctx,
buf[3] = pkt_len >> 8;
buf[4] = pkt_len & 0xFF;
memcpy(buf + TLS_NONCE_OFFSET,
- ctx->iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE, iv_size);
+ ctx->tx.iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE, iv_size);
}
static inline void tls_make_aad(char *buf,
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index d824d54..c671560 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -259,8 +259,8 @@ static void tls_sk_proto_close(struct sock *sk, long
timeout)
}
}
- kfree(ctx->rec_seq);
- kfree(ctx->iv);
+ kfree(ctx->tx.rec_seq);
+ kfree(ctx->tx.iv);
if (ctx->tx_conf == TLS_SW_TX)
tls_sw_free_tx_resources(sk);
@@ -319,9 +319,9 @@ static int do_tls_getsockopt_tx(struct sock *sk, char
__user *optval,
}
lock_sock(sk);
memcpy(crypto_info_aes_gcm_128->iv,
- ctx->iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE,
+ ctx->tx.iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE,
TLS_CIPHER_AES_GCM_128_IV_SIZE);
- memcpy(crypto_info_aes_gcm_128->rec_seq, ctx->rec_seq,
+ memcpy(crypto_info_aes_gcm_128->rec_seq, ctx->tx.rec_seq,
TLS_CIPHER_AES_GCM_128_REC_SEQ_SIZE);
release_sock(sk);
if (copy_to_user(optval,
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index d58f675..dd4441d 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -79,7 +79,7 @@ static void trim_both_sgl(struct sock *sk, int target_size)
target_size);
if (target_size > 0)
- target_size += tls_ctx->overhead_size;
+ target_size += tls_ctx->tx.overhead_size;
trim_sg(sk, ctx->sg_encrypted_data,
&ctx->sg_encrypted_num_elem,
@@ -207,21 +207,21 @@ static int tls_do_encryption(struct tls_context *tls_ctx,
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;
+ ctx->sg_encrypted_data[0].offset += tls_ctx->tx.prepend_size;
+ ctx->sg_encrypted_data[0].length -= tls_ctx->tx.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);
+ data_len, tls_ctx->tx.iv);
aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_BACKLOG,
[PATCH RFC 3/5] tls: Pass error code explicitly to tls_err_abort
Pass EBADMSG explicitly to tls_err_abort. Receive path will
pass additional codes - E2BIG if framing is larger than max
TLS record size.
Signed-off-by: Dave Watson
---
include/net/tls.h | 6 +++---
net/tls/tls_sw.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/net/tls.h b/include/net/tls.h
index 019e52d..6b44875 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -174,9 +174,9 @@ static inline bool tls_is_pending_open_record(struct
tls_context *tls_ctx)
return tls_ctx->pending_open_record_frags;
}
-static inline void tls_err_abort(struct sock *sk)
+static inline void tls_err_abort(struct sock *sk, int err)
{
- sk->sk_err = EBADMSG;
+ sk->sk_err = err;
sk->sk_error_report(sk);
}
@@ -197,7 +197,7 @@ static inline void tls_advance_record_sn(struct sock *sk,
struct cipher_context *ctx)
{
if (tls_bigint_increment(ctx->rec_seq, ctx->rec_seq_size))
- tls_err_abort(sk);
+ tls_err_abort(sk, EBADMSG);
tls_bigint_increment(ctx->iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE,
ctx->iv_size);
}
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index dd4441d..6a0a669 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -269,7 +269,7 @@ static int tls_push_record(struct sock *sk, int flags,
/* Only pass through MSG_DONTWAIT and MSG_NOSIGNAL flags */
rc = tls_push_sg(sk, tls_ctx, ctx->sg_encrypted_data, 0, flags);
if (rc < 0 && rc != -EAGAIN)
- tls_err_abort(sk);
+ tls_err_abort(sk, EBADMSG);
tls_advance_record_sn(sk, &tls_ctx->tx);
return rc;
--
2.9.5
[PATCH RFC 4/5] tls: RX path for ktls
Add rx path for tls software implementation.
recvmsg, splice_read, and poll implemented.
An additional sockopt TLS_RX is added, with the same interface as
TLS_TX. Either TLX_RX or TLX_TX may be provided separately, or
together (with two different setsockopt calls with appropriate keys).
Control messages are passed via CMSG in a similar way to transmit.
If no cmsg buffer is passed, then only application data records
will be passed to userspace, and EIO is returned for other types of
alerts.
EBADMSG is passed for decryption errors, and E2BIG is passed for framing
errors. Both are unrecoverable.
strparser is used to parse TLS framing. Decryption is done directly
in to userspace buffers if they are large enough to support it, otherwise
sk_cow_data is called (similar to ipsec), and buffers are decrypted in
place and copied. splice_read always decrypts in place, since no
buffers are provided to decrypt in to.
sk_poll is overridden, and only returns POLLIN if a full TLS message is
received. Otherwise we wait for strparser to finish reading a full frame.
Actual decryption is only done during recvmsg or splice_read calls.
Signed-off-by: Dave Watson
---
include/net/tls.h| 27 ++-
include/uapi/linux/tls.h | 2 +
net/tls/Kconfig | 1 +
net/tls/tls_main.c | 62 -
net/tls/tls_sw.c | 574 ++-
5 files changed, 596 insertions(+), 70 deletions(-)
diff --git a/include/net/tls.h b/include/net/tls.h
index 6b44875..7202026 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
@@ -58,8 +59,18 @@
struct tls_sw_context {
struct crypto_aead *aead_send;
+ struct crypto_aead *aead_recv;
struct crypto_wait async_wait;
+ /* Receive context */
+ struct strparser strp;
+ void (*saved_data_ready)(struct sock *sk);
+ unsigned int (*sk_poll)(struct file *file, struct socket *sock,
+ struct poll_table_struct *wait);
+ struct sk_buff *recv_pkt;
+ u8 control;
+ bool decrypted;
+
/* Sending context */
char aad_space[TLS_AAD_SPACE_SIZE];
@@ -96,12 +107,17 @@ struct tls_context {
struct tls_crypto_info crypto_send;
struct tls12_crypto_info_aes_gcm_128 crypto_send_aes_gcm_128;
};
+ union {
+ struct tls_crypto_info crypto_recv;
+ struct tls12_crypto_info_aes_gcm_128 crypto_recv_aes_gcm_128;
+ };
void *priv_ctx;
u8 tx_conf:2;
struct cipher_context tx;
+ struct cipher_context rx;
struct scatterlist *partially_sent_record;
u16 partially_sent_offset;
@@ -128,12 +144,19 @@ int tls_sk_attach(struct sock *sk, int optname, char
__user *optval,
unsigned int optlen);
-int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx);
+int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx);
int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size);
int tls_sw_sendpage(struct sock *sk, struct page *page,
int offset, size_t size, int flags);
void tls_sw_close(struct sock *sk, long timeout);
-void tls_sw_free_tx_resources(struct sock *sk);
+void tls_sw_free_resources(struct sock *sk);
+int tls_sw_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
+ int nonblock, int flags, int *addr_len);
+unsigned int tls_sw_poll(struct file *file, struct socket *sock,
+struct poll_table_struct *wait);
+ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos,
+ struct pipe_inode_info *pipe,
+ size_t len, unsigned int flags);
void tls_sk_destruct(struct sock *sk, struct tls_context *ctx);
void tls_icsk_clean_acked(struct sock *sk);
diff --git a/include/uapi/linux/tls.h b/include/uapi/linux/tls.h
index 293b2cd..c6633e9 100644
--- a/include/uapi/linux/tls.h
+++ b/include/uapi/linux/tls.h
@@ -38,6 +38,7 @@
/* TLS socket options */
#define TLS_TX 1 /* Set transmit parameters */
+#define TLS_RX 2 /* Set receive parameters */
/* Supported versions */
#define TLS_VERSION_MINOR(ver) ((ver) & 0xFF)
@@ -59,6 +60,7 @@
#define TLS_CIPHER_AES_GCM_128_REC_SEQ_SIZE8
#define TLS_SET_RECORD_TYPE1
+#define TLS_GET_RECORD_TYPE2
struct tls_crypto_info {
__u16 version;
diff --git a/net/tls/Kconfig b/net/tls/Kconfig
index eb58303..89b8745a 100644
--- a/net/tls/Kconfig
+++ b/net/tls/Kconfig
@@ -7,6 +7,7 @@ config TLS
select CRYPTO
select CRYPTO_AES
select CRYPTO_GCM
+ select STREAM_PARSER
default n
---help---
Enable kernel support for TLS protocol. This allows symmetric
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index c671560..9a6d533 100644
--- a/net/tls
[PATCH RFC 1/5] tls: Generalize zerocopy_from_iter
Refactor zerocopy_from_iter to take arguments for pages and size,
such that it can be used for both tx and rx. RX will also support
zerocopy direct to output iter, as long as the full message can
be copied at once (a large enough userspace buffer was provided).
Signed-off-by: Dave Watson
---
net/tls/tls_sw.c | 31 +++
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index f26376e..d58f675 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -281,23 +281,24 @@ static int tls_sw_push_pending_record(struct sock *sk,
int flags)
}
static int zerocopy_from_iter(struct sock *sk, struct iov_iter *from,
- int length)
+ int length, int *pages_used,
+ unsigned int *size_used,
+ struct scatterlist *to, int to_max_pages,
+ bool charge)
{
- struct tls_context *tls_ctx = tls_get_ctx(sk);
- struct tls_sw_context *ctx = tls_sw_ctx(tls_ctx);
struct page *pages[MAX_SKB_FRAGS];
size_t offset;
ssize_t copied, use;
int i = 0;
- unsigned int size = ctx->sg_plaintext_size;
- int num_elem = ctx->sg_plaintext_num_elem;
+ unsigned int size = *size_used;
+ int num_elem = *pages_used;
int rc = 0;
int maxpages;
while (length > 0) {
i = 0;
- maxpages = ARRAY_SIZE(ctx->sg_plaintext_data) - num_elem;
+ maxpages = to_max_pages - num_elem;
if (maxpages == 0) {
rc = -EFAULT;
goto out;
@@ -317,10 +318,11 @@ static int zerocopy_from_iter(struct sock *sk, struct
iov_iter *from,
while (copied) {
use = min_t(int, copied, PAGE_SIZE - offset);
- sg_set_page(&ctx->sg_plaintext_data[num_elem],
+ sg_set_page(&to[num_elem],
pages[i], use, offset);
- sg_unmark_end(&ctx->sg_plaintext_data[num_elem]);
- sk_mem_charge(sk, use);
+ sg_unmark_end(&to[num_elem]);
+ if (charge)
+ sk_mem_charge(sk, use);
offset = 0;
copied -= use;
@@ -331,8 +333,9 @@ static int zerocopy_from_iter(struct sock *sk, struct
iov_iter *from,
}
out:
- ctx->sg_plaintext_size = size;
- ctx->sg_plaintext_num_elem = num_elem;
+ *size_used = size;
+ *pages_used = num_elem;
+
return rc;
}
@@ -429,7 +432,11 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg,
size_t size)
if (full_record || eor) {
ret = zerocopy_from_iter(sk, &msg->msg_iter,
-try_to_copy);
+ try_to_copy, &ctx->sg_plaintext_num_elem,
+ &ctx->sg_plaintext_size,
+ ctx->sg_plaintext_data,
+ ARRAY_SIZE(ctx->sg_plaintext_data),
+ true);
if (ret)
goto fallback_to_reg_send;
--
2.9.5
[PATCH RFC 5/5] tls: Add receive path documentation
Add documentation on rx path setup and cmsg interface.
Signed-off-by: Dave Watson
---
Documentation/networking/tls.txt | 59 ++--
1 file changed, 57 insertions(+), 2 deletions(-)
diff --git a/Documentation/networking/tls.txt b/Documentation/networking/tls.txt
index 77ed006..d341016 100644
--- a/Documentation/networking/tls.txt
+++ b/Documentation/networking/tls.txt
@@ -48,6 +48,9 @@ the transmit and the receive into the kernel.
setsockopt(sock, SOL_TLS, TLS_TX, &crypto_info, sizeof(crypto_info));
+Transmit and receive are set separately, but the setup is the same, using
either
+TLS_TX or TLS_RX.
+
Sending TLS application data
@@ -79,6 +82,21 @@ for memory), or the encryption will always succeed. If
send() returns
-ENOMEM and some data was left on the socket buffer from a previous
call using MSG_MORE, the MSG_MORE data is left on the socket buffer.
+Receiving TLS application data
+--
+
+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.
+
Send TLS control messages
-
@@ -118,6 +136,43 @@ using a record of type @record_type.
Control message data should be provided unencrypted, and will be
encrypted by the kernel.
+Receiving TLS control messages
+--
+
+TLS control messages are passed in the userspace buffer, with message
+type passed via cmsg. If no cmsg buffer is provided, an error is
+returned if a control message is received. Data messages may be
+received without a cmsg buffer set.
+
+ char buffer[16384];
+ char cmsg[CMSG_SPACE(sizeof(unsigned char))];
+ struct msghdr msg = {0};
+ msg.msg_control = cmsg;
+ msg.msg_controllen = sizeof(cmsg);
+
+ struct iovec msg_iov;
+ msg_iov.iov_base = buffer;
+ msg_iov.iov_len = 16384;
+
+ msg.msg_iov = &msg_iov;
+ msg.msg_iovlen = 1;
+
+ int ret = recvmsg(sock, &msg, 0 /* flags */);
+
+ struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg);
+ if (cmsg->cmsg_level == SOL_TLS &&
+ cmsg->cmsg_type == TLS_GET_RECORD_TYPE) {
+ int record_type = *((unsigned char *)CMSG_DATA(cmsg));
+ // Do something with record_type, and control message data in
+ // buffer.
+ //
+ // Note that record_type may be == to application data (23).
+ } else {
+// Buffer contains application data.
+ }
+
+recv will never return data from mixed types of TLS records.
+
Integrating in to userspace TLS library
---
@@ -126,10 +181,10 @@ layer of a userspace TLS library.
A patchset to OpenSSL to use ktls as the record layer is here:
-https://github.com/Mellanox/tls-openssl
+https://github.com/Mellanox/openssl/commits/tls_rx
An example of calling send directly after a handshake using
gnutls. Since it doesn't implement a full record layer, control
messages are not supported:
-https://github.com/Mellanox/tls-af_ktls_tool
+https://github.com/ktls/af_ktls-tool/commits/RX
--
2.9.5
Re: [PATCH RFC 4/5] tls: RX path for ktls
Hi Dave,
On 03/08/18 18:50, Dave Watson wrote:
Add rx path for tls software implementation.
recvmsg, splice_read, and poll implemented.
An additional sockopt TLS_RX is added, with the same interface as
TLS_TX. Either TLX_RX or TLX_TX may be provided separately, or
together (with two different setsockopt calls with appropriate keys).
Control messages are passed via CMSG in a similar way to transmit.
If no cmsg buffer is passed, then only application data records
will be passed to userspace, and EIO is returned for other types of
alerts.
EBADMSG is passed for decryption errors, and E2BIG is passed for framing
errors. Both are unrecoverable.
I think E2BIG is for too long argument list. EMSGSIZE might be more
appropriate.
Also, we must check that the record is not too short (cipher specific).
For TLS1.2 with AES-GCM the minimum length is 8 (IV) + 16 (TAG).
The correct error for this case is EBADMSG, like a decryption failure.
Also, how about bad TLS version (e.g. not TLS1.2)?
A separate error type is required for bad version, because it triggers a
unique alert in libraries such as OpenSSL.
I thought of using EINVAL for bad version. What do you think?
I wonder if we should provide a more flexible method of obtaining errors
for the future.
Maybe use a special CMSG for errors?
This CMSG will be triggered only after the socket enters the error state.
+
+int tls_sw_recvmsg(struct sock *sk,
+ struct msghdr *msg,
+ size_t len,
+ int nonblock,
+ int flags,
+ int *addr_len)
+{
+ struct tls_context *tls_ctx = tls_get_ctx(sk);
+ struct tls_sw_context *ctx = tls_sw_ctx(tls_ctx);
+ unsigned char control;
+ struct strp_msg *rxm;
+ struct sk_buff *skb;
+ ssize_t copied = 0;
+ bool cmsg = false;
+ int err = 0;
+ long timeo;
Maybe try to read from the error queue here?
+
+ flags |= nonblock;
+
+ lock_sock(sk);
+
+ timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+ do {
+ bool zc = false;
+ int chunk = 0;
+
+ skb = tls_wait_data(sk, flags, timeo, &err);
+ if (!skb)
+ goto recv_end;
+
+ rxm = strp_msg(skb);
+ if (!cmsg) {
+ int cerr;
+
+ cerr = put_cmsg(msg, SOL_TLS, TLS_GET_RECORD_TYPE,
+ sizeof(ctx->control), &ctx->control);
+ cmsg = true;
+ control = ctx->control;
+ if (ctx->control != TLS_RECORD_TYPE_DATA) {
+ if (cerr || msg->msg_flags & MSG_CTRUNC) {
+ err = -EIO;
+ goto recv_end;
+ }
+ }
+ } else if (control != ctx->control) {
+ goto recv_end;
+ }
+
+ if (!ctx->decrypted) {
+ int page_count;
+ int to_copy;
+
+ page_count = iov_iter_npages(&msg->msg_iter,
+MAX_SKB_FRAGS);
+ to_copy = rxm->full_len - tls_ctx->rx.overhead_size;
+ if (to_copy <= len && page_count < MAX_SKB_FRAGS &&
+ likely(!(flags & MSG_PEEK))) {
+ struct scatterlist sgin[MAX_SKB_FRAGS + 1];
+ char unused[21];
+ int pages = 0;
+
+ zc = true;
+ sg_init_table(sgin, MAX_SKB_FRAGS + 1);
+ sg_set_buf(&sgin[0], unused, 13);
+
+ err = zerocopy_from_iter(sk, &msg->msg_iter,
+to_copy, &pages,
+&chunk, &sgin[1],
+MAX_SKB_FRAGS, false);
+ if (err < 0)
+ goto fallback_to_reg_recv;
+
+ err = decrypt_skb(sk, skb, sgin);
+ for (; pages > 0; pages--)
+ put_page(sg_page(&sgin[pages]));
+ if (err < 0) {
+ tls_err_abort(sk, EBADMSG);
+ goto recv_end;
+ }
+ } else {
+fallback_to_reg_recv:
+ err = decrypt_skb(sk, skb, NULL);
+ if (err < 0) {
+ tls_err_abort(sk, EBADMSG);
+ goto recv_end;
+
Re: [PATCH] crypto/ecc: Remove stack VLA usage
On Thu, Mar 8, 2018 at 1:43 AM, Tudor Ambarus
wrote:
> Hi, Kees,
>
>
> On 03/07/2018 11:56 PM, Kees Cook wrote:
>>
>> On the quest to remove all VLAs from the kernel[1], this switches to
>> a pair of kmalloc regions instead of using the stack. This also moves
>> the get_random_bytes() after all allocations (and drops the needless
>> "nbytes" variable).
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Kees Cook
>> ---
>> crypto/ecc.c | 23 +--
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/crypto/ecc.c b/crypto/ecc.c
>> index 18f32f2a5e1c..5bfa63603da0 100644
>> --- a/crypto/ecc.c
>> +++ b/crypto/ecc.c
>> @@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
>> unsigned int ndigits,
>> {
>> int ret = 0;
>> struct ecc_point *product, *pk;
>> - u64 priv[ndigits];
>> - u64 rand_z[ndigits];
>> - unsigned int nbytes;
>> + u64 *priv, *rand_z;
>> const struct ecc_curve *curve = ecc_get_curve(curve_id);
>> if (!private_key || !public_key || !curve) {
>> @@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int
>> curve_id, unsigned int ndigits,
>> goto out;
>> }
>> - nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
>> + priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL);
>> + if (!priv) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> - get_random_bytes(rand_z, nbytes);
>> + rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL);
>> + if (!rand_z) {
>> + ret = -ENOMEM;
>> + goto kfree_out;
>> + }
>> pk = ecc_alloc_point(ndigits);
>> if (!pk) {
>> ret = -ENOMEM;
>> - goto out;
>> + goto kfree_out;
>> }
>> product = ecc_alloc_point(ndigits);
>> @@ -1051,6 +1057,8 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
>> unsigned int ndigits,
>> goto err_alloc_product;
>> }
>> + get_random_bytes(rand_z, ndigits << ECC_DIGITS_TO_BYTES_SHIFT);
>> +
>> ecc_swap_digits(public_key, pk->x, ndigits);
>> ecc_swap_digits(&public_key[ndigits], pk->y, ndigits);
>> ecc_swap_digits(private_key, priv, ndigits);
>> @@ -1065,6 +1073,9 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
>> unsigned int ndigits,
>> ecc_free_point(product);
>> err_alloc_product:
>> ecc_free_point(pk);
>> +kfree_out:
>> + kfree(priv);
>
>
> I think we should use kzfree here.
>
>> + kfree(rand_z);
>
>
> Probably here too.
Ah yeah, good idea. I'll send a v2.
> Looks like there are few intermediate buffers in ecc
> that should be zeroized as well.
Can you send a patch for those?
Thanks!
-Kees
>
> Best,
> ta
>>
>> out:
>> return ret;
>> }
>>
>
--
Kees Cook
Pixel Security
[PATCH v2] crypto/ecc: Remove stack VLA usage
On the quest to remove all VLAs from the kernel[1], this switches to
a pair of kmalloc regions instead of using the stack. This also moves
the get_random_bytes() after all allocations (and drops the needless
"nbytes" variable).
[1] https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Kees Cook
---
crypto/ecc.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/crypto/ecc.c b/crypto/ecc.c
index 18f32f2a5e1c..9c066b5ac12d 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
unsigned int ndigits,
{
int ret = 0;
struct ecc_point *product, *pk;
- u64 priv[ndigits];
- u64 rand_z[ndigits];
- unsigned int nbytes;
+ u64 *priv, *rand_z;
const struct ecc_curve *curve = ecc_get_curve(curve_id);
if (!private_key || !public_key || !curve) {
@@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
unsigned int ndigits,
goto out;
}
- nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
+ priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ ret = -ENOMEM;
+ goto out;
+ }
- get_random_bytes(rand_z, nbytes);
+ rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL);
+ if (!rand_z) {
+ ret = -ENOMEM;
+ goto kfree_out;
+ }
pk = ecc_alloc_point(ndigits);
if (!pk) {
ret = -ENOMEM;
- goto out;
+ goto kfree_out;
}
product = ecc_alloc_point(ndigits);
@@ -1051,6 +1057,8 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
unsigned int ndigits,
goto err_alloc_product;
}
+ get_random_bytes(rand_z, ndigits << ECC_DIGITS_TO_BYTES_SHIFT);
+
ecc_swap_digits(public_key, pk->x, ndigits);
ecc_swap_digits(&public_key[ndigits], pk->y, ndigits);
ecc_swap_digits(private_key, priv, ndigits);
@@ -1065,6 +1073,9 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
unsigned int ndigits,
ecc_free_point(product);
err_alloc_product:
ecc_free_point(pk);
+kfree_out:
+ kzfree(priv);
+ kzfree(rand_z);
out:
return ret;
}
--
2.7.4
--
Kees Cook
Pixel Security
Re: [PATCH RFC 4/5] tls: RX path for ktls
On 03/08/18 09:48 PM, Boris Pismenny wrote:
> Hi Dave,
>
> On 03/08/18 18:50, Dave Watson wrote:
> > Add rx path for tls software implementation.
> >
> > recvmsg, splice_read, and poll implemented.
> >
> > An additional sockopt TLS_RX is added, with the same interface as
> > TLS_TX. Either TLX_RX or TLX_TX may be provided separately, or
> > together (with two different setsockopt calls with appropriate keys).
> >
> > Control messages are passed via CMSG in a similar way to transmit.
> > If no cmsg buffer is passed, then only application data records
> > will be passed to userspace, and EIO is returned for other types of
> > alerts.
> >
> > EBADMSG is passed for decryption errors, and E2BIG is passed for framing
> > errors. Both are unrecoverable.
>
> I think E2BIG is for too long argument list. EMSGSIZE might be more
> appropriate.
Sounds good.
> Also, we must check that the record is not too short (cipher specific).
> For TLS1.2 with AES-GCM the minimum length is 8 (IV) + 16 (TAG).
> The correct error for this case is EBADMSG, like a decryption failure.
>
> Also, how about bad TLS version (e.g. not TLS1.2)?
> A separate error type is required for bad version, because it triggers a
> unique alert in libraries such as OpenSSL.
> I thought of using EINVAL for bad version. What do you think?
Ah, I did not realize there was a separate alert for that, sounds good.
>
> I wonder if we should provide a more flexible method of obtaining errors for
> the future.
> Maybe use a special CMSG for errors?
> This CMSG will be triggered only after the socket enters the error state.
I'm not opposed to this in principle, but without a concrete use am
hesitant to add it. I don't know of any other error codes that could
be returned besides the ones discussed above.
> >
> > +
> > +int tls_sw_recvmsg(struct sock *sk,
> > + struct msghdr *msg,
> > + size_t len,
> > + int nonblock,
> > + int flags,
> > + int *addr_len)
> > +{
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context *ctx = tls_sw_ctx(tls_ctx);
> > + unsigned char control;
> > + struct strp_msg *rxm;
> > + struct sk_buff *skb;
> > + ssize_t copied = 0;
> > + bool cmsg = false;
> > + int err = 0;
> > + long timeo;
> Maybe try to read from the error queue here?
Sure.
