Re: [PATCH net] tls: check the right open record in TLS_TX_MAX_PAYLOAD_LEN

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The TLS_HW check added to do_tls_setsockopt_tx_payload_len()
  (net/tls/tls_main.c) only rejects the change when…

--

tls: check the right open record in TLS_TX_MAX_PAYLOAD_LEN

This patch changes do_tls_setsockopt_tx_payload_len() to look up the open
record based on ctx->tx_conf. For TLS_HW it now checks
tls_offload_context_tx::open_record, instead of casting priv_ctx_tx to
struct tls_sw_context_tx. The aim is to stop HW offloaded TX from building
records larger than a limit that setsockopt has accepted.

> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index fbb274287aa5..8f11b8629492 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -837,12 +837,23 @@ static int do_tls_setsockopt_tx_payload_len(struct sock *sk, sockptr_t optval,
>  					    unsigned int optlen)
>  {
>  	struct tls_context *ctx = tls_get_ctx(sk);
> -	struct tls_sw_context_tx *sw_ctx = tls_sw_ctx_tx(ctx);
>  	u16 value;
>  	bool tls_13 = ctx->prot_info.version == TLS_1_3_VERSION;
>  
> -	if (sw_ctx && sw_ctx->open_rec)
> -		return -EBUSY;
> +	/* Both TX paths size the record they are filling against
> +	 * tx_max_payload_len, so refuse to change it under an open record.
> +	 * priv_ctx_tx is a different struct for each TX configuration.
> +	 */
> +	switch (ctx->tx_conf) {
> +	case TLS_SW:
> +		if (tls_sw_ctx_tx(ctx)->open_rec)
> +			return -EBUSY;
> +		break;
> +	case TLS_HW:
> +		if (tls_offload_ctx_tx(ctx)->open_record)
> +			return -EBUSY;
> +		break;
> +	}

[Severity: Medium]
Can a concurrent HW send still produce records larger than the new limit
after this check passes?

The setsockopt path takes only the socket lock and not ctx->tx_lock:

net/tls/tls_main.c:do_tls_setsockopt() {
    ...
	case TLS_TX_MAX_PAYLOAD_LEN:
		lock_sock(sk);
		rc = do_tls_setsockopt_tx_payload_len(sk, optval, optlen);
		release_sock(sk);
    ...
}

On the device side, tls_device_sendmsg() holds tx_lock for the whole
send. However, tls_push_data() reads the limit only once, before its loop:

net/tls/tls_device.c:tls_push_data() {
    ...
	max_open_record_len = tls_ctx->tx_max_payload_len +
			      prot->prepend_size;
	do {
		rc = tls_do_allocation(sk, ctx, pfrag, prot->prepend_size);
		if (unlikely(rc)) {
			rc = sk_stream_wait_memory(sk, &timeo);
    ...
		copy = min_t(size_t, size, max_open_record_len - record->len);
    ...
		if (done || record->len >= max_open_record_len ||
    ...
}

Inside the loop, the socket lock can be dropped while open_record is NULL.
This can happen in two places:

  tls_push_record()
    offload_ctx->open_record = NULL;
    tls_push_sg()->tcp_sendmsg_locked()->sk_stream_wait_memory()
      sk_wait_event()->release_sock()

and also when tls_do_allocation() fails with no open record and
tls_push_data() calls sk_stream_wait_memory() directly.

If a second thread calls setsockopt(SOL_TLS, TLS_TX_MAX_PAYLOAD_LEN) with a
smaller value during either window, the new TLS_HW check sees open_record
as NULL and the call returns 0. The blocked send then continues to build
records against the old, larger max_open_record_len.

The SW path looks different here. tls_sw_sendmsg_locked() re-reads the
limit after open_rec is allocated on every iteration:

	record_room = tls_ctx->tx_max_payload_len - msg_pl->sg.size;

Given that, is the new comment accurate for TLS_HW? It says "Both TX paths
size the record they are filling against tx_max_payload_len", but the
device path uses a cached copy.

Would it make sense to recompute max_open_record_len from
tls_ctx->tx_max_payload_len each time tls_push_data() starts a new record?
Another option would be to take tx_lock in the TLS_TX_MAX_PAYLOAD_LEN
setsockopt path before lock_sock().

>  
>  	if (sockptr_is_null(optval) || optlen != sizeof(value))
>  		return -EINVAL;

[ ... ]

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