[PATCH net] tls: fix TX context confusion in the max payload size setsockopt

[PATCH net] tls: fix TX context confusion in the max payload size setsockopt

[Jiayuan Chen]

do_tls_setsockopt_tx_payload_len() refuses to resize records while one is
open, but reaches for the open record through tls_sw_ctx_tx(), an unchecked
cast of ctx->priv_ctx_tx.  Under device offload that pointer is a
tls_offload_context_tx, so the check reads a field of the wrong struct: it
returns EBUSY on whatever happens to be there, and never sees the record
that really is open, which lets the limit be lowered mid-record.

Dispatch on tx_conf.  Offload was in scope from the start, the same commit
taught tls_push_data() to honour tx_max_payload_len.

Fixes: 82cb5be6ad64 ("net/tls: support setting the maximum payload size")
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
base on my netdevsim + tls (in progress)
https://lore.kernel.org/netdev/20260728125658.390500-1-jiayuan.chen@linux.dev/

Previous finding:
b17cf742eaad ("tls: device: fix out-of-bounds write in tls_append_frag()")
---
 net/tls/tls_main.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index fbb274287aa5..e4f288c1fa34 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -833,15 +833,29 @@ static int do_tls_setsockopt_no_pad(struct sock *sk, sockptr_t optval,
 	return rc;
 }
 
+/* priv_ctx_tx holds a different structure on each TX path, so tx_conf has to
+ * say which open record to look at.
+ */
+static bool tls_tx_record_is_open(struct tls_context *ctx)
+{
+	switch (ctx->tx_conf) {
+	case TLS_SW:
+		return !!tls_sw_ctx_tx(ctx)->open_rec;
+	case TLS_HW:
+		return !!tls_offload_ctx_tx(ctx)->open_record;
+	default:
+		return false;
+	}
+}
+
 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)
+	if (tls_tx_record_is_open(ctx))
 		return -EBUSY;
 
 	if (sockptr_is_null(optval) || optlen != sizeof(value))
-- 
2.43.0

Re: [PATCH net] tls: fix TX context confusion in the max payload size setsockopt

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
    tls: fix TX context confusion in the max payload size setsockopt

The patch replaces the unchecked tls_sw_ctx_tx() dereference in
do_tls_setsockopt_tx_payload_len() with a new helper,
tls_tx_record_is_open(), that dispatches on ctx->tx_conf so the TLS_HW
path looks at tls_offload_ctx_tx(ctx)->open_record instead of a field of
the wrong structure.  The rest of the option handling is unchanged.

> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index fbb274287aa5..e4f288c1fa34 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -833,15 +833,29 @@ static int do_tls_setsockopt_no_pad(struct sock *sk, sockptr_t optval,
>  	return rc;
>  }
>
> +/* priv_ctx_tx holds a different structure on each TX path, so tx_conf has to
> + * say which open record to look at.
> + */
> +static bool tls_tx_record_is_open(struct tls_context *ctx)
> +{
> +	switch (ctx->tx_conf) {
> +	case TLS_SW:
> +		return !!tls_sw_ctx_tx(ctx)->open_rec;
> +	case TLS_HW:
> +		return !!tls_offload_ctx_tx(ctx)->open_record;
> +	default:
> +		return false;
> +	}
> +}
> +
>  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)
> +	if (tls_tx_record_is_open(ctx))
>  		return -EBUSY;

[Severity: High]
For the TLS_HW arm, is open_record enough to tell that no record is being
built?  do_tls_setsockopt() calls this helper with only the socket lock
held:

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);
}

but the offload TX path is serialized by ctx->tx_lock, and it drops the
socket lock at a point where open_record is already NULL:

net/tls/tls_device.c:tls_push_record() {
	offload_ctx->open_record = NULL;
	...
	return tls_push_sg(sk, ctx, offload_ctx->sg_tx_data, 0, flags);
}

tls_push_sg() calls tcp_sendmsg_locked(), which for a blocking sendmsg()
with a full send buffer waits in sk_stream_wait_memory() and releases the
socket lock there.  tls_push_data() has a second such wait on the
tls_do_allocation() failure path.

So can a concurrent setsockopt() see open_record == NULL, take the "not
open" branch and publish a smaller ctx->tx_max_payload_len while a sender
is parked mid-sendmsg holding tx_lock?

If so, does the offload path then keep using the old limit for the rest of
that sendmsg?  tls_push_data() latches it once, before the loop, and never
refreshes it:

net/tls/tls_device.c:tls_push_data() {
	max_open_record_len = tls_ctx->tx_max_payload_len +
			      prot->prepend_size;
	do {
		...
		copy = min_t(size_t, size, max_open_record_len - record->len);
}

That would mean setsockopt returns 0 while records larger than the new
limit continue to go out, which does not match what
Documentation/networking/tls.rst says about TLS_TX_MAX_PAYLOAD_LEN ("the
kernel enforces the specified limit on all outgoing TLS records.  No
plaintext fragment will exceed this size").

There is a follow-on question about the next call.  If the sender leaves a
record open across the syscall (MSG_MORE with record->num_frags <
MAX_SKB_FRAGS - 1), the next tls_push_data() latches the new, smaller
max_open_record_len while record->len is still the old, larger value.
max_open_record_len is u32 and record->len is int, so does

	copy = min_t(size_t, size, max_open_record_len - record->len);

wrap around here and drop the record-size clamp entirely, letting
tls_append_frag() grow the record past the 2^14 plaintext maximum before
"record->len >= max_open_record_len" closes it?

Note that tls_sw_sendmsg_locked() re-reads the limit on every iteration:

net/tls/tls_sw.c:tls_sw_sendmsg_locked() {
		record_room = tls_ctx->tx_max_payload_len - msg_pl->sg.size;
}

so the latched offload value looks like the odd one out.

Would it be better to serialize the check and the store against the lock
that actually protects record building?  Since tls_device_sendmsg() takes
tx_lock before lock_sock:

net/tls/tls_device.c:tls_device_sendmsg() {
	mutex_lock(&tls_ctx->tx_lock);
	lock_sock(sk);
}

tx_lock is the outer lock, so taking it inside the already socket-locked
helper is not an option; the acquisition in do_tls_setsockopt() would have
to be reordered (or the helper made to wait for in-flight senders, as
tls_sk_proto_cleanup() does on sk_write_pending).

One smaller point: the plain read of tls_ctx->tx_max_payload_len in
tls_push_data() against the plain write in
do_tls_setsockopt_tx_payload_len() is an unannotated data race that KCSAN
would flag.
-- 
pw-bot: cr

Re: [PATCH net] tls: fix TX context confusion in the max payload size setsockopt

[Jiayuan Chen]

on 9/1/26 11:19 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
>      tls: fix TX context confusion in the max payload size setsockopt
>
> The patch replaces the unchecked tls_sw_ctx_tx() dereference in
> do_tls_setsockopt_tx_payload_len() with a new helper,
> tls_tx_record_is_open(), that dispatches on ctx->tx_conf so the TLS_HW
> path looks at tls_offload_ctx_tx(ctx)->open_record instead of a field of
> the wrong structure.  The rest of the option handling is unchanged.
>
>> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
>> index fbb274287aa5..e4f288c1fa34 100644
>> --- a/net/tls/tls_main.c
>> +++ b/net/tls/tls_main.c
>> @@ -833,15 +833,29 @@ static int do_tls_setsockopt_no_pad(struct sock *sk, sockptr_t optval,
>>   	return rc;
>>   }
>>
>> +/* priv_ctx_tx holds a different structure on each TX path, so tx_conf has to
>> + * say which open record to look at.
>> + */
>> +static bool tls_tx_record_is_open(struct tls_context *ctx)
>> +{
>> +	switch (ctx->tx_conf) {
>> +	case TLS_SW:
>> +		return !!tls_sw_ctx_tx(ctx)->open_rec;
>> +	case TLS_HW:
>> +		return !!tls_offload_ctx_tx(ctx)->open_record;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>>   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)
>> +	if (tls_tx_record_is_open(ctx))
>>   		return -EBUSY;
> [Severity: High]
> For the TLS_HW arm, is open_record enough to tell that no record is being
> built?  do_tls_setsockopt() calls this helper with only the socket lock
> held:
>
> 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);
> }
>
> but the offload TX path is serialized by ctx->tx_lock, and it drops the
> socket lock at a point where open_record is already NULL:
>
> net/tls/tls_device.c:tls_push_record() {
> 	offload_ctx->open_record = NULL;
> 	...
> 	return tls_push_sg(sk, ctx, offload_ctx->sg_tx_data, 0, flags);
> }
>
> tls_push_sg() calls tcp_sendmsg_locked(), which for a blocking sendmsg()
> with a full send buffer waits in sk_stream_wait_memory() and releases the
> socket lock there.  tls_push_data() has a second such wait on the
> tls_do_allocation() failure path.
>
> So can a concurrent setsockopt() see open_record == NULL, take the "not
> open" branch and publish a smaller ctx->tx_max_payload_len while a sender
> is parked mid-sendmsg holding tx_lock?


Yes, this window exists, but it seems to be a pre-existing issue. I 
think I can fix it by taking tx_lock.

> If so, does the offload path then keep using the old limit for the rest of
> that sendmsg?  tls_push_data() latches it once, before the loop, and never
> refreshes it:
>
> net/tls/tls_device.c:tls_push_data() {
> 	max_open_record_len = tls_ctx->tx_max_payload_len +
> 			      prot->prepend_size;
> 	do {
> 		...
> 		copy = min_t(size_t, size, max_open_record_len - record->len);
> }
>
> That would mean setsockopt returns 0 while records larger than the new
> limit continue to go out, which does not match what
> Documentation/networking/tls.rst says about TLS_TX_MAX_PAYLOAD_LEN ("the
> kernel enforces the specified limit on all outgoing TLS records.  No
> plaintext fragment will exceed this size").
>
> There is a follow-on question about the next call.  If the sender leaves a
> record open across the syscall (MSG_MORE with record->num_frags <
> MAX_SKB_FRAGS - 1), the next tls_push_data() latches the new, smaller
> max_open_record_len while record->len is still the old, larger value.
> max_open_record_len is u32 and record->len is int, so does
>
> 	copy = min_t(size_t, size, max_open_record_len - record->len);
>
> wrap around here and drop the record-size clamp entirely, letting
> tls_append_frag() grow the record past the 2^14 plaintext maximum before
> "record->len >= max_open_record_len" closes it?
>
> Note that tls_sw_sendmsg_locked() re-reads the limit on every iteration:
>
> net/tls/tls_sw.c:tls_sw_sendmsg_locked() {
> 		record_room = tls_ctx->tx_max_payload_len - msg_pl->sg.size;
> }
>
> so the latched offload value looks like the odd one out.
>
> Would it be better to serialize the check and the store against the lock
> that actually protects record building?  Since tls_device_sendmsg() takes
> tx_lock before lock_sock:
>
> net/tls/tls_device.c:tls_device_sendmsg() {
> 	mutex_lock(&tls_ctx->tx_lock);
> 	lock_sock(sk);
> }
>
> tx_lock is the outer lock, so taking it inside the already socket-locked
> helper is not an option; the acquisition in do_tls_setsockopt() would have
> to be reordered (or the helper made to wait for in-flight senders, as
> tls_sk_proto_cleanup() does on sk_write_pending).
>
> One smaller point: the plain read of tls_ctx->tx_max_payload_len in
> tls_push_data() against the plain write in
> do_tls_setsockopt_tx_payload_len() is an unannotated data race that KCSAN
> would flag.

I don't think so: all tls_push_data() callers hold the socket lock, and the

write is under the socket lock too, so the accesses are serialized.