Re: [PATCH net-next 5/6] tls: RX path for ktls

[Boris Pismenny <borisp@mellanox.com>]

On 3/20/2018 7:54 PM, 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 EMSGSIZE is passed for
> framing errors (either framing too big *or* too small with crypto
> overhead). EINVAL is returned for TLS versions that do not match the
> original setsockopt call.  All 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 <davejwatson@fb.com>
> ---
...
> +
> +static int tls_read_size(struct strparser *strp, struct sk_buff *skb)
> +{
> +	struct tls_context *tls_ctx = tls_get_ctx(strp->sk);
> +	struct tls_sw_context *ctx = tls_sw_ctx(tls_ctx);
> +	char header[tls_ctx->rx.prepend_size];
> +	struct strp_msg *rxm = strp_msg(skb);
> +	size_t cipher_overhead;
> +	size_t data_len = 0;
> +	int ret;
> +
> +	/* Verify that we have a full TLS header, or wait for more data */
> +	if (rxm->offset + tls_ctx->rx.prepend_size > skb->len)
> +		return 0;
> +
> +	/* Linearize header to local buffer */
> +	ret = skb_copy_bits(skb, rxm->offset, header, tls_ctx->rx.prepend_size);
> +
> +	if (ret < 0)
> +		goto read_failure;
> +
> +	ctx->control = header[0];
> +
> +	data_len = ((header[4] & 0xFF) | (header[3] << 8));
> +
> +	cipher_overhead = tls_ctx->rx.tag_size + tls_ctx->rx.iv_size;
> +
> +	if (data_len > TLS_MAX_PAYLOAD_SIZE + cipher_overhead) {
> +		ret = -EMSGSIZE;
> +		goto read_failure;
> +	}
> +	if (data_len < cipher_overhead) {
> +		ret = -EMSGSIZE;

I think this should be considered EBADMSG, because this error is cipher 
dependent. At least, that's what happens within OpenSSL. Also, EMSGSIZE 
is usually used only for too long messages.

> +		goto read_failure;
> +	}
> +
> +	if (header[1] != TLS_VERSION_MINOR(tls_ctx->crypto_recv.version) ||
> +	    header[2] != TLS_VERSION_MAJOR(tls_ctx->crypto_recv.version)) {
> +		ret = -EINVAL;
> +		goto read_failure;
> +	}
> +
> +	return data_len + TLS_HEADER_SIZE;
> +
> +read_failure:
> +	tls_err_abort(strp->sk, ret);
> +
> +	return ret;
> +}
> +
...