netdev - RE: [PATCH net-next,v3] net/tls: Calculate nsg for zerocopy path without skb_cow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB7PR04MB425253CED3A31CF0D5919D2F8B270@DB7PR04MB4252.eurprd04.prod.outlook.com>
Date:   Tue, 7 Aug 2018 16:10:50 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     Doron Roberts-Kedes <doronrk@...com>,
        "David S . Miller" <davem@...emloft.net>
CC:     Dave Watson <davejwatson@...com>,
        Boris Pismenny <borisp@...lanox.com>,
        Aviad Yehezkel <aviadye@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next,v3] net/tls: Calculate nsg for zerocopy path
 without skb_cow_data.



> -----Original Message-----
> From: Doron Roberts-Kedes [mailto:doronrk@...com]
> Sent: Tuesday, August 7, 2018 1:18 AM
> To: David S . Miller <davem@...emloft.net>
> Cc: Vakul Garg <vakul.garg@....com>; Dave Watson
> <davejwatson@...com>; Boris Pismenny <borisp@...lanox.com>; Aviad
> Yehezkel <aviadye@...lanox.com>; netdev@...r.kernel.org; Doron
> Roberts-Kedes <doronrk@...com>
> Subject: [PATCH net-next,v3] net/tls: Calculate nsg for zerocopy path
> without skb_cow_data.
> 
> decrypt_skb fails if the number of sg elements required to map is
> greater than MAX_SKB_FRAGS. As noted by Vakul Garg, nsg must always be
> calculated, but skb_cow_data adds unnecessary memcpy's for the zerocopy
> case.
> 
> The new function skb_nsg calculates the number of scatterlist elements
> required to map the skb without the extra overhead of skb_cow_data. This
> function mimics the structure of skb_to_sgvec.
> 
> Fixes: c46234ebb4d1 ("tls: RX path for ktls")
> Signed-off-by: Doron Roberts-Kedes <doronrk@...com>
> ---
>  net/tls/tls_sw.c | 96
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index ff3a6904a722..eb87f931a0d6 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -43,6 +43,80 @@
> 
>  #define MAX_IV_SIZE	TLS_CIPHER_AES_GCM_128_IV_SIZE
> 
> +static int __skb_nsg(struct sk_buff *skb, int offset, int len,
> +		     unsigned int recursion_level)
> +{
> +	int start = skb_headlen(skb);
> +	int i, copy = start - offset;
> +	struct sk_buff *frag_iter;
> +	int elt = 0;
> +
> +	if (unlikely(recursion_level >= 24))
> +		return -EMSGSIZE;
> +
> +	if (copy > 0) {
> +		if (copy > len)
> +			copy = len;
> +		elt++;
> +		len -= copy;
> +		if (len == 0)
> +			return elt;
> +		offset += copy;
> +	}
> +
> +	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> +		int end;
> +
> +		WARN_ON(start > offset + len);
> +
> +		end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
> +		copy = end - offset;
> +		if (copy > 0) {
> +			if (copy > len)
> +				copy = len;
> +			elt++;
> +			len -= copy;
> +			if (len == 0)
> +				return elt;
> +			offset += copy;
> +		}
> +		start = end;
> +	}
> +
> +	skb_walk_frags(skb, frag_iter) {
> +		int end, ret;
> +
> +		WARN_ON(start > offset + len);
> +
> +		end = start + frag_iter->len;
> +		copy = end - offset;
> +		if (copy > 0) {
> +			if (copy > len)
> +				copy = len;
> +			ret = __skb_nsg(frag_iter, offset - start, copy,
> +					recursion_level + 1);
> +			if (unlikely(ret < 0))
> +				return ret;
> +			elt += ret;
> +			len -= copy;
> +			if (len == 0)
> +				return elt;
> +			offset += copy;
> +		}
> +		start = end;
> +	}
> +	BUG_ON(len);
> +	return elt;
> +}
> +
> +/* Return the number of scatterlist elements required to completely map
> the
> + * skb, or -EMSGSIZE if the recursion depth is exceeded.
> + */
> +static int skb_nsg(struct sk_buff *skb, int offset, int len)
> +{
> +	return __skb_nsg(skb, offset, len, 0);
> +}
> +
>  static int tls_do_decryption(struct sock *sk,
>  			     struct scatterlist *sgin,
>  			     struct scatterlist *sgout,
> @@ -693,7 +767,7 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb,
>  	struct scatterlist sgin_arr[MAX_SKB_FRAGS + 2];
>  	struct scatterlist *sgin = &sgin_arr[0];
>  	struct strp_msg *rxm = strp_msg(skb);
> -	int ret, nsg = ARRAY_SIZE(sgin_arr);
> +	int ret, nsg;
>  	struct sk_buff *unused;
> 
>  	ret = skb_copy_bits(skb, rxm->offset + TLS_HEADER_SIZE,
> @@ -704,11 +778,27 @@ int decrypt_skb(struct sock *sk, struct sk_buff
> *skb,
> 
>  	memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
>  	if (!sgout) {
> -		nsg = skb_cow_data(skb, 0, &unused) + 1;
> +		nsg = skb_cow_data(skb, 0, &unused);
> +	} else {
> +		nsg = skb_nsg(skb,
> +			      rxm->offset + tls_ctx->rx.prepend_size,
> +			      rxm->full_len - tls_ctx->rx.prepend_size);
> +		if (nsg <= 0)
> +			return nsg;
> +	}
> +
> +	// We need one extra for ctx->rx_aad_ciphertext
> +	nsg++;
> +
> +	if (nsg > ARRAY_SIZE(sgin_arr)) {
>  		sgin = kmalloc_array(nsg, sizeof(*sgin), sk->sk_allocation);
> -		sgout = sgin;
> +		if (!sgin)
> +			return -ENOMEM;
>  	}
> 
> +	if (!sgout)
> +		sgout = sgin;
> +
>  	sg_init_table(sgin, nsg);
>  	sg_set_buf(&sgin[0], ctx->rx_aad_ciphertext, TLS_AAD_SPACE_SIZE);
> 
> --
> 2.17.1

I prefer my name be removed from commit message.

Reported-by: Vakul Garg <Vakul.garg@....com>
Reviewed-by: Vakul Garg <Vakul.garg@....com>
Tested-by: Vakul Garg <Vakul.garg@....com>