Re: [PATCH net-next v17 01/15] net: tls: reject TLS 1.3 offload in chcr_ktls and nfp drivers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Claim: the two new `crypto_info->version != TLS_1_2_VERSION`
  guards in chcr_ktls_dev_add() and nfp_net_tls_add() are dead code,…
- [Low] Claim: the patch implements the same rejection with two different
  semantics — nfp_net_tls_add() returns -EOPNOTSUPP while…

--

net: tls: reject TLS 1.3 offload in chcr_ktls and nfp drivers

The patch adds an early version check to the ->tls_dev_add callbacks of
the chcr_ktls and nfp drivers so that a crypto_info whose version is not
TLS_1_2_VERSION is refused before any hardware state is programmed.

A couple of questions on the two new checks below.

> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> index f5acd4be1e69d..29e108ce67645 100644
> --- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> +++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> @@ -431,6 +431,9 @@ static int chcr_ktls_dev_add(struct net_device *netdev, struct sock *sk,
>  	atomic64_inc(&port_stats->ktls_tx_connection_open);
>  	u_ctx = adap->uld[CXGB4_ULD_KTLS].handle;
>  
> +	if (crypto_info->version != TLS_1_2_VERSION)
> +		goto out;
> +

[Severity: Low]
Is this new check in chcr_ktls_dev_add() reachable as of this commit?  At
this point net/tls still filters the version before calling
->tls_dev_add:

net/tls/tls_device.c:tls_set_device_offload() {
	...
	if (crypto_info->version != TLS_1_2_VERSION) {
		rc = -EOPNOTSUPP;
		goto release_netdev;
	}
	...
}

and the same for the receive side in tls_set_device_offload_rx():

	if (ctx->crypto_recv.info.version != TLS_1_2_VERSION)
		return -EOPNOTSUPP;

The guards do become live once those two core checks are dropped later in
the series, and the ordering here looks bisect-safe, so this is only a
changelog question: could the commit message say that this is a
prerequisite for the later core change that starts accepting TLS 1.3
crypto_info, rather than describing it as preventing attempts that the
core currently already blocks?

For nfp the guard is also not covered by nfp_net_cipher_supported(), which
only looks at cipher_type, so a TLS 1.3 AES-GCM-128 crypto_info would pass
that helper once the core stops filtering versions.

[Severity: Low]
This isn't a bug, but the two halves of the patch report the same
condition differently.  nfp_net_tls_add() returns -EOPNOTSUPP while
chcr_ktls_dev_add() jumps to the shared failure label:

out:
	atomic64_inc(&port_stats->ktls_tx_connection_fail);
	return -1;

so chcr reports -EPERM for an unsupported version and also counts the
request in both ktls_tx_connection_open and ktls_tx_connection_fail.

The errno does not reach userspace on the only reachable path, since
do_tls_setsockopt_conf() treats any nonzero return from
tls_set_device_offload() on an initial install as a software fallback:

net/tls/tls_main.c:do_tls_setsockopt_conf() {
	...
	rc = tls_set_device_offload(sk);
	conf = TLS_HW;
	if (!rc) {
	...
	} else {
		rc = tls_set_sw_offload(sk, 1, update ? crypto_info : NULL);
	...
}

and the counter behaviour matches the existing reject branches in the same
function (RX direction, chcr_get_ktls_tx_info(), u_ctx->detach), which all
use the same out: label.  Would it still be preferable to return
-EOPNOTSUPP here to match nfp_net_tls_add() and the core?

>  	if (direction == TLS_OFFLOAD_CTX_DIR_RX) {
>  		pr_err("not expecting for RX direction\n");
>  		goto out;
> diff --git a/drivers/net/ethernet/netronome/nfp/crypto/tls.c b/drivers/net/ethernet/netronome/nfp/crypto/tls.c
> index 9983d7aa2b9cd..13864c6a55dce 100644
> --- a/drivers/net/ethernet/netronome/nfp/crypto/tls.c
> +++ b/drivers/net/ethernet/netronome/nfp/crypto/tls.c
> @@ -287,6 +287,9 @@ nfp_net_tls_add(struct net_device *netdev, struct sock *sk,
>  	BUILD_BUG_ON(offsetof(struct nfp_net_tls_offload_ctx, rx_end) >
>  		     TLS_DRIVER_STATE_SIZE_RX);
>  
> +	if (crypto_info->version != TLS_1_2_VERSION)
> +		return -EOPNOTSUPP;
> +
>  	if (!nfp_net_cipher_supported(nn, crypto_info->cipher_type, direction))
>  		return -EOPNOTSUPP;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917224355.2288021-1-rjethwani%40purestorage.com