netdev - Re: [PATCH net-next v3 1/6] cxgb4/chcr : Register to tls add and del callback

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: <57eb8055-a4ad-1afd-b4f4-07bbeaa2b6f6@mellanox.com>
Date:   Sun, 1 Mar 2020 10:36:01 +0200
From:   Boris Pismenny <borisp@...lanox.com>
To:     Rohit Maheshwari <rohitm@...lsio.com>, netdev@...r.kernel.org,
        davem@...emloft.net, herbert@...dor.apana.org.au
Cc:     secdev@...lsio.com, varun@...lsio.com, kuba@...nel.org
Subject: Re: [PATCH net-next v3 1/6] cxgb4/chcr : Register to tls add and del
 callback

Hi Rohit,

On 2/29/2020 3:24 AM, Rohit Maheshwari wrote:
> A new macro is defined to enable ktls tx offload support on Chelsio
> T6 adapter. And if this macro is enabled, cxgb4 will send mailbox to
> enable or disable ktls settings on HW.
> In chcr, enabled tx offload flag in netdev and registered tls_dev_add
> and tls_dev_del.
>
> v1->v2:
> - mark tcb state to close in tls_dev_del.
> - u_ctx is now picked from adapter structure.
> - clear atid in case of failure.
> - corrected ULP_CRYPTO_KTLS_INLINE value.
>
> v2->v3:
> - add empty line after variable declaration.
> - local variable declaration in reverse christmas tree ordering.
>
> Signed-off-by: Rohit Maheshwari <rohitm@...lsio.com>
> ---
...
> +
> +/*
> + * chcr_ktls_dev_add:  call back for tls_dev_add.
> + * Create a tcb entry for TP. Also add l2t entry for the connection. And
> + * generate keys & save those keys locally.
> + * @netdev - net device.
> + * @tls_cts - tls context.
> + * @direction - TX/RX crypto direction
> + * return: SUCCESS/FAILURE.
> + */
> +static int chcr_ktls_dev_add(struct net_device *netdev, struct sock *sk,
> +			     enum tls_offload_ctx_dir direction,
> +			     struct tls_crypto_info *crypto_info,
> +			     u32 start_offload_tcp_sn)
> +{
> +	struct tls_context *tls_ctx = tls_get_ctx(sk);
> +	struct chcr_ktls_ofld_ctx_tx *tx_ctx;
> +	struct chcr_ktls_info *tx_info;
> +	struct dst_entry *dst;
> +	struct adapter *adap;
> +	struct port_info *pi;
> +	struct neighbour *n;
> +	u8 daaddr[16];
> +	int ret = -1;
> +
> +	tx_ctx = chcr_get_ktls_tx_context(tls_ctx);
> +
> +	pi = netdev_priv(netdev);
> +	adap = pi->adapter;
> +	if (direction == TLS_OFFLOAD_CTX_DIR_RX) {
> +		pr_err("not expecting for RX direction\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	if (tx_ctx->chcr_info) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	tx_info = kvzalloc(sizeof(*tx_info), GFP_KERNEL);
> +	if (!tx_info) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	spin_lock_init(&tx_info->lock);
> +
> +	/* clear connection state */
> +	spin_lock(&tx_info->lock);
> +	tx_info->connection_state = KTLS_CONN_CLOSED;
> +	spin_unlock(&tx_info->lock);
> +
> +	tx_info->sk = sk;
> +	/* initialize tid and atid to -1, 0 is a also a valid id. */
> +	tx_info->tid = -1;
> +	tx_info->atid = -1;
> +
> +	tx_info->adap = adap;
> +	tx_info->netdev = netdev;
> +	tx_info->tx_chan = pi->tx_chan;
> +	tx_info->smt_idx = pi->smt_idx;
> +	tx_info->port_id = pi->port_id;
> +
> +	tx_info->rx_qid = chcr_get_first_rx_qid(adap);
> +	if (unlikely(tx_info->rx_qid < 0))
> +		goto out2;
> +
> +	tx_info->prev_seq = start_offload_tcp_sn;
> +	tx_info->tcp_start_seq_number = start_offload_tcp_sn;
> +
> +	/* get peer ip */
> +	if (sk->sk_family == AF_INET ||
> +	    (sk->sk_family == AF_INET6 && !sk->sk_ipv6only &&
> +	     ipv6_addr_type(&sk->sk_v6_daddr) == IPV6_ADDR_MAPPED)) {
> +		memcpy(daaddr, &sk->sk_daddr, 4);
> +	} else {
> +		goto out2;
> +	}
> +
> +	/* get the l2t index */
> +	dst = sk_dst_get(sk);
> +	if (!dst) {
> +		pr_err("DST entry not found\n");
> +		goto out2;
> +	}
> +	n = dst_neigh_lookup(dst, daaddr);
> +	if (!n || !n->dev) {
> +		pr_err("neighbour not found\n");
> +		dst_release(dst);
> +		goto out2;
> +	}
> +	tx_info->l2te  = cxgb4_l2t_get(adap->l2t, n, n->dev, 0);

I see that you make an effort to obtain the the L2 tunnel, but did you test it? I would expect that offload would fail for such a connection as the KTLS code would not find the lower device with the offload capability..

If this doesn't work, better remove it, until the stack supports such functionality. Then, you wouldn't need to retrospectively obtain these parameters. Instead, you could just implement the proper flow by working with the L2 tunnel.

> +
> +	neigh_release(n);
> +	dst_release(dst);
> +
> +	if (!tx_info->l2te) {
> +		pr_err("l2t entry not found\n");
> +		goto out2;
> +	}
> +
> +	tx_ctx->chcr_info = tx_info;
> +
> +	/* create a filter and call cxgb4_l2t_send to send the packet out, which
> +	 * will take care of updating l2t entry in hw if not already done.
> +	 */
> +	ret = chcr_setup_connection(sk, tx_info);
> +	if (ret)
> +		goto out2;
> +
> +	return 0;
> +out2:
> +	kvfree(tx_info);
> +out:
> +	return ret;
> +}
> +
...