From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from mail-db5eur01on0043.outbound.protection.outlook.com ([104.47.2.43]:24029
        "EHLO EUR01-DB5-obe.outbound.protection.outlook.com"
        rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
        id S1751317AbeCUFVH (ORCPT <rfc822;netdev@vger.kernel.org>);
        Wed, 21 Mar 2018 01:21:07 -0400
Subject: Re: [PATCH net-next 5/6] tls: RX path for ktls
To: Dave Watson <davejwatson@fb.com>,
        "David S. Miller" <davem@davemloft.net>,
        Tom Herbert <tom@quantonium.net>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        herbert@gondor.apana.org.au, linux-crypto@vger.kernel.org,
        netdev@vger.kernel.org
Cc: Atul Gupta <atul.gupta@chelsio.com>,
        Vakul Garg <vakul.garg@nxp.com>,
        Hannes Frederic Sowa <hannes@stressinduktion.org>,
        Steffen Klassert <steffen.klassert@secunet.com>,
        John Fastabend <john.fastabend@gmail.com>,
        Daniel Borkmann <daniel@iogearbox.net>
References: <cover.1521567760.git.davejwatson@fb.com>
 <20180320175434.GA23938@davejwatson-mba.local>
From: Boris Pismenny <borisp@mellanox.com>
Message-ID: <95b3174b-c860-3c87-2c8e-0917ccb72f1d@mellanox.com>
Date: Wed, 21 Mar 2018 07:20:55 +0200
MIME-Version: 1.0
In-Reply-To: <20180320175434.GA23938@davejwatson-mba.local>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>



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;
> +}
> +
...